Log of #mupdf at irc.freenode.net.

Search:
 <<<Back 1 day (to 2017/06/26)20170627 
dcaro ray_laptop: i'm not confused04:43.16 
  mupdf runs on windows which is closed source...04:43.45 
RobinWattsLenovo dcaro: You CAN build a shared lib MuPDF -we do for android, for example.07:44.10 
  But we don't support that for unix, as the ABI changes slightly between releases.07:44.30 
dcaro Robin_Watts: i guess that i have to change the build system08:40.08 
  i see that you use Makefile08:40.18 
  i don't care honestly08:40.26 
Robin_Watts dcaro: You'd need to tweak the build slightly, yes.08:40.39 
dcaro and anyway, it's better to use what the OS provides08:41.11 
Robin_Watts The OS doesn't "provide" any of this :)08:43.38 
dcaro the packager(s) if you want :)08:44.32 
sebras tor8: ok, so I prepared a few commits on sebras/master.12:40.04 
  tor8: they cluster without differences.12:41.10 
tor8 sebras: first two LGTM12:46.49 
  have you changed the 'encstm' one since I last looked at it?12:48.06 
  'Make the entire cached color converter defined in case of error.' also LGTM12:48.27 
sebras I have.12:49.09 
  I merged the two encstrm-patches into a single one.12:49.23 
tor8 okay, so that one LGTM too.12:49.32 
sebras and attempted to clarify the commit message.12:49.32 
tor8 last one, the writer device one, I'm looking at now12:49.42 
sebras yeah, that one is likely the most controversial.12:50.02 
tor8 yes...12:50.37 
  how about we just plain call fz_drop_device in fz_drop_document_writer if wri->dev is not NULL?12:52.09 
  that means we never called end_page, and should clean up safely12:53.22 
sebras tor8: well, the device is created in each document type's begin_page and close and dropped in end_page under normal circumstances.12:53.46 
  I guess this means that the document type's should all _fully_ handle their created devices..?12:54.13 
tor8 yes, we do that so that some devices can keep one device around for multiple pages12:54.21 
sebras tor8: I seem to recall that robin was of this opinion..?12:54.23 
  tor8: I think it would work equally well with a fz_drop_device() in fz_drop_document_writer regardless of wri->dev is null or not.12:55.29 
tor8 I don't know (or remember) anything Robin said about this12:55.38 
Robin_Watts either.12:55.48 
  but that doesn't prove I didn't express an opinion.12:55.58 
tor8 the fz_documen_writer wrappers handle remembering the device to pass it back in to the end_page callback12:56.38 
  so that the caller doesn't need to do that busywork12:56.49 
  so it wouldn't strike me as all that weird to let the fz_document_writer wrapper also clean up any leftover devices12:57.15 
  my other option would be to add an abort_page callback to do the cleanup. passing it to the drop_writer call works too I guess, but feels a bit weird to me. at least it does today :)12:57.57 
sebras https://ghostscript.com/mupdfirclogs/2017/06/16.html search for "Given that the document_writer is the thing"12:58.35 
Robin_Watts tor8: devices are reference counted.12:58.37 
  So whoever drops it last will destroy it.12:59.02 
sebras tor8: I like that you added "today". :-D12:59.20 
tor8 I'm fickle and you know it.12:59.49 
  dropping the wri->dev in fz_drop_document_writer is my vote.13:00.14 
sebras tor8: as long as robin is also ok with that I'll change the patch..?13:00.35 
tor8 it's the simplest approach and doesn't violate any contracts. ideally we'd hope that whoever calls begin_page makes sure to call end_page before dropping the writer13:00.57 
Robin_Watts If the writer "keeps" the device", then it should "drop" it.13:01.31 
sebras Robin_Watts: at what level? the generic document writer level or in the specialized writer level?13:01.57 
Robin_Watts Possibly we shouldn't pass the device out of the writer interface at all.13:02.13 
tor8 Robin_Watts: fz_begin_page returns a "borrowed" device that the caller shouldn't need to bother reference counting13:02.19 
Robin_Watts We should just have an fz_device *fz_writer_device(ctx, writer) call that gives access to the internal one.13:02.39 
  tor8: right, same effect.13:02.51 
