| | <<<Back 1 day (to 2018/10/10) | 20181011 |
| sebras | tor8: ping? | 11:01.31 |
| | tor8: wouldn't "gl: Fix launching mupdf-gl to the page number passed on the command line." be ble to remove the currentpage assignment and fz_clampi() at the end of load_document(), it seems to be checked by jump_to_page()... | 11:02.29 |
| | tor8: in "Fix FZ_ENABLE_SPOT_RENDERING config.h ifdeffery." should the comment above /* #define FZ_ENABLE_SPOT_RENDERING 1 */ explain what the default value is? the other comments seem to do this. | 11:04.52 |
| | the same goes for the commit with FZ_ENABLE_ICC. | 11:05.28 |
| | tor8: why remove the CSS classes in "Don't rely on CSS classes in HTML5 text output."? that is missing from the commit message. | 11:06.49 |
| | tor8: "Allow SVG output to keep ID numbers unique across pages." LGTM | 11:08.13 |
| | tor8: Add fz_write_pixmap_as_data_uri helper function."Add fz_write_pixmap_as_data_uri helper function" looks reasonable. | 11:09.48 |
| tor8 | sebras: yes, the #ifdeffery should probably be better documented | 11:18.47 |
| | sebras: I ran into an issue with the NO_ICC case where it renders bogus colors, which looks like a premultiplied/non-premultiplied error | 11:19.24 |
| | maybe we should try a build with NO_ICC and look at the bmpcmp for obvious bad cases that aren't just down to missing color management | 11:20.01 |
| | sebras: https://ghostscript.com/~tor/Law_of_Root_(5_September_2018).pdf front page, with make XCFLAGS=-DFZ_ENABLE_ICC=0 on the tor/wasm branch (or -DNO_ICC=1 on master) | 11:22.03 |
| sebras | tests. | 11:30.04 |
| | tor8: ehm. yes, that definitely looks bogus. | 11:33.54 |
| | tor8: the bad rendering seems to be related to the /Group entry in the page dict. | 11:50.13 |
| | without it it renders fine even without ICC. | 11:50.55 |
| tor8 | sebras: is it the Multiply BM group causing trouble? | 11:52.11 |
| sebras | possibly. | 11:52.46 |
| tor8 | or is it all the Isolated groups? | 11:52.47 |
| sebras | don't know ymore yet. | 11:52.54 |
| tor8 | doesn't seem like it, if I nuke the /BM Multiply it still renders crap | 11:54.14 |
| sebras | tor8: the page object has a /Group with the contents: | 11:55.05 |
| | << | 11:55.07 |
| | /CS /DeviceCMYK | 11:55.07 |
| | /S /Transparency | 11:55.07 |
| | /Type /Group | 11:55.07 |
| | >> | 11:55.09 |
| | and of course it is /CS at the beginning that triggers this. | 11:55.21 |
| tor8 | nuking object 73's /Group entry makes the rendering look OK | 11:58.12 |
| sebras | yes. I did clean the file however, so my objects don't match up. | 11:58.48 |
| | is object 73 the page object? | 11:58.58 |
| tor8 | sebras: I just cleaned with -gd | 11:59.00 |
| | (no renumbering) | 11:59.04 |
| sebras | ah. | 11:59.07 |
| tor8 | yes, 73 is the page object | 11:59.27 |
| | 448 is the group dictionary | 11:59.51 |
| sebras | tor8: I agree after having recleaned. | 11:59.55 |
| tor8 | if I change it to DeviceRGB it renders okay too | 11:59.57 |
| sebras | yes. | 12:00.03 |
| | we add a gropu in pdf_run_page_contents_with_usage() | 12:00.29 |
| | if there's transparency. | 12:00.35 |
| | tor8: it is possible to see the issue withonly object 79 in the /Contents arrya in object 73 | 12:03.06 |
| tor8 | sebras: and if you remove object 103's group, the frame looks okay | 12:06.16 |
| | I'm guessing blending two separate transparency groups in CMYK goes wrong with NO_ICC | 12:06.46 |
| sebras | tor8: perhaps the issue is somewhere in template_span_4_general() | 12:10.51 |
| | as it gets called as part of fz_draw_end_group | 12:11.06 |
| tor8 | sebras: wouldn't that be the same with ICC as without? | 12:13.03 |
| | maybe time to play with robin's DUMP_GROUP_BLENDS :) | 12:13.28 |
| sebras | perhaps not. the colorspace used wouldn't be the same..? | 12:13.44 |
| tor8 | it would still be CMYK no? | 12:14.04 |
| | they're all using DeviceCMYK | 12:14.10 |
| | so 4-component | 12:14.16 |
| sebras | I think so. | 12:14.30 |
| tor8 | Group end: blending 11(PSD)(0x3c34630) onto 12(0x3c7e560) (isolated) to get 13(0x3c7e560) is where I think it may be going wrong | 12:17.53 |
| | it's a bit hard, imagemagick's 'display' only shows the PSDs as RGB | 12:18.40 |
| | and gimp refuses to open CMYK photoshop files | 12:19.08 |
| sebras | yes. | 12:20.16 |
| | I'm guessing this is why I have a partial psd decoder on my WIP-branch... | 12:20.53 |
| tor8 | because dump13.png is definitely wrong | 12:20.56 |
| sebras | agreed. | 12:21.32 |
| tor8 | but dump11.psd looks reasonable if I ask imagemagick to convert it to png | 12:21.39 |
| sebras | gm convert: No decode delegate for this image format (dump10.psd). | 12:24.12 |
| | tor8: hm.. I was unsuccessful in converting the output to pkm by altering the blend dumping. | 12:26.57 |
| | tor8: https://pastebin.com/raw/eP809fVS gives me dump??.pam that mupdf itself cna prase. | 12:32.14 |
| | parse. | 12:32.17 |
| | I see no problems until dump13.png | 12:32.46 |
| | the pam files all look fine! | 12:32.59 |
| tor8 | sebras: right, so we're closer to knowing which operation is botched at least. | 12:34.35 |
| sebras | yes | 12:34.43 |
| | so likely in paint_span_3_da_sa(). | 12:39.49 |
| | it's fz_convert_pixmap() that goes wrong when doing the last step | 12:50.13 |
| | it gets called if (state[0].dest->colorspace != state[1].dest->colorspace) | 12:50.25 |
| | state1 being cmyk, state 0 being rgb. | 12:51.08 |
| | tor8: I added an extra fz_dump_blend() after the convert and the image looks incorrect at this point. | 12:51.38 |
| tor8 | sebras: so it's transforming from cmyk to rgb and failing to a correctly converted pixmap out? | 12:58.39 |
| sebras | yes, that's what I'm seeing. | 12:58.52 |
| tor8 | fast_cmyk_to_rgb then? | 12:59.05 |
| sebras | that makes me suspecious of cached_cmyk_conv(), but I'm not entirely sure yet. | 12:59.13 |
| tor8 | I suspect it doesn't know about alpha channels and premultiplied alpha, having only been tested on fully opaque pixmaps | 13:00.21 |
| sebras | tor8: dump13.png on casper is the cmyk pixmap converted to rgb. | 13:01.44 |
| | tor8: your assumption sounds reasonable. | 13:02.09 |
| tor8 | yeah, looks like it could be | 13:02.39 |
| sebras | tor8: wouldn't we then be able as a debug step to unmultiply it and then do the convertion only to multiply it again after..? | 13:03.33 |
| | tor8: resurrecting fz_unmultiply_pixmap() and doing exactly that gives a better result at least. | 13:07.03 |
| tor8 | the ICC code uses fz_lcms_unmultiply_row | 13:07.57 |
| | yes. if I unmultiply the source values before calling cached_cmyk_conv, and multiply the rgb values after, things seem to come out ok | 13:09.47 |
| | except I then get stupid assertions :( | 13:10.08 |
| sebras | I don't. :) | 13:10.17 |
| tor8 | mupdf: source/fitz/draw-device.c:2426: void fz_draw_end_group(fz_context *, fz_device *): Assertion `state[0].group_alpha == NULL || state[0].group_alpha != state[1].group_alpha' failed. | 13:10.19 |
| sebras | then you forgot to drop/reference something. | 13:10.33 |
| | or perhaps something fz_threw()..? | 13:10.50 |
| | tor8: with debugging.dif on casper I get resonable output. | 13:11.35 |
| tor8 | I'm manipulating the cmyk values inside fast_cmyk_to_rgb... | 13:11.59 |
| sebras | tor8: good luck! I don't understand any of that. :) | 13:15.39 |
| tor8 | I get error: compression error -2 | 13:20.07 |
| sebras | tor8: oops. | 13:20.36 |
| tor8 | yeah. the DUMP_GROUP_BLENDS crashes when I premultiply the alpha coming out | 13:21.08 |
| sebras | strange that this doesn't happen for me. | 13:21.27 |
| tor8 | yeah.... this color conversion code in source/fitz/colorspace.c looks like it can't handle pixmaps with alpha channels AT ALL | 13:23.42 |
| | it just converts the premultiplied values as is | 13:24.07 |
| | that's not going to work with any kind of non-linear colorspace | 13:24.23 |
| sebras | right. | 13:24.32 |
| tor8 | I blame Robin_Watts and mvrhel.... | 13:24.36 |
| | and now it doesn't help that we've got dozens upon dozens of manually unrolled hand-optimized variants to fix.... | 13:25.07 |
| sebras | tor8: at least we now know where the problem is. | 13:26.06 |
| tor8 | and we probably shouldn't be changing the input pixmap to unmultiply, call the conversion, then premultiply it back | 13:26.12 |
| sebras | that's better than two hours ago. | 13:26.18 |
| tor8 | because it could be using the same source pixmap in a different thread | 13:26.24 |
| sebras | tor8: why would that matter.? | 13:26.41 |
| tor8 | if the second thread also needs the source pixmap for a different blend operation ... *boom* | 13:26.56 |
| sebras | oh, you mean using the samples, not just refering to it, but using it _right now_..? | 13:27.11 |
| tor8 | the source pixmap values should *not* be changed | 13:27.14 |
| | even temporarily | 13:27.18 |
| | yes. | 13:27.26 |
| sebras | we can clone unmultiply and convert and premultiply... | 13:27.35 |
| tor8 | though it does seem like lcms does indeed unmultiply a row, transform it, and premultiply it back | 13:28.03 |
| | I hope that is guarded by a lock....... | 13:28.13 |
| | it doesn't look like it though | 13:28.24 |
| | Robin_Watts: do you know anything about that? | 13:28.32 |
| Robin_Watts | tor8: just reading... | 13:28.39 |
| tor8 | fz_lcms_transform_pixmap temporarily mutates the input pixmap's samples without locking | 13:29.07 |
| | could that cause us problems when rendering say a CMYK image in multiple threads at the same time? | 13:29.28 |
| Robin_Watts | It could. And it's bad. And it shouldn't. | 13:30.05 |
| sebras | tor8: that's only one of the issues. the other is in fast_cmyk_to_rgb(). | 13:31.25 |
| Robin_Watts | And although it is Michael's code originally, I updated it for spots, so I have to bear some of the responsibility. | 13:31.35 |
| tor8 | sebras: and in fast_cmyk_to_bgr | 13:32.15 |
| | and all the other fast_*_to_* transforms | 13:32.22 |
| sebras | yes. | 13:32.26 |
| Robin_Watts | I wonder if cmsDoTransform can't do the unpremultiplication itself... | 13:32.38 |
| tor8 | used to be we only converted opaque pixmaps (since the images in PDF never have an inline alpha channel) | 13:32.54 |
| | but when we start doing rendering in the transparency group colorspaces and need to convert between spaces for blending there, we run afoul of previous assumptions | 13:33.42 |
| | Robin_Watts: are spot colors premultiplied? | 13:34.05 |
| Robin_Watts | Hold on... | 13:34.06 |
| | No, none of us can read C. | 13:34.43 |
| | fx_lcms_unmultiply_row copies from the source into a new buffer. | 13:35.01 |
| | that is allocated in fz_lcms_transform_pixmap. | 13:35.15 |
| | So we're golden. | 13:35.22 |
| | Yes, spot colors should eb premultiplied, I believe. | 13:35.46 |
| | So where was the next issue? | 13:36.01 |
| sebras | Robin_Watts: in fast_cmyk_to_rgb that doesn't handle premultiplied pixmaps. | 13:38.02 |
| | Robin_Watts: and as tor8 said above, thats likely true for all fast_*_to_*. | 13:38.22 |
| Robin_Watts | /* Spots must match, and we can never drop alpha (but we can invent it) */ | 13:38.28 |
| | I see what you mean, yes. | 13:39.13 |
| sebras | we have boyth sa and da. | 13:39.15 |
| | ss ought to be == ds | 13:39.32 |
| | and copy_spots is unknown to me, but likely set..? | 13:39.47 |
| tor8 | Robin_Watts: fab. so maybe we should make all the non-ICC color transforms use the same unpremultiply_row | 13:40.37 |
| Robin_Watts | tor8: If that works, sure. | 13:41.04 |
| | Anything that works is an improvement here. | 13:41.14 |
| tor8 | would be better than adding the same stuff to all the individual fast_*_to_* cases | 13:41.21 |
| Robin_Watts | If it turns out to be a hotspot we can optimise it then. | 13:41.22 |
| | unless it's obviously a hotspot, yes. | 13:41.40 |
| | Forward 1 day (to 2018/10/12)>>> | |
Log of #mupdf at irc.freenode.net.