| | <<<Back 1 day (to 2017/06/15) | 20170616 |
| electrolex | :) | 02:33.10 |
| kroovy | Robin_Watts: awesome! we need to put that in the manpage! :)) | 06:46.46 |
| avih | tor8: two unrelated issues: 1. re pkgconfig, i got a comment that it's much more useful when version is not a random hash, since this allows testing for a minimum version. maybe use tags and commit-count since last tag? 2. it seems that Number.toString(radix) doesn't support radix which isn't 10. | 08:52.52 |
| | it does test for the correct range of 2-36, but then throws if it's not 10 :) | 08:54.08 |
| | if you don't want a generic radix support, i'd say at the very least you should also support 2, 8 and 16 | 08:55.09 |
| | (but then, it will be nicer to support it properly) | 08:55.49 |
| | re pkg version, i think it makes sense to choose 0.8 or 0.9 for the current state, since it's stable but maybe allow some wider feedback before calling it 1.0 | 09:00.54 |
| | (and the api did change only recently, with setreport at least) | 09:01.46 |
| tor8 | avih: API additions are backward compatible at least :) | 10:37.20 |
| | the random hash will be based on the release tags (once I do a release) | 10:37.38 |
| avih | tor8: so any objection to tagging it 0.8 or so now? | 10:45.39 |
| | (and yes, i do realize this change was backward compatible ;) ) | 10:46.08 |
| | you don't have to call it a release, just to get a usable pkgconfig version | 10:48.44 |
| tor8 | I don't mind, it's long past time to make a release. I'm just so used to working off of git :) | 10:49.15 |
| avih | everyone is :) and tagging it _is_ working off git :) | 10:51.51 |
| | has there ever been a mujs "release"? | 10:52.35 |
| tor8 | sorry, I mean "I'm so used to working off of git and not looking at the release tags" | 10:52.50 |
| Robin_Watts | tor8: 2 small commits: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=a1857dfdc5d4b7988b19e59092dc5a80794569d4 | 10:56.57 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=c87adf950ad83fc9737cf22d91943e0fc21352fd | 10:57.08 |
| tor8 | Robin_Watts: LGTM. | 10:57.45 |
| Robin_Watts | Ta. | 10:57.51 |
| tor8 | mupdf-gl has a slightly more advanced version, but that's not of huge significance | 10:58.07 |
| | mupdf-gl can take URL #fragments with named destinations | 10:58.22 |
| | https://mupdf.com/docs/manual-mupdf-gl.html | 10:58.29 |
| | oh, that's not really documented there | 10:58.46 |
| | it just says "[page]" | 10:59.02 |
| Robin_Watts | tor8: one more: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=d20714c0da5c07268687817f964c88b77308c22e | 11:03.36 |
| tor8 | Robin_Watts: LGTM | 11:03.56 |
| Robin_Watts | Ta. | 11:04.00 |
| | tor8: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=2ee8e02105ae06045203b5878bb204507c2678e3 | 11:11.49 |
| | I think that leaves the lcms branch pretty much ready to go. | 11:12.12 |
| | I'll prepare a squashed branch for final review. | 11:12.22 |
| tor8 | Robin_Watts: MSVC missing header commit LGTM | 11:13.52 |
| Robin_Watts | tor8: Thanks. | 11:20.40 |
| | "Final" Squashed commit for review: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=00d45e48cc3bbb3693b0aab33597606cca561164 | 11:21.06 |
| tor8 | avih: you got your wish. I've tagged a 1.0.0 release (hey, it's the first release ever, it *needs* to be 1.0) | 11:21.27 |
| Robin_Watts | tor8: Or 0.01 ;) | 11:21.50 |
| avih | heh. thanks! checking what the pkgconfig version is now. | 11:21.57 |
| Robin_Watts | but yes, 1.0 seems more appropriate. | 11:22.00 |
| tor8 | I thought about 0.0.1 :) | 11:22.02 |
| | avih: when you're on the tag it should be 1.0.0 | 11:22.16 |
| | when you're on some non-release git commit it'll be 1.0.0-gX-SHA1 where X is number of commits since the tag | 11:22.39 |
| Robin_Watts | tor8: Possibly I should have kept the PSD device and the rasterizer/anti-alias -> draw device commits out of the squashed one. | 11:22.45 |
| avih | tor8: hmm.. i think that won't allow checking minimum pkg version. | 11:23.26 |
| | i'm not entirely sure, but i'd think major.minor.commits-since-tag would be a more common kind of version number/string | 11:24.11 |
| Robin_Watts | tor8: I see you renamed the new virtual ops for ExtGState entries. | 11:24.52 |
| tor8 | Robin_Watts: looking at the PSD commit, I think it'd make some sense to merge write_header and write_icc into one call (i.e. just add an optional colorspace argument to fz_write_header) | 11:25.32 |
| | Robin_Watts: yes, I did. | 11:26.28 |
| | Robin_Watts: I did not add separate virtual ops for extgstate entries that have regular ops that do the same thing | 11:26.56 |
| | if that's your next question | 11:27.04 |
| Robin_Watts | right. I'm just thinking that we have virtual ops for Xobjects too... | 11:27.18 |
| tor8 | avih: this is what 'git describe --tags --always' returns | 11:27.20 |
| | Robin_Watts: those are named op_Do_image/shade/etc | 11:27.35 |
| Robin_Watts | Right, so already renamed (was just trying to lookup to see) | 11:27.51 |
| tor8 | yeah, the "Do" operator is split into an op_Do_image and op_Do_form depending on whether it's an image or form XObject | 11:28.36 |
| avih | tor8: "this" being what you posted above? | 11:28.40 |
| tor8 | avih: yes, the TAGNAME-gX-SHA1 string | 11:29.07 |
| | ah, my bad, it's TAG-X-gSHA1 | 11:29.42 |
| | 1.11-238-g97974326a is my current mupdf git checkout 'version' string | 11:29.58 |
| avih | tor8: let me try to play with it a bit and see if i can come up with something useful. can we say the tag will be "1.0" ? | 11:30.03 |
| tor8 | avih: the tag is 1.0.0 and as long as you're using a tag with no further commits then "1.0.0" is what you'll get | 11:30.31 |
| | this is if you install a non-release-tag build | 11:30.42 |
| avih | huh? | 11:31.01 |
| | oh | 11:31.05 |
| tor8 | sorry, being a bit unclear there. if you install a non-release-tag-build you'll get "tag-1.0.0-52-gABCDEF0" type versions | 11:31.24 |
| avih | but if you install git master then you get a non numeric version | 11:31.34 |
| tor8 | if you install a release tag build you'll get "1.0.0" versions | 11:31.43 |
| avih | or at least non numeric suffix | 11:31.43 |
| | let me play with describe a bit. i think it could be made more numeric. | 11:32.11 |
| tor8 | I like the git-describe versions we have now, they make it absolutely clear what version you've built | 11:32.35 |
| | and also some human readability in how far they've diverged thanks to the distance number | 11:32.55 |
| avih | yes, describe is great, but it doesn't necessarily make a great pkg config version | 11:34.08 |
| | i'll also read how the version field is treated in pkgconfig | 11:34.35 |
| | for reference, i added one commit on top of your master, and indeed it looks like this: 1.0.0-1-gfe3a9b5 | 11:39.07 |
| | which, before exploring further, i think that for the sake of a version string, would have been better as 1.0.1 | 11:39.47 |
| | so, with that version string, `pkg-config --modversion mujs` prints exactly this string. so it doesn't try to extract something which looks like numeric version number from it | 11:43.01 |
| | tor8: at least as far as pkgconfig is concerned with comparing order, it seems to be ok. i'm testing with this template: | 11:50.04 |
| | pkg-config --exists 'mujs > 1.0.1' && echo ok || echo not-found | 11:50.06 |
| | and it prints ok for 0.9, 1.0 but not-found for 1.0.1. | 11:50.43 |
| Robin_Watts | tor8: Yes, I think moving the write_icc into the write_header would make sense. | 11:52.42 |
| | tor8: I stripped this out separately as it makes sense independently of the branch: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=e644f4711bc2d087eb18e3d9a099b639990d041a | 11:53.56 |
| avih | tor8: thanks for the quick updates on master/github :) | 12:19.28 |
| Robin_Watts | tor8: OK, updated lcms_squashed branch: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=shortlog;h=refs/heads/lcms_squashed | 12:34.37 |
| | That has the combination of fz_write_icc into fz_write_header | 12:34.58 |
| | If you like it, I'll figure out the git runes for setting the author to be michael before pushing it :) | 12:36.18 |
| avih | tor8: thanks. fyi, merged pkgconfig https://github.com/mpv-player/mpv/commit/071118e4 | 12:47.47 |
| | so where were we.. ah, Number.toString(2) :) | 12:49.22 |
| Robin_Watts | tor8: In fz_get_icc_link, how is the "/* On failure use the default */" stuff ever called? | 14:04.55 |
| tor8 | if fz_cmm_new_profile fails to populate src_icc->cmm_handle? | 14:18.21 |
| Robin_Watts | tor8: Right, but unless I'm blind, it always throws on failure ? | 14:20.07 |
| tor8 | I haven't looked that closely | 14:21.50 |
| | my review so far has been of the public surface and shape, I have not yet dug into the internals | 14:22.15 |
| sebras | three minor commits on sebras/master | 14:25.26 |
| tor8 | sebras: all look reasonable to me, if fz_defer_reap_start and fz_purge_glyph_cache are guaranteed to not throw | 14:28.07 |
| Robin_Watts | tor8: Also (and I'm mentioning it here more so I have a list than because I expect you to be able to answer definitively) icc_base_conv_pixmap doesn't throw on error - is that right? | 14:30.51 |
| sebras | fz_defer_reap_start() only calls fz_(un)lock() today and fz_purge_glyph_cache() calls free/drop functions. | 14:32.29 |
| | tor8: I could add them to fz_try()s which swallow any errors if you think that is advicable. | 14:32.51 |
| | fz_purge_glyph_cache() also calls fz_glyph_size() which doesn't throw today. | 14:33.09 |
| Robin_Watts | sebras: What do you think the problem is? | 14:33.49 |
| | I can't see any problems with fz_defer_reap_start. | 14:35.12 |
| | I could maybe see that fz_defer_reap_end could have some try/catchery to ensure that it never threw with it locked. | 14:35.42 |
| | sebras: Your first commit looks like a case for fz_new_link_drop to me. | 14:36.24 |
| | Your second commit looks exactly wrong to me :) | 14:37.44 |
| sebras | Robin_Watts: I know, that is frequently the case. | 14:38.15 |
| Robin_Watts | We definitely need a try/always there to make sure that fz_defer_reap_end is always called. | 14:38.44 |
| | We possibly could not throw in the catch though. | 14:38.55 |
| | And the third one lgtm. | 14:39.39 |
| sebras | today there is only one place where fz_new_link() takes something that needs to be freed, at the other two places the URI is a borrowed reference. | 14:40.56 |
| | and since fz_new_link() internall strdups the uri then I guess that is also the prefered way to do ti. | 14:41.29 |
| | if there were several instances where it needed to be freed I'd think that adding fz_new_link_drop() would make sense. | 14:42.03 |
| Robin_Watts | fz_new_link_drop would be clearer to read (and faster to execute) and almost no extra code. But yes, it's not a compelling case without another use of it. | 14:42.50 |
| sebras | tor8: fz_new_link_drop() or keep the commit as is? | 14:43.14 |
| Robin_Watts | sebras: I'm happy with it as is, if you prefer it like that. | 14:43.38 |
| sebras | Robin_Watts: the 2nd commit comes about since we have a fz_throw() in fz_decouple_type3_font(). | 14:44.16 |
| | Robin_Watts: if it throws, then we decide not to free anything after that call. | 14:44.28 |
| | Robin_Watts: so I opted to swallow that error and then keep the other drops without any fz_try() as they are not allowed to throw. | 14:45.10 |
| Robin_Watts | sebras: Ah, I see. | 14:45.15 |
| | but fz_purge_glyph_cache *can* throw, you said ? | 14:45.29 |
| sebras | Robin_Watts: no, not today. | 14:45.46 |
| Robin_Watts | In that case I withdraw my objections. | 14:46.15 |
| | You are right, and it lgtm. | 14:46.18 |
| sebras | Robin_Watts: but since it calls fz_glyph_size() which calls fz_pixmap_size() it might be an accident waiting to happen. | 14:46.24 |
| | should I be adding fz_try() for fitz calls like these? | 14:47.41 |
| Robin_Watts | fz_pixmap_size should never throw (he said, with his fingers crossed) | 14:48.03 |
| sebras | Robin_Watts: yeah, about those fingers. :) | 14:48.21 |
| tor8 | sebras: I'd go for future proofing and try/catch/ignore guard the purge_glyph_cache call | 14:49.07 |
| | sebras: and I'd not bother with fz_new_link_drop until we use more than once | 14:49.23 |
| sebras | Robin_Watts: one thing you mentioned though: why would fz_new_link_drop() be faster to execute? wouldn't we just be moving the code to another public function and so actually having the jump jumping to the code we alreay have inline in... pdf_load_link()? | 14:49.25 |
| Robin_Watts | sebras: We could implement fz_new_link_drop with no strdup and no free. | 14:49.51 |
| | implement fz_new_link in terms of fz_new_link_drop, in fact? | 14:50.47 |
| sebras | Robin_Watts: ah! _now_ I got what you meant! :) | 14:51.03 |
| tor8 | Robin_Watts: sebras: but strings aren't reference counted, so that'd be hard | 14:51.31 |
| Robin_Watts | tor8: eh? | 14:52.40 |
| tor8 | or maybe I'm just reading too much into it... | 14:52.50 |
| sebras | tor8: I guess the only requirement would be that it is allocated by the same allocator that fz_free() would call, no? | 14:53.04 |
| Robin_Watts | fz_new_link currently strdups the uri, returns and then the caller (at least once) frees it. | 14:53.09 |
| tor8 | Robin_Watts: some places the string is a static array | 14:53.34 |
| Robin_Watts | fz_new_link could be implemented by strdupping and then calling fz_new_link_drop, which does what fz_new_link does, but doesn't have to strdup. | 14:53.38 |
| | tor8: so such places must never call fz_new_link_drop :) | 14:53.58 |
| tor8 | fz_new_link_own would be a better name | 14:54.13 |
| Robin_Watts | I am failing to see a problem here. Probably being myopic again. | 14:54.27 |
| | tor8: _drop is the convention throughout MuPDF though. | 14:54.40 |
| tor8 | confusing naming, if fz_new_link_drop doesn't actually drop :) | 14:54.40 |
| Robin_Watts | _drop implies "the reference is passed in" | 14:54.57 |
| tor8 | it does, so maybe it was a bad choice of name to begin with (detailing the how, not the why) | 14:55.17 |
| sebras | we do have _own() in the JNI and murun code atm. | 14:55.38 |
| | ffi_pushimage_own() e.g. | 14:56.01 |
| tor8 | sebras: anyway, I think the code as you wrote it is fine. that's a very common idiom. | 14:56.28 |
| | wrapping it up is handy if it's a pattern that keeps repeating throughout | 14:56.47 |
| Robin_Watts | sebras: Yes, ignore my burbling, it was merely an observation. | 14:56.50 |
| sebras | and I intend to swallow any at the moment non-existent exception from fz_purge_glyph_cache(). | 14:57.15 |
| tor8 | like what paul did when he (fantastically) introduced the pdf_dict_put_drop family of functions | 14:57.24 |
| sebras | tor8: ok, last LGTM on sebras/master~1? | 15:00.33 |
| tor8 | sebras: LGTM | 15:06.55 |
| sebras | another triplet of small patches on sebras/master in the same vein as the previous one's. | 15:21.30 |
| Robin_Watts | sebras: All lgtm | 15:30.16 |
| | sebras, tor8: I'm still looking for reviews of: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=691a6b2c26767186ee7e0e7d0813391f41ac0850 | 15:31.36 |
| | and http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=6c36b5ce05013fd336e318ad414edc5de0c2739c | 15:31.46 |
| | The second one of those is definitely sebras shaped. | 15:32.11 |
| sebras | Robin_Watts: so... short and slightly rounded? ;) | 15:36.10 |
| | the first commit is because of commit f84a189d5f94250e46d2cbd1a75aba00130e2dd6 | 15:38.38 |
| | and to me it looks reasonable. | 15:39.48 |
| Robin_Watts | It was michaels commit originally, and I've rebased it out of the branch. it looks good to me too. | 15:40.32 |
| | Morning mvrhel_laptop: | 15:41.09 |
| | mvrhel_laptop: I have questions when you are set up and ready to go. | 15:41.38 |
| mvrhel_laptop | Morning Robin_Watts | 15:41.39 |
| Robin_Watts | (no immediate hurry, I'm making a list) | 15:42.06 |
| mvrhel_laptop | ok. we can do it now. I am close to having this icc-create stuff fixed | 15:42.14 |
| | Robin_Watts: if you want to wait give me 20 minutes... | 15:42.27 |
| | make your list long.... | 15:42.36 |
| Robin_Watts | sure. 20 minutes it is :) | 15:42.40 |
| sebras | Robin_Watts: agh, why isn't jsize defined as a size_t as you would be lead to believe? :-/ | 15:42.42 |
| | Robin_Watts: 2nd patch LGTM. | 15:43.19 |
| Robin_Watts | sebras: Thanks. | 15:43.25 |
| sebras | Robin_Watts: in the absence of tor8, do you might taking a look at the three commits on sebras/master? | 15:43.55 |
| | I'm doing my best to get rid of these things that I have had stacked on different branches. | 15:44.15 |
| Robin_Watts | sebras: A third 3? | 15:44.16 |
| sebras | Robin_Watts: oh, I missted your LGTM, but there will be a third three in 1min. :) | 15:44.58 |
| Robin_Watts | I lgtm them just before I gave links to mine. | 15:44.58 |
| Robin_Watts | fetches a drink then. | 15:45.07 |
| mvrhel_laptop | oh. the old icc-create.c code had issues too with its description tags. Robin_Watts I am ready when you are | 16:02.52 |
| sebras | Robin_Watts: ok so there are five patches on sebras/master | 16:03.08 |
| Robin_Watts | mvrhel_laptop/sebras: OK, typing on 2 keyboards, one with each hand... | 16:03.37 |
| mvrhel_laptop | luckily you had the connections between your brain hemispheres severed | 16:04.07 |
| Robin_Watts | mvrhel_laptop: In fz_get_icc_link, how is the "/* On failure use the default */" stuff ever called? | 16:04.20 |
| mvrhel_laptop | ok hold on | 16:04.33 |
| Robin_Watts | mvrhel_laptop: Yeah, they did that while they were in there for the lobotomy. | 16:04.47 |
| sebras | Robin_Watts: the document writer commit at the top might be controversial. | 16:04.54 |
| mvrhel_laptop | Robin_Watts: fz_cmm_new_profile(ctx, src_icc); could fail if the profile was not valid | 16:05.25 |
| Robin_Watts | mvrhel_laptop: My reading of the code says it will throw. | 16:05.44 |
| mvrhel_laptop | oh we dont want that | 16:05.52 |
| Robin_Watts | Ok, so that might have been a change from tor. | 16:06.05 |
| mvrhel_laptop | hold on let me dig | 16:06.09 |
| | fz_lcms_new_profile does not throw | 16:06.53 |
| | oh it does | 16:06.58 |
| | if we do that, we need to catch upstream and then use the default | 16:07.20 |
| Robin_Watts | sebras: first one lgtm. | 16:07.55 |
| mvrhel_laptop | It used to just return a null for the handle | 16:08.02 |
| | hence the test | 16:08.05 |
| | but a throw would be better | 16:08.27 |
| | I suppose | 16:08.33 |
| Robin_Watts | mvrhel_laptop: A return code might be even better? | 16:09.17 |
| mvrhel_laptop | Robin_Watts: so I am fine either way. Basically though if the profile creation fails from the cmm then we should use the appropriate default | 16:09.42 |
| Robin_Watts | if (fz_cmm_new_profile(ctx, src_icc)) { /* failed so use default */ .... } | 16:10.02 |
| mvrhel_laptop | that would be nice and clean | 16:10.12 |
| Robin_Watts | I can push that through in a mo. | 16:10.23 |
| | mvrhel_laptop: Next question: icc_base_conv_pixmap doesn't throw on error - is that right? | 16:11.00 |
| | sebras: 2 and 3 lgtm. | 16:11.54 |
| | 4 makes me worried. | 16:11.58 |
| | Can we wait on that until after the lcms branch goes in, cos it'll conflict, I fear. | 16:12.14 |
| | Or at least let me check :) | 16:12.20 |
| mvrhel_laptop | Robin_Watts: so some of the functions in there will throw | 16:13.42 |
| Robin_Watts | mvrhel_laptop: But why doesn't the final catch? | 16:13.58 |
| sebras | Robin_Watts: looks like both branches affect fz_init_cached_color_converter(), but in different locations. | 16:14.15 |
| Robin_Watts | sebras: Leave that one with me. | 16:14.31 |
| | The 5th one... | 16:14.39 |
| sebras | Robin_Watts: I think the 5th one might need tor8./ | 16:15.00 |
| Robin_Watts | Given that the document_writer is the thing that creates the fz_device, it seems very wrong to be passing the device back into it. | 16:15.06 |
| mvrhel_laptop | Robin_Watts: ok so lets say that fz_new_pixmap_with_bbox fails. What should it do with that? I thought it would get passed up | 16:15.31 |
| Robin_Watts | mvrhel_laptop: It should. | 16:15.41 |
| sebras | Robin_Watts: in that case there is a pattern of fz_begin_page()...fz_end_page() where if fz_begin_page() is called a fz_device is allocated and since fz_end_page() might not be called the device is never freed. | 16:16.25 |
| | Robin_Watts: and the device is stored internally inside the document writer. | 16:16.48 |
| Robin_Watts | mvrhel_laptop: Everything in that function seems ideal to me, except after the fz_catch(ctx) at the bottom, surely we should fz_rethrow(ctx) ? | 16:17.02 |
| | sebras: Indeed. | 16:17.07 |
| mvrhel_laptop | when fz_icc_conv_pixmap finishes the base pix map is dropped with the always even if something throws in fz_icc_conv_pixmap | 16:17.08 |
| | oh . Yes it should rethrow | 16:17.27 |
| | so the next level knows | 16:17.37 |
| Robin_Watts | mvrhel_laptop: Cool. I have that fixed locally. | 16:17.39 |
| mvrhel_laptop | sorry | 16:17.41 |
| Robin_Watts | No worries. | 16:17.45 |
| sebras | Robin_Watts: though I guess it could be moved into fz_drop_document_writer() instead..? | 16:17.48 |
| Robin_Watts | sebras: that might be better. I'd need to look at it a bit more closely. | 16:18.13 |
| | mvrhel_laptop: Ok, last one for now... fz_draw_fill_shade | 16:18.28 |
| | No, sorry, fz_draw_clip_text | 16:19.30 |
| | In that, we can end up calling fz_draw_fill_path(...) to draw the path of the text as a clip. | 16:20.02 |
| mvrhel_laptop | ok. I don't think I touched that.... | 16:20.19 |
| | or at least I hope I did not | 16:20.26 |
| Robin_Watts | No, you didn't, but hear me out :) | 16:20.37 |
| mvrhel_laptop | I see the white.... | 16:20.55 |
| Robin_Watts | We draw the clip using a greyscale space with a value of 1. | 16:20.57 |
| | the plan being that we'll get a pixmap full of bytes varying from 0 to 255 that we can use as a mask. | 16:21.31 |
| | We pass in NULL as the color_params, so we use the default ones. | 16:21.46 |
| | This is presumably going to use the default greyscale profile. | 16:22.12 |
| | which won't necessarily map 1 to 255, right? | 16:22.28 |
| mvrhel_laptop | is the target gray scale too? | 16:22.53 |
| | i.e. the model | 16:23.00 |
| | model = state->dest->colorspace; | 16:23.13 |
| | I guess its the target device | 16:23.19 |
| | yeah this may be an issue | 16:23.33 |
| Robin_Watts | dest = mask at this point, so it's either greyscale or no color space. | 16:23.39 |
| mvrhel_laptop | oh if its gray scale we should be safe | 16:24.04 |
| | identity transform should occur | 16:24.11 |
| | i.e. no transform | 16:24.16 |
| | color management is skipped during the transform | 16:24.57 |
| Robin_Watts | Even if we have a screwy proofing profile? (he says, aware that he might be exposing the limits of his understanding here) | 16:25.18 |
| | This might bear a quick test. | 16:26.03 |
| mvrhel_laptop | hmm ok. let me check that. could be if we had an output intent things would not work. we may need to add an option to avoid proofing in such a case | 16:26.13 |
| | hold ono | 16:26.15 |
| Robin_Watts | sebras: so, 1-3 lgtm, 4 = wait a mo, and 5 = you are tweaking ? | 16:27.14 |
| | sebras: If you push 1-3, I'll try to rebase on top of 1-4 and then if that works, you can push it too :) | 16:27.51 |
| mvrhel_laptop | oh | 16:28.57 |
| | is_identity is getting set true even when a proof profile is present. That needs to be fixed | 16:30.54 |
| | ok. We are going to need to add another item to fz_color_params | 16:31.59 |
| | to let the fz_new_icc_link know that it should not use the proof profile | 16:32.18 |
| Robin_Watts | really? :( | 16:32.19 |
| mvrhel_laptop | in cases like that | 16:32.21 |
| | Unless you can think of another way to do it | 16:32.38 |
| | Right now we have | 16:32.58 |
| | fz_new_icc_link(fz_context *ctx, fz_iccprofile *src, fz_iccprofile *prf, fz_iccprofile *dst, const fz_color_params *rend, int num_bytes, int alpha) | 16:33.00 |
| Robin_Watts | but that should never be signalled across the device interface? | 16:33.07 |
| mvrhel_laptop | I guess I am having trouble understanding how and when this text clip mask is created | 16:34.19 |
| | lets go back there for a sec | 16:34.34 |
| Robin_Watts | I think the same problem may occur more generally with clipping, but this was the case I spotted. | 16:35.08 |
| | So, the device call is fz_draw_clip_text, and is called to say "set the clip region to the area of this text" | 16:35.53 |
| mvrhel_laptop | Robin_Watts: to be honest, due to the way white is handled in the cmm, I don't think it is going to be an issue even with proofing | 16:35.56 |
| | the only time you would end up with something non-white would be if someone was doing absolute colorimetric rendering | 16:36.19 |
| | and in that case even the white background of the page should be off white | 16:36.35 |
| Robin_Watts | OK, but let me explain it and you can see whether there are other problems. | 16:36.45 |
| mvrhel_laptop | but I do need to fix this one thing with is_identity | 16:37.42 |
| Robin_Watts | The way we do (non rectangular) clipping in MuPDF is to make pixmap 'masks'. | 16:37.43 |
| | They basically encode how much of a pixel shows through the clipped region. | 16:38.10 |
| | 0 = none, 255 = all. | 16:38.18 |
| | so we draw subsequent content to a pixmap (unclipped) and then when the clip is popped, we mask the image back to the underlying pixmap. Make sense? | 16:39.04 |
| | (effectively clips are extremely akin to softmasks) | 16:39.36 |
| mvrhel_laptop | the subsequent content being the unclipped image | 16:39.50 |
| | ? | 16:39.52 |
| Robin_Watts | Yeah, I explained that badly. | 16:40.02 |
| mvrhel_laptop | I follow it | 16:40.24 |
| Robin_Watts | Suppose we have a backdrop A, and we start a clip. We render the clip to a mask,M, and then draw the contents to a new pixmap copied from A, B. | 16:40.47 |
| | When the clip is finished we plot B back onto A through M. | 16:40.57 |
| sebras | Robin_Watts: I removed 4 from sebras/master (temporarily) | 16:41.46 |
| | Robin_Watts: then redid 5 to make fz_end_page() do most of the wrok. | 16:42.06 |
| mvrhel_laptop | Robin_Watts: hold on I am being slow | 16:42.08 |
| | ...and then draw the contents to a new pixmap copied from A, B. | 16:42.24 |
| | Not sure what B was in this | 16:42.38 |
| | you defined A | 16:42.46 |
| Robin_Watts | We copy A to give a new pixmap B. | 16:42.48 |
| mvrhel_laptop | you defined M | 16:42.50 |
| | ok | 16:42.55 |
| Robin_Watts | Then we draw the contents onto B. | 16:43.00 |
| | so B ends up containing the original contents of A overlaid with the new contents. | 16:43.17 |
| mvrhel_laptop | You copy A to B | 16:43.18 |
| | what new contents? | 16:43.25 |
| Robin_Watts | OK, concrete example time. | 16:43.36 |
| mvrhel_laptop | you need to turn down the hose pressure.... ;) | 16:44.00 |
| Robin_Watts | Imagine I have a file that has a red background, then a circular clip that contains a green square. | 16:44.16 |
| | So we interpret that file, and we draw the red background. | 16:44.32 |
| | That is pixmap A. | 16:44.47 |
| | now we get told to start a circular clip. | 16:44.57 |
| | so we create a mask M, and draw the circle onto it. | 16:45.11 |
| | Now, we need to create a new pixmap B, to render the contents of the clip onto. | 16:45.28 |
| | and we do that by copying A. | 16:45.37 |
| | Now we render the green square onto B. | 16:45.56 |
| mvrhel_laptop | I see | 16:46.07 |
| Robin_Watts | so B has the red background + green square (unclipped) on it. | 16:46.08 |
| | Now the clip ends, so we write B back to A through M. | 16:46.24 |
| mvrhel_laptop | gotcha | 16:46.30 |
| Robin_Watts | and our final image is on A. | 16:46.34 |
| | cool. | 16:46.36 |
| mvrhel_laptop | hehe | 16:46.39 |
| | sorry I am a little slow this morning... | 16:46.52 |
| Robin_Watts | (Obviously, we only make the copied bitmaps as large as they have to be) | 16:47.03 |
| | no worries. | 16:47.18 |
| mvrhel_laptop | ok. It really would be ideal to have a non-cm path for creating M | 16:47.20 |
| Robin_Watts | It would. | 16:47.28 |
| mvrhel_laptop | one thing I did in gs might work nicely here | 16:47.38 |
| | essentially I am thinking we could use a different cmm on the fly when we are dealing with mask creation | 16:48.50 |
| | I would think with the abstraction you did (which is similar to what is in gs) we could do that without too much trouble | 16:49.34 |
| | it just requires a little work in tying in the non-cm methods that exist in mupdf today into the methods use to convert colors and pixmaps | 16:50.44 |
| mvrhel_laptop | is wondering if I reversed the hose | 16:51.27 |
| Robin_Watts | sorry, back. | 16:51.38 |
| | mvrhel_laptop: eek, no. | 16:51.54 |
| mvrhel_laptop | this works nicely in gs | 16:52.10 |
| Robin_Watts | We can't swap cmm engine on the fly, cos the private contents of the colorspaces are cmm engine specific. | 16:52.23 |
| mvrhel_laptop | well you have to realize what you are doing in this case | 16:52.50 |
| Robin_Watts | Currently the cmm engine in use is a global setting. | 16:53.07 |
| mvrhel_laptop | hmm | 16:53.13 |
| | ok | 16:53.20 |
| Robin_Watts | We can't enable/disable that in one thread cos another thread might be doing operations too. | 16:53.47 |
| mvrhel_laptop | This other cmm would only use the "n" value to determine what transform to do | 16:53.53 |
| | ok | 16:54.13 |
| | that is true | 16:54.16 |
| | too bad | 16:54.20 |
| Robin_Watts | There are big red (metaphorical) letters in the docs for setting the cmm_engine for this reason. | 16:54.30 |
| | but I think I see what you mean about another color_params bit. | 16:55.02 |
| | I'm just trying to get my head around how widespread that would be. | 16:55.22 |
| mvrhel_laptop | yes. fz_draw_clip_text (and others) would set it appropriately | 16:55.25 |
| sebras | Robin_Watts: need to sleep. I pushed 1-3 for now, and I'll continue preparing the other commits during the weekend and have them ready for you guys on monday. :) | 16:55.33 |
| mvrhel_laptop | and the get_link would know that hey I am going to use identity here | 16:55.36 |
| | and not color manage | 16:55.44 |
| Robin_Watts | sebras: Ta. | 16:55.44 |
| | mvrhel_laptop: It is *possible* that we always use the NULL colorspace for alphas and masks. | 16:56.16 |
| mvrhel_laptop | so when the transform (color convert) call occurs the values are passed unmolested | 16:56.18 |
| Robin_Watts | so could you maybe key off that ? | 16:56.23 |
| mvrhel_laptop | Robin_Watts: hmm I don't know what in the world happens now if the src is NULL | 16:57.08 |
| | so model = state->dest->colorspace; could be NULL? | 16:57.24 |
| Robin_Watts | It is possible that that would be the case :) | 16:57.43 |
| mvrhel_laptop | And what does dest = fz_new_pixmap_with_bbox(ctx, model, &bbox, state[0].dest->alpha); do? | 16:57.44 |
| | oh | 16:57.58 |
| | alpha | 16:58.00 |
| | so it says hey I am pure alpha | 16:58.10 |
| | ok looking to see what the cmm does in this case | 16:58.37 |
| Robin_Watts | yeah. | 16:58.39 |
| mvrhel_laptop | hold on | 16:58.39 |
| | oh fz_draw_fill_path(ctx, devp, path, 0, in_ctm, fz_device_gray(ctx), &white, 1, NULL); uses device_gray | 17:00.12 |
| Robin_Watts | brb, must feed puppy. | 17:00.39 |
| mvrhel_laptop | oh but fz_draw_fill_path could still have model NULL | 17:01.31 |
| | oh its not converted in that case | 17:02.13 |
| | n = fz_colorspace_n(ctx, model); | 17:02.16 |
| | if (n > 0) | 17:02.18 |
| | { | 17:02.19 |
| | fz_convert_color(ctx, color_params, prf, model, colorfv, colorspace, color); | 17:02.21 |
| | for (i = 0; i < n; i++) | 17:02.22 |
| | colorbv[i] = colorfv[i] * 255; | 17:02.24 |
| | } | 17:02.25 |
| | else | 17:02.27 |
| | i = 0; | 17:02.28 |
| | colorbv[i] = alpha * 255; | 17:02.30 |
| | as n = 0 | 17:02.31 |
| | It is not even using white | 17:03.12 |
| | is uses the alpha =1 value | 17:03.18 |
| | from the call to fz_draw_fill_path | 17:03.29 |
| | So the mask in this case is handled with no color management | 17:03.50 |
| | So we don't need to do anything | 17:04.03 |
| Robin_Watts | back. | 17:06.00 |
| mvrhel_laptop | Robin_Watts: so looks like the case when the model is null is handled fine | 17:06.16 |
| Robin_Watts | cool. so as long as I make sure that model == NULL in the mask case, we're OK. | 17:06.42 |
| mvrhel_laptop | yes | 17:06.47 |
| Robin_Watts | Nice. | 17:06.52 |
| mvrhel_laptop | Robin_Watts: so you did uncover the one item which is that is_identity should not be set if proofing profile is present | 17:08.05 |
| | I will fix that after I fix this irritating item with icc-create | 17:08.26 |
| Robin_Watts | cool. | 17:08.50 |
| mvrhel_laptop | Then I wonder if we are almost to a merge point. What do you think? | 17:09.29 |
| Robin_Watts | mvrhel_laptop: Yes. I've been trying to get it there all day. | 17:10.14 |
| mvrhel_laptop | :) | 17:10.30 |
| Robin_Watts | It's in doing the final review that I spotted this stuff. | 17:10.31 |
| mvrhel_laptop | ok. good deal | 17:10.45 |
| Robin_Watts | (my final review, we need to get Tor to look it over) | 17:10.46 |
| mvrhel_laptop | ah! found my problem. Had the version number wrong. They changed the profile description tag in 4.2 and I had the vers set to that | 17:15.26 |
| | but I was working off of vers 2.2 as it is simplier | 17:15.47 |
| | ok. should have this out is a bit | 17:16.00 |
| | need to check to make sure it works color wise still..... | 17:16.10 |
| Robin_Watts | mvrhel_laptop: Another question, sorry... | 17:17.04 |
| | load_icc_based.... | 17:17.09 |
| mvrhel_laptop | ok | 17:17.27 |
| Robin_Watts | we read n, we try to get cs, and that throws an exception.... | 17:17.29 |
| | and we then swallow that exception and return NULL. | 17:17.46 |
| | Is that right? | 17:17.55 |
| | pdf_load_colorspace_imp cannot cope with cs == NULL | 17:18.58 |
| mvrhel_laptop | yes. I think in the catch we should return one of the default spaces? A problem though is that if the default color space is the one that is getting thrown we are in hip deep | 17:20.25 |
| | Robin_Watts: oh | 17:20.52 |
| | Also there is this | 17:20.57 |
| | oh never mind | 17:21.24 |
| Robin_Watts | pdf_load_output_intent probably needs to cope with the output intent failing to read, I would imagine. | 17:21.41 |
| mvrhel_laptop | I was thinking of another case where the output intent claimed to be 4 component but had a 3 compoenent profile. That caused a diffierent problem | 17:21.59 |
| | yes. A failure on that and it should just pretend there was no output intent | 17:22.35 |
| | Any internal profile that fails, should use the alternate | 17:22.59 |
| | There are several cases to consider here. The one that worried me (and it would be from our own fault) was if someone packed a bad profile into the resource | 17:23.41 |
| | as a default | 17:23.48 |
| | do you see what I am saying? | 17:24.10 |
| | perhaps it is a non-issue | 17:24.25 |
| Robin_Watts | I see enough that I feel I ought to let you handle this ;) | 17:24.33 |
| mvrhel_laptop | ok. why don't you make a cut at fixing this, and then I will hack one of our defaults and see what explodes. | 17:25.36 |
| | during startup | 17:25.40 |
| | and make sure I can catch that | 17:25.53 |
| | or I can give it all a try | 17:26.09 |
| | and you can keep reviewing | 17:26.24 |
| Robin_Watts | I've shoved it onto my list and I'll push through towards the end. | 17:26.54 |
| mvrhel_laptop | ok | 17:27.01 |
| | Robin_Watts: ok. icc-color is working. I am going to push this to my repos on the robin-lcms | 17:30.26 |
| Robin_Watts | cool. I'll cherry pick that in a mo. | 17:31.23 |
| | mvrhel_laptop: You made a change in pdf_load_page... | 17:33.41 |
| mvrhel_laptop | Robin_Watts: ok pushed | 17:33.54 |
| Robin_Watts | you moved the loading of the page resources to be earlier... I can't see why. | 17:33.57 |
| mvrhel_laptop | Robin_Watts: when? | 17:33.58 |
| Robin_Watts | Dunno offhand, I'm looking at the squashed diffs :) | 17:34.15 |
| mvrhel_laptop | oh | 17:34.17 |
| | that was because we used to need it to get the default cs | 17:34.31 |
| | but tor pulled that out to happen at run_contents | 17:34.40 |
| | so now you can move it back down to the trans code | 17:34.49 |
| Robin_Watts | Ah, ok. Will do. ta. | 17:34.56 |
| mvrhel_laptop | I think that was where it was before | 17:34.58 |
| | Robin_Watts: let me know when you grab my commit, I will update to your branch and then work off of that | 17:36.05 |
| Robin_Watts | mvrhel_laptop: OK, lcms branch updated. | 17:39.37 |
| | Your commit is on there, just not at the top. | 17:39.46 |
| | The top 2 commits will go in not as part of the squashed branch. | 17:40.04 |
| | actually.... I just rebased it all on golden/master | 17:41.05 |
| | try again now maybe. | 17:41.16 |
| mvrhel_laptop | ok cool | 17:41.49 |
| Robin_Watts | mvrhel_laptop: In mutool you'd increased the number of colorspaces for each output format from 6 to 8, but now we don't need that ? | 17:42.17 |
| | I'll put it back. | 17:42.31 |
| mvrhel_laptop | right | 17:42.39 |
| Robin_Watts | So, we don't cope with iccgrayalpha etc anymore? I can remove those from the usage message? | 17:46.22 |
| mvrhel_laptop | right. | 17:47.41 |
| | We do need to think about how we might want to have a user specify their target ICC profile | 17:48.02 |
| | that is the device profile | 17:48.10 |
| Robin_Watts | A new (set of) command line flag(s) I guess? | 17:48.52 |
| mvrhel_laptop | yes | 17:48.56 |
| Robin_Watts | And for the purposes of review, there is an lcms_squashed branch which is more or less what will go in. | 17:52.19 |
| | I need to fix the other stuff on the list, and then make sure it's labelled with you as the author ;) | 17:52.45 |
| mvrhel_laptop | Robin_Watts: no problem. So one other commit on robin_lcms | 17:53.44 |
| | to make sure we go through the proofing profile when needed. I may need to revisit that | 17:54.03 |
| | I need to take a break now. I have to eat lunch and then take Ethan to UW to get his id | 17:54.29 |
| | and make sure he can find his class room | 17:54.44 |
| Robin_Watts | mvrhel_laptop: I don't follow the commit message? | 17:54.52 |
| | mvrhel_laptop: Sure, no problem. | 17:55.00 |
| mvrhel_laptop | it says to always map through the proof profile if it exists | 17:55.21 |
| Robin_Watts | Yeah, I get it, sorry. | 17:55.32 |
| | ttyl (or have a good weekend if not!) | 17:55.47 |
| mvrhel_laptop | no problem I should have written a bit more probably | 17:55.48 |
| | yes. thanks you too | 17:55.53 |
| avih | Robin_Watts: btw, i saw you're dealing with lcms and icc profiles, fwiw, there's a guy which is _very_ knowledgeable on this subject including on hdr and many other mathematical aspects of video processing at #mpv (nick: hanna). he might be able to help with issues you're having | 18:16.25 |
| | (if you have use for help, that is) | 18:16.52 |
| Robin_Watts | avih: mvrhel is a professional color scientist - has written text books on the subject etc. | 18:17.40 |
| | So I leave the hard stuff to him :) | 18:17.47 |
| avih | gotcha :) | 18:17.55 |
| Robin_Watts | but thanks. | 18:17.56 |
| | Forward 1 day (to 2017/06/17)>>> | |
Log of #mupdf at irc.freenode.net.