tor8 Robin_Watts: without the device, how should we expose the device interface for writing to a page?13:02.52 
Robin_Watts having fz_writer_device makes it clearer (to me) that it's a borrowed reference that only lives as long as the writer does.13:03.21 
tor8 Robin_Watts: well, that's what fz_begin_page is supposed to do, as well as set up whatever needs setting up for the mediaboxes etc :)13:04.23 
  and that may return a new device, or a reused device, caller shouldn't need to know or care13:04.54 
sebras tor8: doesn't this mean that robin is arguing that begin_page shouldn't return a device, but that fz_begin_page() calls the specialized begin_page and then returns the result of fz_writer_device()?13:05.04 
Robin_Watts yeah. I think I'm happy either way.13:05.05 
tor8 it just needs to play things to the device, and then end_page when it's done13:05.13 
  sebras patch covers what happens when you drop the writer but forgot (or didn't bother with, due to aborting to errors) to end the page13:05.56 
  and I think that can be easier done with the one-liner in fz_drop_document_writer than passing it to the subclass callbacks13:06.25 
Robin_Watts tor8: I will defer to you on this. I've got my head up CMYK stuff.13:06.48 
  swapping to the laptop downstairs for an hour...13:06.56 
tor8 but it might be worth adding a comment somewhere explaining that13:07.04 
  Robin_Watts: I've been thinking a bit (but not too hard) about whether blending CMYK with alpha really needs to be inverted13:07.28 
sebras tor8: is not forgetting to call end_page a programming error rather than a user error..?13:07.29 
tor8 RobinWattsLenovo: (in case you already ran downstairs...)13:07.42 
sebras tor8: i.e. is that something we care about?13:07.53 
RobinWattsLenovo tor8: It's not just cmyk blending that needs us to invert.13:08.22 
sebras tor8: also, is it always safe to call end_page in case of errors?13:08.38 
RobinWattsLenovo Currently every pixmap has n channels.13:08.52 
  That's pix->colorspace->n + pix->alpha channels.13:09.10 
tor8 sebras: actually, looking at the code, calling end_page in case of errors shouldn't really be necessary (or strictly correct) since that calls fz_close_device which is what you're supposed to call when everything is A-ok13:09.20 
RobinWattsLenovo In order to proper cope with spots, we're going to need to change that to be pix->colorspace->n + pix->s + pix->alpha channels.13:09.34 
  where pix->s is the number of spots.13:09.46 
  spots are traditionally subtractive too.13:10.00 
tor8 so spot colors are a property of the pixmap not the current colorspace?13:10.06 
RobinWattsLenovo tor8: I think that's where mvrhel and I landed, yes.13:10.22 
tor8 RobinWattsLenovo: I was wondering whether the blending math (and premultiplied alpha) would come out correct if we didn't invert them13:10.43 
RobinWattsLenovo cos the alternative is that we have to have lots of 'rgb + 1 spot', 'rgb + 2 spot' etc colorspaces kicking around.13:10.48 
tor8 yeah, that seems a bit wrong too13:10.58 
  but how to communicate such colors through the device interface?13:11.11 
  oh, in PDF a spot color is just Spot-colorspace + one component (or DeviceN + spots)13:12.01 
  so in the device interface we never see the RGB+spots13:12.18 
  so yes, I'm in agreement that it belongs in the pixmap not the colorspace13:12.29 
RobinWattsLenovo cool.13:12.32 
  You got there faster than me, but your logic seems sound.13:12.44 
tor8 the draw device needs to look at the incoming colorspace, see that it's a spot and figure out whether to map that to channel N in the pixmap or invoke the tint transform to go to the "regular" RGB/CMYK/Gray colorspace13:13.24 
sebras tor8: final LGTM on sebras/master?13:13.54 
tor8 sebras: LGTM.13:14.28 
sebras tor8: ty.13:14.31 
tor8 RobinWattsLenovo: so the fz_pixmap would have an array of spot color names to go along with the pix->s count13:15.18 
RobinWattsLenovo tor8: Would it?13:15.34 
tor8 for the draw device to be able to map a spot color to the corresponding channel13:15.51 
RobinWattsLenovo I was imagining that the draw device would keep a list of the spot nams.13:16.05 
tor8 spot colors have names like Pantone52 or Gloss or something like that13:16.13 
RobinWattsLenovo cos the spot names would be the same in every pixmap.13:16.25 
tor8 I think it would be helpful in the fz_pixmap for saving to image files13:16.32 
RobinWattsLenovo cos the spot names would be the same in every pixmap used by the draw device.13:16.36 
tor8 but they could just as well live in the draw device if we don't need them externally13:16.53 
sebras tor8: I think you might have missed me mentioning wanting to remove the fz_warn() in do_hash_insert(): https://ghostscript.com/mupdfirclogs/2017/06/24.html13:16.56 
RobinWattsLenovo tor8: In that case we should have a reference counted fz_spot_list object.13:17.20 
tor8 sebras: I don't remember much about the hash table after robin reworked it to his liking.13:17.21 
RobinWattsLenovo and the fz_pixmap should have an optional reference to that.13:17.35 
sebras tor8: also, if you need to hash out these separation things (which I think you are taling about?) first, then I'll stop bothering you for a bit.13:17.38 
tor8 Robin_Watts: yes, some sort of spot color descriptor that could be used as the input to the draw device/pixmap/pixmap_document_writer constructors13:18.01 
RobinWattsLenovo yeah.13:18.10 
sebras tor8: (because I would be too distracted myself to work on two things simultaneously)13:18.16 
tor8 sebras: I can't answer with any certainty13:23.10 
  IIRC we only called the old fz_hash_insert_with_pos in one place, and that place ignored the &pos result13:24.41 
  the presence of the unused &pos would have masked the warnings13:24.59 
  sebras: try - fz_warn(ctx, "assert: overwrite hash slot");13:26.57 
  + if (val != ents[pos].val)13:26.57 
  + fz_warn(ctx, "assert: overwrite hash slot with different value!");13:26.57 
  + else13:26.57 
  + fz_warn(ctx, "assert: overwrite hash slot with same value");13:26.57 
sebras tor8: I already get this assert error while rendering pdfref17.pdf page 1143 nowadays though.13:30.42 
  tor8: and that doesn't seem right to me.13:30.48 
  tor8: I haven't looked into why this happens yet because the previous comment stated this case was actually legal (until 9322cd)13:31.59 
tor8 sebras: the comment is confusing, I don't understand it, you should ask Robin about that13:32.14 
  the backtrace in the code points to fz_draw_end_tile where it's looking at cached tile renderings13:32.39 
  that coud *could* be rewritten to check the store for an existing tile before mallocing one13:33.44 
RobinWattsLenovo sebras: Yeah, the assert going off in single threaded builds worries me.13:33.47 
  tor8: Urm, race condition?13:33.55 
tor8 RobinWattsLenovo: /* We already have a tile. This will either have been13:34.01 
  * produced by a racing thread, or there is already13:34.01 
  * an entry for this one in the store. */13:34.01 
RobinWattsLenovo It already *does* check the store before creating one :)13:34.15 
  Check the store, see if there is an entry there that does what we want. If not, we go create an entry, and store it. When we store it we have to check because another thread might have run through the same process as we just did, and also decided to make one.13:35.12 
