Log of #mupdf at irc.freenode.net.

 <<<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." LGTM11: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 documented11: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 error11: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 management11: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 crap11:54.14 
sebras tor8: the page object has a /Group with the contents:11:55.05 
  /CS /DeviceCMYK11:55.07 
  /S /Transparency11:55.07 
  /Type /Group11:55.07 
  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 OK11: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 -gd11:59.00 
  (no renumbering)11:59.04 
sebras ah.11:59.07 
tor8 yes, 73 is the page object11:59.27 
  448 is the group dictionary11:59.51 
sebras tor8: I agree after having recleaned.11:59.55 
tor8 if I change it to DeviceRGB it renders okay too11: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 7312:03.06 
tor8 sebras: and if you remove object 103's group, the frame looks okay12:06.16 
  I'm guessing blending two separate transparency groups in CMYK goes wrong with NO_ICC12: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_group12: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 DeviceCMYK12:14.10 
  so 4-component12: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 wrong12:17.53 
  it's a bit hard, imagemagick's 'display' only shows the PSDs as RGB12:18.40 
  and gimp refuses to open CMYK photoshop files12: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 wrong12:20.56 
sebras agreed.12:21.32 
tor8 but dump11.psd looks reasonable if I ask imagemagick to convert it to png12: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 
  I see no problems until dump13.png12: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 yes12: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 step12: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 pixmaps13: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 be13: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_row13: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 ok13: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 -213:20.07 
sebras tor8: oops.13:20.36 
tor8 yeah. the DUMP_GROUP_BLENDS crashes when I premultiply the alpha coming out13: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 ALL13:23.42 
  it just converts the premultiplied values as is13:24.07 
  that's not going to work with any kind of non-linear colorspace13: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 back13: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 thread13: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 changed13:27.14 
  even temporarily13:27.18 
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 back13:28.03 
  I hope that is guarded by a lock.......13:28.13 
  it doesn't look like it though13: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 locking13: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_bgr13:32.15 
  and all the other fast_*_to_* transforms13: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 assumptions13: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 == ds13: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_row13: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_* cases13: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)>>> 
ghostscript.com #ghostscript