| | <<<Back 1 day (to 2016/10/10) | 20161011 |
| sebras | tor8: I have a patch brewing where I want to move the fz_free() of doc from xps_drop_document() to fz_drop_document(). | 10:50.17 |
| | i.e. the super class frees the document. this makes freeing documents inline with pages and all the other super-thingies. | 10:50.39 |
| | removes a few lines of code and makes sure we never forget to free the document. | 10:50.57 |
| | tor8: http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/fitz/document.c;h=e6d814b0c0e94b8788a9c0c287b7a28071403839;hp=4c02f1c4fb7bf6defad6164722b6145258fa234d;hb=fa562f5ed6617b37636327356bd7653bdc39efa5;hpb=eb23fc80997bbb4409abe06a8392c0da4e445504 | 10:51.32 |
| | and http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/xps/xps-zip.c;h=1d8f5bc8c8e47242ba222322b23966f9e24f57e7;hp=fbcb6d448eac92a28d654bd0ee3104d4975aa96f;hb=fa562f5ed6617b37636327356bd7653bdc39efa5;hpb=eb23fc80997bbb4409abe06a8392c0da4e445504 | 10:51.37 |
| tor8 | sebras: yes, that's a good change to do. | 10:51.52 |
| sebras | tor8: ok, noted. | 10:52.02 |
| | tor8: also http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/html/epub-doc.c;h=96c426942a71fdfc3ad3a0f76a690ff50a3c77d9;hp=f179c5e1459407237fec646d7b2dcd751e83bffb;hb=fa562f5ed6617b37636327356bd7653bdc39efa5;hpb=eb23fc80997bbb4409abe06a8392c0da4e445504 | 10:52.05 |
| | in epub_parse_chapter() e.g. fz_write_buffer_byte() might throw (because it resizes the buffer) and if so we leak the buffer. | 10:52.31 |
| | do we care about these kind of things? | 10:52.36 |
| tor8 | yeah, the error handling in large chunks of epub is not fool proof | 10:52.37 |
| | don't even start looking at the css parsing :) | 10:53.03 |
| | I want to change the css parser to use a fz_pool for allocation | 10:53.12 |
| sebras | tor8: right. | 10:53.16 |
| tor8 | then you only need a top-level try/catch to catch the memory leaks | 10:53.23 |
| sebras | tor8: so no need to care about the epub/html/css-stuff? | 10:53.26 |
| tor8 | I think we should start caring, at least for the higher level stuff | 10:53.57 |
| | the html box stuff is already handled with a pool allocator | 10:54.18 |
| sebras | tor8: yeah, but if you are planning an imminent change to pool allocation in epub/html/css then I'll stay out of that area and let you muck about there on your own. :) | 10:54.41 |
| tor8 | sebras: only in css-parse.c | 10:54.53 |
| sebras | tor8: right. | 10:55.02 |
| | tor8: so that means http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/pdf/pdf-write.c;h=dab3f9cdb0370b924cdc13981a3a88381658b4da;hp=eb1a2f29167bce369c44bd3c84a3498b645aafba;hb=fa562f5ed6617b37636327356bd7653bdc39efa5;hpb=eb23fc80997bbb4409abe06a8392c0da4e445504 has similar issues that I ought to fix. | 10:55.18 |
| tor8 | yes | 10:55.29 |
| sebras | tor8: basically I did git grep -B 5 -Hn '^\tfz_drop_' and looked at this code since I assumed that these were outside of a fz_always()/fz_catch(). | 10:56.25 |
| | same for pdf_drop_. | 10:56.31 |
| tor8 | I was just wondering how you dig these up | 10:58.02 |
| sebras | tor8: guessing. :) | 10:58.14 |
| | tor8: I think you could do it using gawk or something, but it is quite hard to do it well. | 10:58.51 |
| | tor8: a related question: http://git.ghostscript.com/?p=user/sebras/mupdf.git;a=blobdiff;f=source/pdf/pdf-page.c;h=9522ce82d780eafafed8ce71167b4c48a6cd2624;hp=73d01480646c87a1093110b5489c64734eeb0c50;hb=fa562f5ed6617b37636327356bd7653bdc39efa5;hpb=eb23fc80997bbb4409abe06a8392c0da4e445504 | 10:59.16 |
| | there are quite a number of drop_imp() functions. | 10:59.29 |
| | I think it is unnecessary for these to check ctx and the object they are removing as that ought to be handled by the upper layer, in this case fz_drop_page().l | 11:00.02 |
| tor8 | sebras: yeah. | 11:00.11 |
| sebras | tor8: also page_objects_list_destroy() -> free_page_objects_list()? | 11:03.14 |
| tor8 | sebras: that's robin's private functions for writing pdf files. I'd leave those alone for now. | 11:04.57 |
| sebras | tor8: so the entire pdf-write.c? including memory leaks in the style I just showed? | 11:05.35 |
| tor8 | I meant the naming only | 11:05.57 |
| sebras | ok. | 11:06.01 |
| tor8 | those functions use robin's object oriented type naming "noun_verb" which is a bit non-standard but better consistent within the same module | 11:07.01 |
| sebras | I was mainly surprised when I saw a function named destroy when I thought it should be free. | 11:07.53 |
| Robin_Watts | prefers mofule_action rather than action_module | 11:10.45 |
| | but that's not mupdf style, so I live with the latter. | 11:11.00 |
| | (except where I forget) | 11:11.05 |
| | I would much prefer to be able to get the entire set of actions that can take place on a foo by grepping for fz_foo_... | 11:11.47 |
| | rather than having to look for fz_footle_foo, fz_fumble_foo, fz_frob_foo etc. | 11:12.08 |
| sebras | Robin_Watts: I guess the main argument against is that it reads like RPN. | 11:14.12 |
| Robin_Watts | I believe tor8's objection is that it reads like OO :) | 11:14.33 |
| sebras | Robin_Watts: correct, but expressed it in another way! :) | 11:15.05 |
| tor8 | that naming falls down when you start involving two types | 11:15.26 |
| | which one becomes the 'main' type :) | 11:15.35 |
| sebras | tor8: trivial patch at sebras/master | 11:15.55 |
| Robin_Watts | tor8: depends on the naming: fz_foo_to_baz, or fz_baz_from_foo | 11:16.06 |
| tor8 | fz_css_apply_to_html or fz_html_apply_css, etc | 11:16.43 |
| | minor quibbles :) my main gripe is OO, as you said before | 11:17.13 |
| | sebras: LGTCM | 11:17.38 |
| | looks good to confuse me? | 11:17.55 |
| Robin_Watts | tor8: So... we need to do a release. | 11:19.58 |
| | Does anyone have anything in particular they want to get in to the release? (or things that they think *should* be in the release?) | 11:20.27 |
| sebras | Robin_Watts: what did we decide about that P1 j2k bug? | 11:20.46 |
| Robin_Watts | sebras: bug697045 ? | 11:21.08 |
| sebras | ! | 11:21.12 |
| Robin_Watts | I think the plan was that we will do a "commercial" release of MuPDF this time, which will have the luratech decoder in it. | 11:22.05 |
| sebras | Robin_Watts: even though we have known issues? | 11:22.24 |
| Robin_Watts | but that's been thrown a bit by the new luratech drop having regressions. | 11:22.30 |
| sebras | I'm good with that, but we should make a conscious decision about it. | 11:22.38 |
| chrisl | Presumably using the previous Luratech release? | 11:22.54 |
| Robin_Watts | sebras: I think we do a commercial release with both luratech and openjpeg in the sources. | 11:23.12 |
| sebras | chrisl: I think that would be the best at this point. | 11:23.15 |
| Robin_Watts | And that way people can delete the luratech sources if they want. | 11:23.27 |
| | (hmm, that'll be interesting in the VS solution). | 11:23.49 |
| sebras | Robin_Watts: gs has no VS solution, right? | 11:24.14 |
| Robin_Watts | gs has a VS solution, but it's one where it just calls the makefiles. | 11:24.33 |
| | so there is nothing that's vital to "keep up to date". | 11:24.48 |
| | sebras: wrt that bug... AIUI the old luratech code renders it fine. | 11:25.29 |
| | the new luratech code spits the dummy. | 11:25.37 |
| sebras | Robin_Watts: it does, and no valgrind issues. | 11:25.40 |
| Robin_Watts | and openjpeg gives some warnings. | 11:25.46 |
| sebras | Robin_Watts: the old code complains a lot on _other_ files however. and these are fixed by the new code. | 11:26.04 |
| | Robin_Watts: right, I promised to build gs using the new luratech code and test it using the new file. | 11:26.37 |
| | Robin_Watts: and if there | 11:26.41 |
| Robin_Watts | I have no experience of the luratech code (and limited experience of openjpeg), but would it be possible to compare the 2 decoders handling of the point at which we get the warning? | 11:26.58 |
| sebras | 's a problem henrys said we (he?) should contact the devs and ask for help. | 11:27.01 |
| Robin_Watts | i.e. if there is a simple fix we can apply to openjpeg, that would be good. | 11:27.24 |
| sebras | Robin_Watts: I haven't looked into the openjpeg code at all. | 11:27.42 |
| Robin_Watts | ok. | 11:27.47 |
| sebras | Robin_Watts: only diffed the old and new luratech code. | 11:27.54 |
| chrisl | thinks: legal issues with such actions...... | 11:28.03 |
| Robin_Watts | chrisl: Yeah... I wasn't proposing copying code. | 11:28.21 |
| | but yes. | 11:28.28 |
| | OK, so running backwards through the bugs, are there any that we think we can fix before the release? | 11:28.48 |
| | bug 697183 seems like it might be simple to fix, but it can't happen on windows. | 11:29.52 |
| | bug 697151 should be closable with a tweaked version of sebras patch. | 11:30.19 |
| sebras | Robin_Watts: yes, it's brewing. | 11:30.36 |
| Robin_Watts | bug 697114 should be closable when we do the new release. | 11:30.43 |
| sebras | Robin_Watts: how so | 11:31.03 |
| Robin_Watts | by us putting up a readme with some md5 sums in? :) | 11:31.30 |
| | where we have a mupdf-1.10-win32.zip, we also have a mupdf-1.10-win32.txt with the md5 sum in. | 11:32.39 |
| | AIUI that's all they are really asking for. | 11:32.45 |
| sebras | Robin_Watts: md5sum _and_ https. | 11:33.14 |
| | Robin_Watts: or maybe more likely SHA1. | 11:33.32 |
| chrisl | and SHA256 and SHA512.... the requests will come :-( | 11:34.03 |
| Robin_Watts | sebras: md5 is enough, I feel. The main thing is that people can get it and then be sure it's the right thing. | 11:34.42 |
| | We do downloads for gs using github. If we do the same with mupdf, then we a) save on casper bandwidth bills, and b) get https. | 11:35.30 |
| chrisl | Robin_Watts: md5 is outdated and insecure..... I keep getting told <sigh> | 11:35.56 |
| Robin_Watts | chrisl: When someone tells you that, ask them to find you another string that matches the hash of "don't waste my time". | 11:36.42 |
| chrisl | I just ignore them now - mucking fuppets....... | 11:37.15 |
| Robin_Watts | bug 697054 seems like it should be easy to fix. | 11:37.31 |
| sebras | Robin_Watts: is 697013 important? | 11:37.54 |
| Robin_Watts | bug 697013 sounds like I should look into it, yes. | 11:38.04 |
| | bug 697012 - part of it is a pretty trivial fix. | 11:38.26 |
| sebras | Robin_Watts: the latter part? | 11:38.54 |
| Robin_Watts | (i.e. the equivalent to what henry did in gs is trivial) | 11:39.02 |
| | the latter part is more work, I reckon. | 11:39.16 |
| | I don't think we should touch bug 696993 until one of us runs into it, we get more credible reports, or it becomes a commercial customer. | 11:40.52 |
| | bug 696959 seems unimportant, unless you had a librar thing kicking around? (I vaguely thought you might have looked at this?) | 11:41.37 |
| | bug 696958 sounds like it's a simple enough fix. | 11:42.13 |
| | bug 696939 sounds plausible, and simple and there is a patch. | 11:43.19 |
| | and after that we're back into bugs that have been there for several releases so there aren't likely to be many trivial things to fix in there. | 11:44.39 |
| sebras | Robin_Watts: I did look into 696959. there is a libarchive which has a permissive license could be used. | 11:45.23 |
| | Robin_Watts: but not in time for the next release. | 11:45.29 |
| Robin_Watts | sebras: Right. I just wanted to check in case there was a 99% finished thing on a branch somewhere. | 11:45.46 |
| sebras | Robin_Watts: tor8 rejected the idea of linking to the library and insteaed preferred to import the code. | 11:45.51 |
| tor8 | sebras: is this the libarchive thing? | 11:46.19 |
| sebras | tor8: it is. | 11:46.23 |
| tor8 | yeah, importing the big overengineered library seemed like a lot of overkill | 11:46.44 |
| sebras | tor8: I haven't gotten around to look at how to extract that code yet. | 11:47.02 |
| | tor8: I did look at the license though and it seems like it is mainly BSDish. | 11:47.28 |
| Robin_Watts | no worries. it's not a priority for us. certainly not for the release. | 11:47.42 |
| sebras | chrisl: btw... all the cool kids use Keccak now. just so you know. | 11:47.46 |
| chrisl | sebras: I'm not much of a follower of fashion ;-) | 11:48.40 |
| | Really, we should host the sums on a different server, but it's all rather a pain. | 11:50.06 |
| sebras | Robin_Watts: Re: 697012 wait... so you want to keep the buffer overflow for the next release? | 11:50.11 |
| Robin_Watts | sebras: No. I want to fix the buffer overflow (that's the thing that Henry did in gxps). | 11:52.55 |
| | That's easy. | 11:52.57 |
| | I want to leave the "reorganise the code..." bit for later. | 11:53.20 |
| sebras | makes sense. | 11:53.33 |
| Robin_Watts | oh, I see. The bug lists 2 overflows. | 11:53.56 |
| | the first one henry has fixed. then there is the "reorganise", then there is "oh, and another overflow". | 11:54.13 |
| | so we should fix the first and last. | 11:54.19 |
| | Is there anything OTHER than what I've listed that we want to fix for this release? | 11:54.37 |
| tor8 | chrisl: downloads on github, checksums on casper, ought to do it? | 11:59.35 |
| kens | Robin_Watts : Bug 697013 is a bonkers test case, the page is ner 200 inches square, the content is a monster path, Acrobat fails to display the result. Ghostscript produces what I htink is hte correct output, but its still a mad file. | 12:06.30 |
| sebras | tor8: still around? | 12:22.25 |
| | tor8: do we want to avoid doing null checks before calling fz_free() {fz,pdf}_drop_*()? | 12:23.16 |
| | tor8: and if the answer is no, then enlighten me as to when it makes sense, and when it does not. :) | 12:24.36 |
| | fz_drop_xml() is an example. | 12:24.56 |
| tor8 | sebras: do you mean in the loop of fz_drop_xml? | 12:25.55 |
| sebras | tor8: no, the if (item->text) fz_free(ctx, item->text) | 12:26.15 |
| | tor8: why the if? fz_free() ought to handle NULL perfectly fine. | 12:26.26 |
| tor8 | "premature" optimization | 12:26.30 |
| | saving a function call just to have it immediately return | 12:26.40 |
| sebras | tor8: right, so when do we do those? | 12:26.43 |
| tor8 | in loops, and hopefully nowhere else. | 12:26.56 |
| sebras | tor8: fz_decomp_image_from_stream() in fz_catch()...? | 12:27.38 |
| | same with fz_new_pixmap_with_data() and fz_catch(). | 12:28.02 |
| tor8 | those can all go away, if you want to clean them up | 12:28.28 |
| | remnants from before fz_drop_xxx could always handle NULL safely | 12:28.40 |
| sebras | tor8: right. | 12:28.45 |
| | tor8: but then there is no corresponding check for item->down in fz_drop_xml(). | 12:30.23 |
| tor8 | the checks in fz_drop_xml are optimizations. probably premature, and you can remove them. | 12:32.13 |
| sebras | tor8: ok, let me provide a patch and you can vet them..? | 12:32.37 |
| tor8 | sure | 12:32.46 |
| Robin_Watts | sebras: public API destructors should always be able to handle NULL. | 12:36.38 |
| | we can drop that in internal functions if we feel like it (like the _drop_foo_imp ones), but for some internal destructors it still makes sense because it makes the cleanup code so much easier. | 12:37.28 |
| tor8 | they must handle NULL and may never throw exceptions | 12:37.32 |
| sebras | tor8: did you see me asking whether the same holds for close-functions? | 12:38.07 |
| tor8 | Robin_Watts: drop_subclass_imp functions should only do internal cleanup and not free the actual object allocation | 12:38.08 |
| Robin_Watts | tor8: yeah. | 12:38.16 |
| tor8 | close functions may throw exceptions | 12:38.22 |
| Robin_Watts | tor8: indeed. | 12:38.23 |
| sebras | ok, then we are all on the same page. :) | 12:38.37 |
| tor8 | that's (one reason) why I separated close from drop | 12:38.39 |
| sebras | tor8: so if drop calls close we need to enclose it in fz_try(). | 12:38.59 |
| tor8 | drop should not call close, it should warn that it's dropping unclosed stuff instead IMO | 12:39.16 |
| Robin_Watts | kens: It may be a wacky file, but we still shouldn't crash on it. (but thanks for looking) | 12:39.21 |
| kens | Yeah my old version of mudraw doesn't crash, but if I don't set the resolution low enough it fails to malloc | 12:39.48 |
| Robin_Watts | tor8: I agree that drop should not call close. I'm not sure it should warn. | 12:39.49 |
| kens | I was actually looking to check GS :) | 12:39.57 |
| tor8 | not calling close is an API user error, which we should warn about not silently patch up | 12:40.14 |
| Robin_Watts | Maybe in debug builds only. | 12:40.22 |
| tor8 | Robin_Watts: asserting would be worse :) | 12:40.26 |
| Robin_Watts | but I agree that we shouldn't silently patch up. | 12:40.36 |
| | asserting would be wrong, indeed. | 12:40.42 |
| | Having a broken file (or running out of memory) are "exceptional" cases, but are still "acceptable" cases. Hence asserting would be bad. | 12:41.24 |
| | asserting is for "impossible" cases :) | 12:41.31 |
| sebras | tor8: so fz_stext_close_device() may close because add_span_to_soup() does allocations. | 12:42.19 |
| | tor8: but display_list_image_get_pixmap() doesn't expect fz_close_device() to throw. | 12:42.31 |
| | tor8: then it is display_list_image_get_pixmap() which is at fault? | 12:42.42 |
| tor8 | display_list_image_get_pixmap leaks on errors, yes | 12:43.26 |
| | regardless of whether close throws or not; run_display_list may throw as well | 12:44.09 |
| Robin_Watts | I'm going to run through the bugs I mentioned earlier, and leave sebras to fix those leaks, ok? If anyone else starts to work on one of those bugs, please tell me so we don't conflict :) | 12:45.41 |
| sebras | Robin_Watts: which leaks? just so we don't forget something. | 12:46.25 |
| | Robin_Watts: the ones I just mentioned? | 12:46.37 |
| Robin_Watts | sebras: The ones you are discussing with tor. | 12:46.39 |
| sebras | Robin_Watts: np. | 12:46.45 |
| Robin_Watts | urgh. | 13:02.44 |
| | fz_write_png_trailer is effectively a destructor, but it can also throw :( | 13:02.59 |
| sebras | tor8: ehm... neither you nor I am smart enough. | 13:04.22 |
| | tor8: 1 patch at sebras/master. | 13:04.55 |
| Robin_Watts | So either I need to stop fz_write_png_trailer throwing, or I need to split it into 2 functions. One to write the trailer, and one to drop the png writing context. | 13:06.50 |
| sebras | Robin_Watts: it can only throw because of deflateEnd returning error right? | 13:07.25 |
| Robin_Watts | the former is tough. | 13:07.25 |
| | deflateEnd can flush, I think. so we get called in a callback to actually do the write and that might throw. | 13:08.01 |
| sebras | Robin_Watts: if deflateEnd() returns != Z_OK we also throw. | 13:08.35 |
| | Robin_Watts: if the callback throws then we need to have a corresponding catch somewhere. | 13:08.57 |
| | or try. | 13:09.04 |
| | Robin_Watts: also putchunk() at the end can throw. | 13:09.22 |
| Robin_Watts | No, wait, there may be no callback. | 13:09.33 |
| | It may just be putchunk. | 13:09.53 |
| sebras | Robin_Watts: why are we calling fz_write_png_Trailer() in fz_always()? | 13:10.44 |
| Robin_Watts | If putchunk fails we *want* to throw, cos the file will be incomplete and that needs to be signalled somehow. | 13:11.03 |
| sebras | Robin_Watts: if fz_write_png_band() throws, e.g. what does it help that we write an extra chunk to the file? | 13:11.06 |
| Robin_Watts | fz_write_png_trailer destructs poc. | 13:11.11 |
| | hence me thinking we should have fz_write_png_trailer and fz_drop_png_output_context. | 13:11.31 |
| sebras | Robin_Watts: yes please. :) | 13:11.54 |
| Robin_Watts | will fix. | 13:12.08 |
| tor8 | fz_write_png_init/fin? | 13:12.37 |
| sebras | why does fz_drop_document_handler_context() check both ctx and ctx->handler? | 13:13.19 |
| | other functions like fz_drop_colorspace_context() only check ctx while fz_drop_aa_context() checks neither. | 13:14.05 |
| tor8 | that's a question for Robin_Watts | 13:14.21 |
| sebras | the check for ctx makes more sense than the check for ctx->handler as that ought to be ok given how fz_drop_imp() is implemented. | 13:15.05 |
| Robin_Watts | tor8: typically we get an fz_png_output_context produced by calling a function like fz_write_pixmap_as_png. | 13:15.38 |
| | I *could* split that into an _init call, but it seems like more work for the caller. | 13:15.57 |
| | I'll need to do the same fix for ps_output_contexts and pcl_output_contexts. | 13:16.25 |
| tor8 | Robin_Watts: hm, have some common piecemeal image writer API? | 13:17.22 |
| Robin_Watts | sebras: probably fz_drop_aa_context *should* check ctx, and ctx->aa | 13:17.38 |
| | I'm thinking of the case where we get a memory failure after we init ctx, but before (say) ctx->aa is inited. | 13:18.06 |
| | (i.e. one of the other context inits fails) | 13:18.14 |
| sebras | Robin_Watts: but we don't need to check for ctx->aa in that case. | 13:18.27 |
| | Robin_Watts: right? | 13:18.30 |
| Robin_Watts | then we'd just want to call destroy_context (or whatever) and have it free all of the others, some of which may not have been inited. | 13:18.42 |
| sebras | Robin_Watts: and correspondingly no need to check for ctx->hanbdler in fz_drop_document_handler_context(). | 13:18.51 |
| | also this is kind of a special case and not a loop... ;) | 13:19.22 |
| Robin_Watts | sebras: If fz_drop_document_handler_context is going to do stuff like: fz_free(ctx->handler->blah[i]), then we need to check that ctx->handler is not NULL. | 13:19.39 |
| sebras | Robin_Watts: I must be missing something, why is checking dev not necessary in fz_drop_device()? | 13:20.41 |
| | Robin_Watts: I can see you argument that we _must_ check ctx. | 13:20.55 |
| Robin_Watts | fz_drop_imp copes with dev being NULL :) | 13:21.17 |
| sebras | Robin_Watts: why doesn't it cope with ctx->handler being null? | 13:21.30 |
| Robin_Watts | oh, it does. | 13:21.57 |
| sebras | Robin_Watts: ok, then I'm not crazy! :) | 13:22.08 |
| Robin_Watts | hence the *explicit* check for ctx->handler is not required. | 13:22.09 |
| | sorry, I didn't have the code in front of me at the time. | 13:22.18 |
| sebras | Robin_Watts: np. | 13:22.21 |
| | Robin_Watts: sorry for being a bit stubborn. I just don't want to mess upp. | 13:22.48 |
| Robin_Watts | (It might be argued that it's clearer to check it explicitly cos we don't need to know that fz_drop_imp copes with NULL. | 13:22.52 |
| sebras | tor8: did you see the patch at sebras/master? | 13:22.56 |
| Robin_Watts | sebras: Thorough, not stubborn. | 13:22.58 |
| sebras | Robin_Watts: different words for the same trait. | 13:23.13 |
| Robin_Watts | sebras: No, stubborn implies not being prepared to change your mind if it turns out there are good reasons. | 13:23.41 |
| kens | No,no its an irregular verb, I'm thorough, you are stubborn | 13:23.43 |
| | irregular adjective* | 13:23.57 |
| Robin_Watts | kens: Yes, Prime Minister. | 13:24.11 |
| tor8 | sebras: I'd say it's two different expressions of the same trait. If even that. | 13:25.07 |
| Robin_Watts | tor8: so, sorry, you were suggesting a generalised output API? | 13:25.43 |
| | "common piecemeal image writer API" | 13:26.02 |
| tor8 | yeah, something for writing images band by band | 13:26.07 |
| | fz_new_image_writer, fz_write_image_band, fz_close_image_writer, fz_drop_image_writer | 13:26.28 |
| Robin_Watts | fz_open_band_writer(ctx, bandsize, "png"); | 13:26.29 |
| tor8 | and subclasses for png, ps, pcl, etc | 13:26.37 |
| Robin_Watts | That's doable. | 13:26.48 |
| tor8 | and maybe even a magic constructor like that fz_open_band_writer | 13:26.49 |
| | which switches on file extension | 13:26.55 |
| | it seems to be a common pattern, now that we have at least three subclasses of it | 13:27.25 |
| Robin_Watts | tor8: Yeah. | 13:27.31 |
| | If I'm changing the API now (as I have to to fix this leak), this seems like a good time to do that. | 13:27.53 |
| | I'll have a crack after lunch. | 13:27.59 |
| sebras | Robin_Watts: do you mind reviewing sebras/master? | 17:22.45 |
| Robin_Watts | sure. | 17:22.52 |
| sebras | Robin_Watts: I made a mistake before and neither me nor tor8 caught it. | 17:23.05 |
| | Robin_Watts: I asked for review but I guess it got lost between the lines. | 17:23.18 |
| Robin_Watts | sorry, I missed that. | 17:23.50 |
| sebras | Robin_Watts: I asked tor8. ;) | 17:24.05 |
| Robin_Watts | I see 2 commits there, both look reasonable to me. | 17:24.18 |
| sebras | Robin_Watts: two?! there should only be 31a544f. | 17:24.53 |
| | Robin_Watts: unfortunately I got an LGTM on 28d9c75 and pushed it, but after the fact discovered that it is broken. | 17:25.13 |
| Robin_Watts | Ah, my golden isn't up to date. | 17:25.18 |
| sebras | Robin_Watts: both patches are needed. | 17:25.20 |
| | ideally combined. | 17:25.27 |
| Robin_Watts | right. lgtm then. | 17:25.31 |
| | too late to combine now. | 17:25.34 |
| sebras | what's the threshold? | 17:25.49 |
| | (also I don't have permissions) | 17:25.55 |
| | ((and I shouldn't have)) | 17:26.03 |
| Robin_Watts | sebras: 2 mins or so, or if it's a REALLY heinous mistake :) | 17:29.11 |
| | (like something that kills the cluster) | 17:29.36 |
| sebras | ok. | 17:29.46 |
| | this just caused a few leaks, and will be fixed now, so no worries. | 17:30.15 |
| | Robin_Watts: care to re-review? I forgot to &..->super so I got compiler warnings. | 17:50.42 |
| Robin_Watts | fz_drop_document(ctx, (fz_document*)doc); in gproof ? | 17:51.33 |
| | should that be &doc->super too, for consistency? | 17:51.54 |
| | similarly in epub-doc.c | 17:52.05 |
| | lgtm other than that though. | 17:52.23 |
| sebras | Robin_Watts: fixed. final LGTM? | 17:53.30 |
| | Robin_Watts: hm... maybe in pdf_drop_graft_map() too? | 17:55.30 |
| Robin_Watts | not now. | 17:56.55 |
| | It doesn't relate to your current changes, and it *might* conflict for Michael, so I'd leave it. | 17:57.17 |
| sebras | Robin_Watts: ok. | 17:57.30 |
| Robin_Watts | so lgtm. | 17:57.30 |
| sebras | Robin_Watts: thanks. off to bed. | 17:58.17 |
| Robin_Watts | night! | 18:00.19 |
| | tor8, sebras: For the logs: A first version of the band_writer stuff is on robin/master. | 20:17.11 |
| | Having pondered it, I dislike the "image_writer" name. | 20:17.30 |
| | We have an entirely understandable 'fz_image' type already. To have an 'image_writer' that doesn't actually have anything to do with images seems bad to me. | 20:18.15 |
| | I much prefer the fz_bander name. | 20:18.31 |
| | It doesn't come close to conflicting with any other names, and it's pithier than fz_band_writer. | 20:19.01 |
| | Forward 1 day (to 2016/10/12)>>> | |
Log of #ghostscript at irc.freenode.net.