tor8 RobinWattsLenovo: it only checks the store in begin_tile13:35.14 
RobinWattsLenovo It's whichever one finishes first that gets stored.13:35.24 
  oh, right, this is the tile stuff, sorry.13:35.34 
tor8 and stores a new tile in end_tile (which is understandable, we don't want to store it before it's finished rendering the tile)13:36.03 
  but then the second time around, why don't we find it and try to recreate the tile? that seems like something's gone wrong.13:36.22 
RobinWattsLenovo tor8: cos it's been evicted?13:36.36 
  I need to recheck the code. Give me a mo.13:37.04 
  OK, so in fz_draw_begin_tile we check to see if we have a tile that's usable.13:37.44 
tor8 tile_key tk is not initialized to zero, and the 'refs' reference count is not used in the fz_find_item?13:37.50 
RobinWattsLenovo If we do, then we return with 1 to say "use the cached tile".13:38.04 
  If we don't then we return with 0, which basically means "recreate the tile".13:38.20 
  (IIRC)13:38.26 
sebras tor8: fz_make_hash_tile_key() doesn't appear to care about refs..?13:39.11 
RobinWattsLenovo So if we have 2 threads running, they can both hit the begin_tile, and both decide that they need to create the cached tile.13:39.12 
  sebras: open a bug, and throw it at me, and I when I did myself out of the current set of holes I'll have a look.13:40.03 
  s/did/dig/13:40.10 
