| | <<<Back 1 day (to 2017/07/03) | 20170704 |
| tor8 | avih: I'm still experimenting with various approaches. | 09:38.30 |
| Robin_Watts | tor8: various commits on robin/master. | 09:39.58 |
| tor8 | Robin_Watts: gah, "Avoid leaking an fz_stream for every unknown crypt handler." indicates we should fix the stream construction refcounting. but I think sebras is already working on that. | 09:52.12 |
| | Robin_Watts: all up to 'Avoid losing dev' LGTM | 09:52.21 |
| | -fz_var(cspace); | 09:53.35 |
| | +fz_var(cspace); /* cspace is only ever a borrowed reference here */ | 09:53.35 |
| | no need for fz_var at all in that function if you remove the fz_always fz_drop_colorspace | 09:53.54 |
| | in fz_new_image_from_buffer | 09:54.05 |
| | apart from that, "Remove is_static" looks reasonable to me | 09:56.19 |
| Robin_Watts | Ta. | 09:56.43 |
| | If sebras has a nicer fix for any of this, great. | 09:57.46 |
| tor8 | we talked about rejigging how the fz_open_filter_foo functions should behave | 09:58.08 |
| | but your fix LGTM now, and might possibly need reverting once that goes in | 09:58.21 |
| | since your fix at least matches the behaviour of what happens with a filter | 09:58.40 |
| Robin_Watts | OK, updated is_static commit. | 10:01.15 |
| | cluster -filter=ppmraw.72.0 is now managing never to leak except in the epub/html/xhtml cases, and 2 SVGs. | 10:01.50 |
| | The epub etcs are expected because of &*%$ing harfbuzz. | 10:02.09 |
| tor8 | Robin_Watts: nice, LGTM. | 10:02.23 |
| Robin_Watts | I'll look into the SVGs in a mo. | 10:02.42 |
| tor8 | Robin_Watts: yeah, sebras was furious about other Gtk projects leaking intentionally at his previous job | 10:02.42 |
| Robin_Watts | Thanks for the review. | 10:02.47 |
| tor8 | "oh, but they're only created once at startup, so we never need to clean these because they never leak" | 10:03.05 |
| | hello valgrind leak checker noise :( | 10:03.11 |
| Robin_Watts | tor8: yeah, I had a big discussion with the harfbuzz devs about this a couple of weeks ago. | 10:03.56 |
| tor8 | Memento_startIgnoring, Memento_stopIgnoring? | 10:04.23 |
| Robin_Watts | Essentially the original authors attitude is "runtime allocators aren't ever important". | 10:04.26 |
| | tor8: yeah, that's what I do to stop Memento complaining. | 10:04.37 |
| | but my current detection hack doesn't use memento on the cluster, cos, speed. | 10:05.10 |
| | It seems the original harfbuzz author has moved on to pastures new, and my comments have been noted for some future version where they are prepared to accept a more complicated API to solve it. | 10:06.15 |
| tor8 | Robin_Watts: we could make the use of harfbuzz a runtime option, and enforce the 'quickshape' flag in walk_string | 10:06.37 |
| | or a compile time option based on some -DLEAK_CHECK_CLUSTER predefine | 10:06.58 |
| Robin_Watts | tor8: I am more tempted to hack an 'hb_closedown' function into our copy of harfbuzz :) | 10:09.14 |
| | but I can live with epub etc leaking. | 10:09.45 |
| tor8 | yeah. | 10:09.55 |
| | as long as we know where the leaks are... | 10:10.05 |
| Robin_Watts | yeah, Memento_startLeaking/stopLeaking covers us for that. | 10:10.32 |
| tor8 | problem will become worse if we start using harfbuzz to shape text for annotation creation | 10:10.36 |
| | but yeah, the start/stopLeaking masking is good to have. can we do the same for valgrind? | 10:11.10 |
| Robin_Watts | tor8: Harfbuzz already has a hook to use atexit to clear all the leaked memory. | 10:11.17 |
| | (if you build it with the right runes) | 10:11.25 |
| | so exposing that mechanism via a deliberate call wouldn't be hard. | 10:11.44 |
| | tor8: 1 more commit ready to go: "Fix possible leaks..." | 11:19.47 |
| | just testing the next 3 now. | 11:19.58 |
| sebras | Robin_Watts: care to take a look at sebras/master while I take a look at robin/master? | 13:41.55 |
| Robin_Watts | sebras: I have my head up SOT at the moment. | 13:42.08 |
| | Let me have a quick look. | 13:42.14 |
| | fz_var(ctx, doc); WTF? :) | 13:43.15 |
| | fz_needs_password(doc) - how can that ever have worked? | 13:43.31 |
| sebras | it was never compiled. | 13:43.40 |
| Robin_Watts | ah, ok. | 13:43.53 |
| sebras | Robin_Watts: your patch bounds the page twice if the output format is OUT_TEXT I think. | 13:43.58 |
| Robin_Watts | sebras: THanks, will check. | 13:44.14 |
| | all lgtm | 13:44.41 |
| | Updated commit online. | 13:46.11 |
| sebras | Robin_Watts: how do you feel about adding the changes in sebras/robinmaster to your patch? | 13:51.12 |
| | the first hunk removes redundant code, the 2nd drops the device early and the third one coalesces the normal path with the always path. | 13:51.55 |
| Robin_Watts | Sure. | 13:52.52 |
| | feel free to squash it in and push. | 13:52.59 |
| | I shall run back to SOT... | 13:53.06 |
| sebras | ok. | 13:54.03 |
| Robin_Watts | sebras: git | 15:30.06 |
| | sebras: This one got dropped: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=4ee34af1881ebab1ad8930a14b85d479a4e533d5 | 15:30.13 |
| | (apologies, wasn't calling you a git :) ) | 15:30.19 |
| sebras | Robin_Watts: well... it _would_ be a fitting description I guess. :) | 15:30.44 |
| | Robin_Watts: I was the one that mentioned that, how did I forget it?! | 15:33.00 |
| | Robin_Watts: other than that maybe it should be "mudraw:" as a prefix in the commit message (I'm not sure) LGTM. | 15:34.52 |
| malc_ | sebras: it's reserved by another swede (although with a swedish/american tint) | 15:35.15 |
| sebras | sebras: Torvalds? Actually he's from the Swedish speaking minority in Finland, but by now I assume he's counted as being American. | 15:39.15 |
| | malc_: ^^ | 15:39.20 |
| malc_ | sebras: hence tint caveat | 15:39.44 |
| Robin_Watts | sebras: You still awake? | 16:39.50 |
| | or tor8? | 16:40.23 |
| sebras | Robin_Watts: I'm here. | 17:10.26 |
| | Robin_Watts: I was on the phone though. | 17:10.52 |
| Robin_Watts | just spotted something strange in icc_base_conv_pixmap | 17:11.05 |
| | and wanted a sanity check. | 17:11.12 |
| sebras | ok. | 17:11.52 |
| Robin_Watts | If we have src being a pixmap with alpha, then we copy the colors+alphas into src_f. | 17:12.23 |
| | We call convert_to_icc_base, which doesn't know how many things we just copied, so presumably operates just on the number of colors, rather than alpha. | 17:12.55 |
| | then we copy stuff out of the results, which will mean that the alpha is never set. | 17:13.15 |
| sebras | well.. that depends on sn and bn right? | 17:14.29 |
| Robin_Watts | sn = src->alpha | 17:15.00 |
| sebras | src->n and base->n, they don't include alpha anymore IIRC. | 17:15.09 |
| | I guess that would leave alpha unconverted at the end of outputpos..? | 17:15.33 |
| Robin_Watts | sebras: pixmap->n includes alpha. | 17:15.34 |
| | n = "total number of components, including alpha" | 17:15.55 |
| sebras | ok... in that case the alpha is converted but it probably not converted, no. | 17:16.02 |
| Robin_Watts | soon to be "total number of components, including spots and alpha" | 17:16.14 |
| sebras | now... can an icc profile affect alpha? | 17:16.14 |
| | shoudl it? | 17:16.16 |
| | ok. | 17:16.22 |
| Robin_Watts | ICC profiles never affect alpha. | 17:16.54 |
| | The ICC engine can be told to 'pass through' alpha. | 17:17.07 |
| | (and we've had to make changes to ensure that we de-premultiply the alpha , and then repremultiply it again afterwards). | 17:17.31 |
| | I'll keep bashing. Thanks for the sanity check. | 17:17.57 |
| sebras | so it seems e.g. for indexed colorspaces the base colorspace is queried about the number of components. | 17:19.57 |
| | and that doesn't include alpha. | 17:21.00 |
| | that means that indexed_to_alt() e.g. will use the wrong number of components. | 17:21.34 |
| | so yes, I believe your analysis is correc.t | 17:21.42 |
| Robin_Watts | Ta. | 17:21.48 |
| sebras | but this seems to only affect Indexed and Separation for some reason. | 17:22.16 |
| | I guess we don't use the icc stuff for other colorspaces. | 17:22.29 |
| | or I just don't understand how it works. :) | 17:23.02 |
| | tor8: on sebras/master is 10 or so patches that all relate to dropping things in case of exceptions. | 18:26.16 |
| Robin_Watts | looks. | 18:26.52 |
| sebras | tor8: I think the most controversial is probably the ones relating to js. | 18:27.04 |
| | I also have a number of patches pending which affect the error handling in some of the tools. | 18:27.37 |
| Robin_Watts | First commit looks suspect to me. | 18:28.05 |
| sebras | I suspect these will cause more discussions so I'm attempting to get the trivial ones in to start with. | 18:28.08 |
| | Robin_Watts: why? | 18:28.22 |
| Robin_Watts | You're putting sc into s and dropping sc. | 18:28.37 |
| | Then you're using sc in the next 4 lines. | 18:28.47 |
| sebras | is this not the same as just calling pdf_dict_get() on it? | 18:29.32 |
| | I can't forsee that sc would be reallocated or released. | 18:29.49 |
| | but perhaps it could..? | 18:29.55 |
| Robin_Watts | It's 'safe', but it's bad form. | 18:30.01 |
| sebras | wouldn't all pdf_dict_get()s be equally bad form? | 18:30.22 |
| Robin_Watts | yeah, I guess. | 18:30.52 |
| sebras | I'll fix the patch of course, but it is crucial that I didn't make any non-safe mistake. :) | 18:31.33 |
| Robin_Watts | I'd be more tempted to use an fz_drop_obj(ctx, sc) in the fz_always. | 18:31.57 |
| | And to only put sc into s at the last moment. | 18:32.22 |
| | That way if we throw an error we haven't affected the structure of the doc. | 18:32.36 |
| sebras | Robin_Watts: pdf_create_annot() would be doing the same 'safe' thing with annot_arr, right? | 18:32.41 |
| | Robin_Watts: yeah I have been trying to do it that way before, but I have been unsure which way is the best. | 18:33.10 |
| | Robin_Watts: i.e. try to not mess with the document until we virtually know it will not fail. | 18:33.37 |
| Robin_Watts | pdf_annot_edit is similarly 'risky, but OK', yes. | 18:34.38 |
| | but given that annot_arr is normally a borrowed reference, I can't see a different way to do it. | 18:35.10 |
| sebras | Robin_Watts: I guess the only way to do this correctly is to have pdf_dict_get() return a reference which must be dropped. | 18:35.55 |
| Robin_Watts | sebras: Or to manually fz_keep it. | 18:36.09 |
| | yeah, ignore my criticisms, other than to say it might be nicest not to modify the object until the last possible minute. | 18:36.38 |
| sebras | Robin_Watts: in a non-multithreaded mupdf you would be guaranteed that it was there upon fz_keep, yes. in a multithreaded setting I have no idea. :) | 18:36.44 |
| Robin_Watts | In multithreaded MUPDF, only 1 thread is supposed to be accessing the file at a time. | 18:37.08 |
| | All the rest is using display lists etc. | 18:37.23 |
| sebras | right. | 18:37.32 |
| Robin_Watts | sebras: Does js_freestate not cope with being called with NULL? | 18:39.31 |
| | All destructors should cope with being called with NULL, IMAO. | 18:39.40 |
| | And malloc_struct is guaranteed to fill structures with 0's. | 18:40.12 |
| | so js->imp = NULL; already. | 18:40.22 |
| sebras | no, it appears to promptly deref its argument. | 18:41.03 |
| | Robin_Watts: in that case I guess the correct fix is in js_freestate() in mujs. | 18:41.47 |
| Robin_Watts | sebras: yeah, I would reckon so. | 18:41.57 |
| | I don't get "pdf: Initialise variables to..." | 18:43.46 |
| | prev is never used after a throw. | 18:43.54 |
| | first IS used after a throw, but has been fz_var'd and is inited in the top of the fz_try.... | 18:44.43 |
| | I guess if the fz_try fails, it might make a difference. | 18:44.56 |
| sebras | Robin_Watts: yes, if e.g. pdf_mark_obj() fails before first is set. | 18:47.05 |
| | Robin_Watts: then we try to drop an uninitialized pointer. | 18:47.23 |
| Robin_Watts | Urm...? | 18:47.36 |
| sebras | oh.. no. yeah, the fz_try() don't mind me. | 18:47.58 |
| Robin_Watts | ok :) | 18:48.07 |
| sebras | I should sleep, it's 3am. | 18:48.11 |
| Robin_Watts | fz_new_buffer_from_pixmap_as_png... | 18:48.31 |
| | previously that didn't drop the pixmap it was given. | 18:48.39 |
| | now it does. | 18:48.52 |
| sebras | I know we've been back and forth at this one before. | 18:49.03 |
| Robin_Watts | oh, no, I see. | 18:49.04 |
| sebras | this is due to the return statement early on in the function. | 18:49.16 |
| Robin_Watts | fz_new_buffer_from_image_as_png now drops it later than it did before. | 18:49.23 |
| | Previously png_from_pixmap avoided having 2 copies of the pixmap around during the png writing if we were dropping. | 18:50.12 |
| | now the pixmaps are held for longer. | 18:50.25 |
| sebras | that is true. | 18:50.38 |
| | the option is of course to drop at the return if drop is set. | 18:50.58 |
| Robin_Watts | if (w == 0 || h == 0) { if (drop).... | 18:51.05 |
| | yeah. | 18:51.09 |
| sebras | or better yet, write out a 0 sized png if that is legal..? | 18:51.26 |
| | 0-sized as in 0 pixels, not bytes. | 18:51.47 |
| | tor8: there's a patch for you on mujs:sebras/master making js_freestate() return on NULL. | 18:52.52 |
| | tor8: as seen in the discussion above I need to fix sebras/master a bit due robin's review. :) | 18:53.21 |
| sebras | sleeps, will check logs tomorrow! | 18:53.43 |
| Robin_Watts | sebras: The last commit I have comments on. We can discuss later. Sleep well! | 18:54.04 |
| | Forward 1 day (to 2017/07/05)>>> | |
Log of #mupdf at irc.freenode.net.