| | <<<Back 1 day (to 2017/06/12) | 20170613 |
| sebras | tor8: hiya! do you mind helping out chasing the memory leak in begin_softmask() for a bit? | 10:26.29 |
| tor8 | sebras: morning | 10:26.52 |
| sebras | tor8: since I have changed the code a bit you'd need to look at sebras/wip. | 10:27.21 |
| | I can see that in begin_softmask() before the fz_try() clauses we get the softmask and its resources from the gstate stack. | 10:27.57 |
| | but I have yet to figure out where they are normally dropped..? | 10:28.10 |
| | and why having a fz_try() error out would cause these objects to never be dropped. | 10:28.30 |
| | at least this is what I think the scenario is, as I can only see the backtraces for the objects being leaked and not the entire state at the time they were leaked. :-/ | 10:29.09 |
| tor8 | sebras: pdf_grestore is called at the end of interpretation to pop back to the top and clear all the gstates | 10:31.11 |
| | see pdf_drop_run_processor | 10:32.04 |
| sebras | but the softmask is removed from the gstate and placed in save and then removed from the gstate. | 10:32.38 |
| tor8 | and pdf_grestore calls pdf_drop_gstate which drops the softmask | 10:32.41 |
| sebras | later it seems to be put back by pdf_end_mask() but that is never called (because of the exception) | 10:32.56 |
| tor8 | so that's where they're *normally* dropped | 10:32.57 |
| | now on to your question :) | 10:33.01 |
| sebras | reads | 10:33.10 |
| | aha! ok, and that explains why the softmask is never dropped: it is not in the gstate stack anymore. | 10:34.05 |
| | pdf_show_path() saves it in its internal variable and never drops it! | 10:34.23 |
| | in the softmask variable. | 10:34.29 |
| | would it be safe to call pdf_end_group() end group in fz_always()/fz_catch()? | 10:35.02 |
| tor8 | sebras: the softmask_save struct? | 10:35.13 |
| sebras | yes. | 10:35.35 |
| | this hold the only reference to the softmask after begin_softmask() errors out. | 10:35.54 |
| tor8 | you mean move the final pdf_end_group int othe fz_always? | 10:35.54 |
| sebras | yes | 10:35.58 |
| tor8 | in pdf_show_path | 10:36.01 |
| sebras | yes | 10:36.04 |
| tor8 | if you split the try-block so you don't call it before having called and succeeded at calling begin_softmask | 10:37.11 |
| | eh, pdf_begin_group | 10:37.25 |
| sebras | hm.. maybe pdf_show_shade() would have a similar issue. | 10:37.57 |
| | or indeed anything having a softmask_save variable. | 10:38.16 |
| | tor8: hm.. so pdf_begin_group() is what is erroring out (due to internally calling begin_softmask() which failed) | 10:39.41 |
| | tor8: though it fails _after_ having populated the softmask_save variable. | 10:39.59 |
| | tor8: maybe the issue is still in begin_softmask() in that in case of error anything saved into the softmask_save variable needs to be undone upon error..? | 10:40.56 |
| tor8 | this is robin's code, your guess is as good as mine :) | 10:41.35 |
| sebras | tor8: robin's code's users are also your users. ;) | 10:43.18 |
| | but yeah, I can see how it is difficult to sort out what was intended, just like I'm having trouble. | 10:43.41 |
| | if pdf_run_xobject() fails in being_softmask() we do e.g. call fz_end_mask() to undo the preceeding fz_begin_mask() even in case of error. | 10:44.36 |
| tor8 | ok, so it's a case of the error happening in pdf_begin_group/begin_softmask after having stashed the old mask, but not restoring the old state | 10:45.57 |
| sebras | yes, something like that. | 10:46.16 |
| | I guess this is what the FIXME in begin_softmask() is about too. | 10:46.29 |
| | but now that I'm actually throwing from inside I also get to resolve the error handling. | 10:46.55 |
| | I guess in the first fz_catch() I should simply undo what happens in the beginning of begin_softmask() | 10:47.43 |
| tor8 | the first catch that unconditionally rethrows, yes, that would be a reasonable place to do the undoing. | 10:53.35 |
| | alternatively, don't change the gstate until after the fz_begin_mask call | 10:53.50 |
| sebras | yes, the latter would probably be easier to do. | 10:54.27 |
| | but is that safe? | 10:54.40 |
| tor8 | it should be | 10:54.47 |
| | nothing in fz_begin_mask uses the pdf_csi and pdf_gstate structs | 10:54.58 |
| sebras | tor8: ok. these are the things I don't know. | 10:55.11 |
| tor8 | the pdf_csi is the interpreter state context, which also has the pdf graphics state (pdf_gstate) stack | 10:55.58 |
| | all of which is only used by the interpreter to push/pop and set the correct state to various device calls | 10:56.18 |
| | s/interpreter/pdf_op_processor/ | 10:56.35 |
| | eh, pdf_run_processor | 10:56.45 |
| sebras | aha, and so then this is never fed back into fitz. | 10:56.47 |
| | I see. | 10:56.49 |
| | and indeed moving the gstate manipulation until after fz_begin_mask() succeeded resolved the issue. | 10:57.14 |
| tor8 | exactly. the fz_begin_mask is the underlying device call | 10:57.20 |
| sebras | so, about the FIXME after pdf_run_xobject() though. | 10:57.37 |
| tor8 | everything else is just keeping track of the PDF interpreter state | 10:57.50 |
| sebras | wouldn't we be able to restore the clip stack in fz_catch()? | 10:57.58 |
| | basically undoing the things I just moved and then fz_rethrow()? | 10:58.34 |
| | N.B. I'm only thinking about fixing the FIXME, not the TODO. | 10:59.05 |
| tor8 | I'd ask Robin_Watts | 10:59.13 |
| | we also *do* throw from there, so it looks like the FIXME comment may be out of date as well | 11:00.00 |
| | but only for one type of exception | 11:00.07 |
| sebras | tor8: seems like it might be RobinWattsLenovo that I should communicate with today. | 11:00.07 |
| | ah, the TRYLATER stuff, right. | 11:00.30 |
| RobinWattsLenovo | reads logs. | 11:00.32 |
| tor8 | yeah. my gut feeling is that we should just throw there, for all exceptions. | 11:00.53 |
| | the pdf_grestore stuff only takes care of not leaking memory | 11:01.54 |
| RobinWattsLenovo | I'm going to have to run Helen to the station in a mo, which will take 45 mins or so, but when I get back, I will be at your disposal, sebras. | 11:02.18 |
| tor8 | so the device would get unbalanced push/pop calls in the case of an error | 11:02.29 |
| | and if we want to render partial content, that would be bad | 11:02.37 |
| | robin has poked around the most with this so I'm going to defer to his judgement | 11:02.55 |
| sebras | tor8: ok. I'll wait for a bit. | 11:04.00 |
| | tor8: but just moving the call fz_begin_mask() resolved the leak I was seeing. | 11:04.17 |
| | and I actually created that myself by letting fz_begin_mask() rethrow. | 11:04.34 |
| tor8 | sebras: right. | 11:05.46 |
| | ah, right. *that* might be worth talking to Robin_Watts about too (the swallowed errors there) | 11:06.46 |
| | but I guess all of that is so we can sort of recover from errors and have balanced push/pop calls even when things fail | 11:07.14 |
| | maybe we should tweak that code to delay the error rather than swallow it | 11:07.49 |
| | aha, they do delay the error! | 11:08.51 |
| sebras | when is that detected? | 11:09.26 |
| tor8 | when we get back to a known safe balancing depth, we throw a new error with the swallowed error's message | 11:09.35 |
| | in source/fitz/device.c | 11:09.40 |
| | see fz_pop_clip and fz_end_group | 11:09.51 |
| | if the error_depth drops back to zero we throw | 11:10.05 |
| sebras | aha! | 11:10.51 |
| tor8 | this part of code basically detects some errors, and then ignores all device calls until it is balanced again | 11:11.31 |
| sebras | https://pastebin.com/raw/Ysbg6Ry4 that error handling didn't work... | 11:11.40 |
| tor8 | so that the interpreter isn't affected by device errors | 11:11.50 |
| | don't ask me why we do that though ... I've completely forgotten | 11:12.41 |
| sebras | I guess these high level situations are the ones that we really should write down. | 11:13.21 |
| | write comments about? on write on paper? I get so confused about languages and grammar right now. ;) | 11:14.01 |
| | ("on write on paper" is what you would say in mandarin chinese apparently) | 11:14.27 |
| | ator8: cleanup_state in pdf_run_xobject() uses >= to check what to cleanup, but if e.g. fz_begin_group() errors out then cleanup_state == 2 and we still call fz_end_group(). | 11:23.20 |
| | tor8: ^^ | 11:23.50 |
| tor8 | sebras: that leads me to believe that this code is written with the assumption that some of these device calls don't throw | 11:31.43 |
| | but that looks like an oversight, the cleanup_state = 2 should happen *after* the call, not before | 11:32.14 |
| | but the comment seems to say that even if fz_begin_group throws it expects a fz_end_group call? | 11:32.34 |
| | that's counter to my understanding | 11:32.42 |
| RobinWattsLenovo | back. | 11:54.52 |
| tor8 | RobinWattsLenovo: looking over the lcms branch | 12:00.42 |
| RobinWattsLenovo | probably best to squash it and look at the single commit | 12:01.13 |
| tor8 | I did :) | 12:01.27 |
| RobinWattsLenovo | OK. | 12:01.30 |
| tor8 | I rebased your robin/lcms branch and removed the WIP: text shaper | 12:01.39 |
| | fz_set_oi *looks* like it doesn't do anything | 12:02.38 |
| RobinWattsLenovo | tor8: I'm just doing the same here. Looking at the oi stuff. | 12:08.26 |
| tor8 | I've got a few pages worth of function/struct naming suggestions (as expected...) | 12:08.53 |
| RobinWattsLenovo | It drops the old oi, and keeps the new one. | 12:09.07 |
| tor8 | I'm not fond of all the abbreviations | 12:09.10 |
| RobinWattsLenovo | Looks sane to me. | 12:09.12 |
| tor8 | yeah, but nothing uses the context->oi | 12:09.19 |
| RobinWattsLenovo | tor8: Sure. naming tweaks are good. | 12:09.25 |
| tor8 | I suspect the bug is in pdf_init_document, where you load the doc oi and then call pdf_set_oi | 12:09.35 |
| | pdf_get_oi returns doc->oi | 12:09.46 |
| | but that isn't ever set, as I see it | 12:09.55 |
| RobinWattsLenovo | fz_get_outputintent | 12:10.13 |
| tor8 | oh, the line above it | 12:10.19 |
| | fz_get_outputintent returns the output intent from the fz_default_cs struct | 12:10.47 |
| | not from the color context | 12:10.50 |
| RobinWattsLenovo | This is michaels code (I've just been tweaking it). | 12:10.59 |
| tor8 | I suspect it's a remnant of something mvrhel tried at the meeting and maybe forgot to remove? | 12:11.04 |
| | he mentioned adding an output intent to the context, then a few hours later having figured out how to not need it in the context | 12:11.26 |
| RobinWattsLenovo | I'll try removing and see what happens. | 12:11.43 |
| | Something went wrong with my last rebase. Will try again. | 12:17.36 |
| | ok, think that's a git bug. The rebase repeatedly goes wrong for me. If I skip the 2 last commits and then cherry pick them it it works OK though | 12:29.00 |
| | so I've got a commit removing the global output intent. | 12:29.13 |
| tor8 | RobinWattsLenovo: is layertest.c supposed to be included here? | 12:29.51 |
| RobinWattsLenovo | tor8: SERIOUSLY? | 12:30.08 |
| | I couldn't even find that file when I hunted for it the other day... | 12:30.21 |
| tor8 | commit 2925a7f9c359f974e427b37e87919eeb1502e9cf | 12:32.37 |
| RobinWattsLenovo | I have a different SHA but that's to be expected. | 12:32.49 |
| | I'm going to update git, then remove that. | 12:32.58 |
| tor8 | and mutest.c? | 12:33.23 |
| | okay, I have one tedious change I'd like to see though (the naming stuff is simple search/replace stuff) | 12:34.26 |
| RobinWattsLenovo | BALLS BALLS BALLS. | 12:35.00 |
| tor8 | the argument for the color params in the device calls sits between the colorspace and the color (now it's: colorspace, color_params, color, alpha) | 12:35.12 |
| RobinWattsLenovo | The scanconverter stuff caused 16000 diffs, then another 16000 diffs. | 12:35.14 |
| | so I'm going to back that out and figure out what went wrong. | 12:35.39 |
| tor8 | in my mind the colorspace belongs with the color and alpha (as a triplet type of thing) so I'd like the color_params to come either before or after that triplet | 12:35.49 |
| | RobinWattsLenovo: okay. | 12:35.57 |
| RobinWattsLenovo | tor8: That seems fair. I can make that change | 12:36.06 |
| | Do you want to mail me your list of renames etc and I'll take care of all that after lunch ? | 12:36.28 |
| tor8 | I can email a list of names and we can bash them out and then I can do the renaming with some sed magic | 12:36.44 |
| | or if you want to do the work of renaming, that's fine too | 12:37.11 |
| | fz_write_icc in the banded writer; the order of calls ought to be documented or the icc profile colorspace could go in the header writer? | 12:38.04 |
| RobinWattsLenovo | We could combine the ICC profile writing into the header, but then we'd need to pass the ICC profile intothe header writing. | 12:39.15 |
| | What if there are formats that need more than 1? | 12:39.22 |
| tor8 | TIFF, JPEG, and PNG only take one color profile IIRC | 12:41.14 |
| | the device calls set_default_cs and set_outputintent I don't understand the need for the function pointers in fz_device_s | 12:44.28 |
| | or well, the implementation seems a bit backwards | 12:45.08 |
| | I understand the need to save the call in the display list, but the way it's implemented is awkward | 12:45.31 |
| | dev->super.set_default_cs = fz_set_default_colorspace in draw-device.c f.ex. | 12:45.57 |
| | and the set_outputintent call has nothing to do with the device | 12:46.44 |
| sebras | I'll let you guys hash out the lcms stuff and return with the softmask stuff tomorrow since I have a test to stuff for tomorrow. | 12:50.17 |
| tor8 | fz_set_default_colorspace should call a device function, fz_device.default_cs field should move to fz_draw_device (and its callback function should set it) | 12:50.21 |
| | sebras: okay, good luck! | 12:50.26 |
| RobinWattsLenovo | tor8: ok I've removd mutest.c and layertest.c - I was being dim. | 12:51.17 |
| | I'll do the device call color_params shifting now. | 12:51.52 |
| | I'll go for after the triplet. | 12:52.03 |
| | Or... are the color_params more 'global' than the color? Should they go before it ? | 12:52.58 |
| | If we ever add a combined 'stroke and fill' device method' we'd be sending 2 colours, but only 1 color_params. | 12:53.56 |
| | hence the color params should go first, I reckon. | 12:54.05 |
| tor8 | it's more global, I could live with either before or as the final param (since it may often just be NULL) | 12:54.34 |
| | final might be easier for java and javascript as there we could make it optional | 12:55.00 |
| | Robin_Watts: ^ | 13:03.19 |
| RobinWattsLenovo | tor8: good point. | 13:05.56 |
| | hmm, no sebras. | 17:11.37 |
| | tor8: you here? | 17:11.45 |
| tor8 | RobinWattsLenovo: got company, but if you can wait a few hours I'll be around then | 17:40.49 |
| RobinWattsLenovo | tor8: No hurry at all. | 17:46.13 |
| tor8 | RobinWattsLenovo: okay, I'm here | 19:40.20 |
| avih | can someone confirm that mujs.h has executable flag set after it's installed? already confirmed on two systems. | 21:36.24 |
| | seems that mujs' Makefile has "install mujs.h $(DESTDIR)$(incdir)" and does +x by default [install --help] "-m, --mode=MODE set permission mode (as in chmod), instead of rwxr-xr-x" | 21:41.05 |
| RobinWattsLenovo | tor8: Was just pondering the java interface. | 22:03.34 |
| | Do I do the color params as a java object, or do I just bundle them into an int? | 22:04.23 |
| | They are a sum total of 5 bits, so creating/destroying a java object seems like an unnecessary overhead. | 22:04.53 |
| | but then maybe that kind of thing is expected for java? | 22:05.07 |
| avih | RobinWattsLenovo: do you like the brand so match you added it to your name?! :) so, the js support in mpv was about to get merged again, gone through some review, and when mpv maintainer wanted to finally try it, he bumped into some issues: | 23:17.43 |
| | 1. missing pkgconfig file. it's not a big deal in general, but apparently it is for him with complex build configurations etc. he went as far as suggesting it might be a blocker for him. | 23:18.55 |
| | 2. mujs.j is installed as executable file. easily solvable with some install -m $whatever | 23:19.30 |
| | 3. mpv can be built as libmpv, and as such it had some issues with linking with mujs which (apparently rightfully) doesn't enable -fPIC when building it as static lib. he did solve it locally, but i think it would have been nice if there was also a libmujs.so which will get -fPIC automatically with most build systems (i think, i'm not too knowledgeable with build subjects) | 23:21.15 |
| | that's it. | 23:21.22 |
| | mujs.h* installed with +x | 23:22.02 |
| | he also created this simple patch to create and install a pkgconfig file. maybe tor8 could incorporate this or a similar solution upstream http://sprunge.us/cDMD | 23:23.33 |
| | Forward 1 day (to 2017/06/14)>>> | |
Log of #mupdf at irc.freenode.net.