sebras RobinWattsLenovo: Ok.13:40.28 
tor8 Robin_Watts: it's being called with id==013:41.03 
  so the cache is not used in fz_draw_begin_tile13:41.24 
  and the end_tile call keeps reinserting the rendered tile at the same slot in the cache13:42.16 
  end_tile key=0 0.400009 0 0 -0.40000913:42.51 
  warning: assert: overwrite hash slot with different value!13:42.51 
  end_tile key=0 0.400009 0 0 -0.40000913:42.51 
  etc...13:42.51 
  and no correspending begin_tile call (since the id=0)13:43.05 
sebras tor8: seems counterintuitive to have id as a separate thing from the hash used in the store..?13:45.15 
  tor8: if we have the store, why have a separate mechanism (id) to check if items are present or not..?13:46.24 
  though I might be missing something important.13:47.07 
  tor8: oh! id and ctm _is_ the input data for the hash, I guess that means that just ctm is not enough to make it unique.13:49.07 
RobinWattsLenovo Possibly if id is 0 we should never encache it.13:52.17 
  (If we never check for something with id 0 being in the cache, why ever put it *into* the cache?)13:52.43 
sebras so fz_begin_tile() always hardcodes id to 0, that means that only tiles from list device, the test device and jni/js calls to fz_begin_tile_id() are the only ways to cache them.13:56.59 
  where as fz_begin_tile() is called when handling pdf patterns and xps tiling brushes. seems to me like those would be good cases to cache..?13:58.45 
RobinWattsLenovo sebras: This was an example of where we changed the API.14:01.28 
  It used to be that people called fz_begin_tile, and I wanted to add an 'id' to it.14:01.48 
  so rather than doing the usual thing and just fixing the damn API, we added 'fz_begin_tile_id' and implemented stuff in terms of it.14:02.22 
  The idea is that everything should move over to calling 'fz_begin_tile_id' but that never happened.14:02.42 
  Any of our interpreters that *don't* call fz_begin_tile_id are bad.14:03.20 
sebras RobinWattsLenovo: why is the id needed, i.e. why is the ctm not enought?14:03.24 
  agh! there's no trailing t in that word. :-P14:03.35 
RobinWattsLenovo sebras: Who is to say I can't have a PDF file that overlays 2 tiled backgrounds in the same place?14:03.55 
tor8 so we need to fix two things here:14:04.12 
  1) don't encache stuff with id==014:04.19 
  2) our interpreters to pass an ID14:04.25 
RobinWattsLenovo tor8: Spot on.14:04.31 
tor8 RobinWattsLenovo: ouch. now I got a bad error :(14:08.09 
  mupdf: source/fitz/draw-paint.c:1951: void fz_paint_pixmap_with_mask(fz_pixmap *restrict, const fz_pixmap *restrict, const fz_pixmap *restrict): Assertion `dst->n == src->n' failed.14:08.13 
RobinWattsLenovo tor8: Open a bug, throw it at me :)14:09.32 
tor8 RobinWattsLenovo: first commit on tor/master with pdfref17.pdf page 114314:13.10 
sebras tor8: so if it is not cached we do not need to process the contents at all?14:16.12 
RobinWattsLenovo tor8: The point is I'm not going to get to it today.14:16.20 
  so I will forget the details.14:16.27 
tor8 sebras: the idea is that if it is cached, the call to begin_tile will be enough to also instantiate the tile14:17.10 
RobinWattsLenovo sebras: If fz_begin_tile returns 0, then the interpreter is expected to just run the tile contents repeatedly, once for each position.14:17.18 
tor8 RobinWattsLenovo: no, that's something else altogether14:17.50 
RobinWattsLenovo ok, yes, scratch that.14:17.59 
tor8 it's supposed to just skip feeding the contents of the tile, the begin_tile arguments says how to repeat the tile14:18.33 
  RobinWattsLenovo: okay, strangeness ensues14:19.30 
sebras tor8: 5d1e8bcc1 LGTM.14:30.33 
  tor8: I believe 8935ab6e7 forgets to remove FZ_MAX_SEPARATIONS from the header, but also it couldn't because gproof uses it..?14:31.40 
  gprf-doc.c14:31.49 
  there's a new set of commits on sebras/master14:59.16 
tor8 sebras: first 3 LGTM15:04.43 
  does zlib need deflateEnd to free memory?15:04.52 
  ah, yes, they do.15:05.31 
  sebras: all 4 LGTM the15:06.15 
Robin_Watts mvrhel_laptop: You here?18:26.40 
mvrhel_laptop Robin_Watts: yes. just about to head out for a bit though18:27.57 
Robin_Watts mvrhel_laptop: ah, ok.18:28.19 
mvrhel_laptop I had not done any work on mupdf today yet18:28.49 
Robin_Watts mvrhel_laptop: I've hit a problem that I'm having trouble explaining.18:29.06 
  and I was going to ask you about it.18:29.13 
  but if you're on your way out, don't let me stop you.18:29.24 
mvrhel_laptop ok18:29.24 
  no lets chat for a bit18:29.33 
  I can wait 10 minutes18:29.39 
Robin_Watts Thanks.18:29.43 
  So, I have stuff working going to ppmraw.18:29.52 
  (and presumably png etc too)18:29.58 
  haven't checked pgm, but it's additive, so I don't forsee a problem.18:30.11 
mvrhel_laptop This is the inverse CMYK18:30.12 
Robin_Watts Yeah.18:30.17 
  I'm now testing going to cmyk.18:30.26 
  (so -o out.pam -c cmyk)18:30.35 
  I've got one file here that I've simplified to the point where it chooses a separation color, and strokes a rectangle, and that rectangle is coming out yellow instead of black.18:31.16 
  The separation color is a function that maps from [0 0 0 0] to [1 1 1 1]18:31.36 
  The separation color is set to 1 in the file (so 0 internally, cos separations are subtractive)18:31.49 
mvrhel_laptop yellow seems a little odd18:31.55 
Robin_Watts For the function I invert the 0 to get 1, map it through to get [1 1 1 1] and then invert that to get back to [0 0 0 0]18:32.36 
  That then gets fed into an icc link going from a 4 colour subtractive space to another 4 colour subtractive space, and I get [.9 .9 .5 .9]18:33.21 
  (i.e. 0 0 .5 0)18:33.33 
  yellow.18:33.38 
mvrhel_laptop So you have a CMYK to CMYK mapping that is taking 0 0 0 0 and mapping it to .9 .9 .5 .9?18:34.32 
Robin_Watts yes, except bear in mind that it's got the 'inverted' formatter on both ends.18:34.59 
  so it's really mapping [1 1 1 1] to [.1 .1 .5 .1]18:35.11 
mvrhel_laptop ok18:35.25 
Robin_Watts acrobat displays this rectangle as black.18:36.26 
  I'm confused :)18:36.43 
mvrhel_laptop hmm. It might be worthwhile for me to take a look at this one when I get back18:36.48 
  what does gs do with it?18:37.20 
Robin_Watts gs says black.18:39.38 
mvrhel_laptop Robin_Watts: ok. I just committed my fix for the gs bug. I can take a look at this if you want when I get back18:39.41 
  Is your latest on your repos?18:39.48 
Robin_Watts mvrhel_laptop: I'll bash at it for another 20 minutes or so, and if I can't get anywhere will mail it all to you. I suspect I'm doing something really dumb.18:40.08 
  Thanks.18:40.11 
mvrhel_laptop Robin_Watts: no problem. I have been there before.....18:40.22 
Robin_Watts If I use 0 0 0 1 K it works fine.18:42.51 
  If I use /CS0 CS 1 SCN it gives me yellow.18:43.05 
  So it's something to do with having the separation in there.18:43.13 
  Ah 0 0 0 1 K is going through the 'is_identity' case.18:45.34 
  More strictly I should be using 1 1 1 1 K of course...18:48.44 
ray_laptop Robin_Watts: 0 0 0 1 K is a perfectly legal (and common) black. 1 1 1 1 K isn't often used18:55.12 
Robin_Watts Gah. Stupid typo.18:55.25 
  ray_laptop: yeah, nonetheless it is the value used in this file18:55.34 
  mvrhel_laptop: Woo Hoo!19:22.30 
  Looks much better now. *Many* progressions in the pam output.19:22.44 
  Latest stuff is on robin/master if you want to play with it. I'll carry on testing it tomorrow.19:23.20 
mvrhel_laptop Robin_Watts: oh the is identity will end up passing the colors through unmolested19:48.37 
Robin_Watts mvrhel_laptop: Yeah, I had a stupid typo that meant I was failing to tell the end of the pipeline to format back out to inverted.20:18.44 
  All fixed now.20:18.50 
  Looks much better.20:18.56 
mvrhel_laptop great!20:19.09 
  Robin_Watts: so is this committed on your repos now?20:40.21 
  oops sorry Robin_Watts reading log. I will grab it and check my blending stuff then21:39.05 
  hmmm colors look a little odd going out to a rgb device when blending space was cmyk. very dull22:07.14 
  Robin_Watts: so the color conversions are not correct even in the non-transparency case. Digging into problem now23:06.50 
Robin_Watts mvrhel_laptop: hi23:13.59 
  I've only clustered the ICC versions.23:14.07 
mvrhel_laptop Robin_Watts: What do you mean?23:14.21 
Robin_Watts I've not cluster tested the non-ICC pathways yet.23:14.40 
mvrhel_laptop I am running the ICC case23:14.48 
Robin_Watts yeah, I realised after I typed :)23:14.57 
mvrhel_laptop I am looking at a simple CMYK fill going to PNG device23:15.23 
  Did you not see any regressions in that case?23:15.37 
Robin_Watts I didn't spot any huge diffs, no.23:17.22 
  but I was running bmpcmp -t 64 or something.23:17.30 
mvrhel_laptop ok. let me do a quick sanity check to makes sure the head is not broken23:17.54 
Robin_Watts I was testing ppmraw, but that should be the same as png.23:18.01 
mvrhel_laptop yes23:18.07 
  RobinWattsLenovo: Robin_Watts: ok without your changes its the same dirty color. Doing a bit of digging on this for sec to see what is going on23:52.06 
  something funky is going on23:53.16 
 Forward 1 day (to 2017/06/28)>>> 
ghostscript.com #ghostscript
Search: