| | <<<Back 1 day (to 2016/10/11) | 20161012 |
| tor8 | Robin_Watts: in fz_write_header and fz_write_band etc you check ctx for NULL ... we shouldn't ever be calling these with a NULL context should we? | 09:29.17 |
| | in the prototype, band_start and bandheight ... ought to be band_height? | 09:30.44 |
| | ugh, 'rem' and 'ch' and 'vw' etc are all CSS 3 features, not part of the EPUB spec (not that a spec has ever prevented anybody from using a feature...) | 09:31.56 |
| | css hex colors without the # prefix? what's the world coming to... | 09:35.45 |
| | that code could reuse the CSS_HASH code above it using a goto or rearranging the if statements | 09:38.33 |
| | wouldn't 1/2 em be closer to the width of '0' than 2/3? | 09:40.20 |
| | and for 'rem', x * 16, N_LENGTH would probably be a better approximation | 09:43.41 |
| | or x * 12 | 09:43.45 |
| Robin_Watts | tor8: We should not, ever be calling with a NULL context. That's why I check that. | 09:44.49 |
| tor8 | why check it? we don't check NULL in every other function | 09:45.10 |
| Robin_Watts | We check it in all API functions I think. | 09:45.24 |
| tor8 | we don't check ctx for NULL everywhere else | 09:45.26 |
| | no, only in the JNI code | 09:45.33 |
| | because that may fail initialization and should never crash the JVM | 09:45.44 |
| | % is already handled above in the CSS_PERCENT block | 09:47.10 |
| | so that comment is misleading | 09:47.18 |
| Robin_Watts | We check for !ctx in all take/drops, and in some other places, but not all. | 09:48.16 |
| sebras | A question about the band writer. Compared to fz_document there is no super. If that was used then most of the function pointer types could be internal to the band writer type instead of also being present in the New function. Is one approach better or worse than another? Why? | 09:49.50 |
| Robin_Watts | sebras: Urm? | 09:50.18 |
| | There is a super. | 09:50.21 |
| | The fz_new_band_writer function allocates enough space for the derived class, and fills in the super class. | 09:51.12 |
| | hence the function pointers are only ever stored in the super class. | 09:51.35 |
| tor8 | Robin_Watts: huh. that prompted me to grep the code. it does look like we check ctx == NULL in a few odd places | 09:51.39 |
| | I expect the check in context.c when setting up contexts | 09:51.48 |
| | I did *not* expect fz_open_document to check ctx and fail silently in the absence of a ctx | 09:52.06 |
| | I totally expect that to crash with a big fat segfault | 09:52.17 |
| | don't pass null to things that must not be null.... | 09:52.33 |
| sebras | Robin_Watts: I'm thinking of e.g. epub setting its own variants of the fz_doc function pointers. | 09:52.57 |
| Robin_Watts | What I've done in the band_writer is akin to having fz_new_document_of_size take a load of function pointers too. | 09:53.57 |
| tor8 | Robin_Watts: and now I see that you've been adding such tests ever since I got back from vacation... :) | 09:54.43 |
| Robin_Watts | sebras: I could remove them as params, and explicitly call writer->super.header = ... and it would be the same. | 09:55.12 |
| | tor8: I could live with not checking ctx, if that's a policy decision we take. | 09:56.18 |
| | (never check an error condition you can't handle, etc) | 09:56.31 |
| | Let me massage those commits a bit then. | 09:57.25 |
| | Another new one on there for your consideration. | 09:57.32 |
| | tor8: Do you have a preference for/against passing function pointers to fz_new_band_writer ? | 09:58.16 |
| tor8 | Robin_Watts: yeah, I feel strongly about silent failures. and I don't think adding an assert(ctx != NULL) (which is the way we should be checking these errors if at all) | 09:59.15 |
| | is worth the effort | 09:59.24 |
| | Robin_Watts: I'll go clean up the superfluous ctx null checks that have made their way in | 10:00.14 |
| | Robin_Watts: elsewhere we init and fill out the pointers in the struct after creation | 10:00.43 |
| sebras | Robin_Watts: I'm thinking that if the function pointers are only ever referenced as types inside the band writer type then we could lose the typedefs.I don't feel too strongly about it though. :) | 10:01.00 |
| tor8 | mostly because passing half a dozen or so function pointers can get ugly with the actual order of arguments | 10:01.08 |
| | but I have no strong preference | 10:01.14 |
| sebras | tor8: I think we use func pointer arguments for the filters. | 10:01.47 |
| tor8 | sebras: I've given up the fight against typedefing function pointers... | 10:01.48 |
| sebras | tor8: oh | 10:01.59 |
| | I wasn't adware. | 10:02.15 |
| | Sigh... On mobile.. | 10:02.27 |
| tor8 | sebras: robin likes them; and I think I'm the only one comfortable with raw function pointer syntax :) | 10:02.43 |
| | sebras: we might. I could just be thinking of the "classes" where we have dozens of functions and many of them are optional | 10:03.04 |
| sebras | tor8: no, you've trained me well. ;) | 10:03.07 |
| tor8 | Robin_Watts: fz_add_separation, fz_control_separation, should those really be okay with NULL sep arguments? | 10:03.40 |
| Robin_Watts | No, probably not. | 10:04.38 |
| | avoiding raw function pointer syntax pays off in 2 big situations. | 10:06.05 |
| | Firstly, if you use the same function pointer type in more than 1 place, it's MUCH nicer to use a type. | 10:06.28 |
| | Secondly, it's MUCH easier to write documentation when you have it as a type. | 10:06.52 |
| | I appreciate that the second one has never been a problem for tor in the past >8*) | 10:07.06 |
| tor8 | I'll grudgingly accept the first argument. The second argument is spot on, but irrelevant :) | 10:07.23 |
| jogux | :-) | 10:09.41 |
| sebras | :) | 10:10.12 |
| tor8 | Robin_Watts: fixes on tor/master (and some ideas on how to fix the css stuff a bit simpler) | 10:27.11 |
| Robin_Watts | Ideas for doing the css more simply would be good. | 10:27.39 |
| | I was thinking that to do it properly we probably need to pass in an "environment" to the evaluation functions (that contains the root element size, the browser window size etc). | 10:28.35 |
| tor8 | fixing 'rem' properly would mean adding a N_ROOT_SCALE enum and passing a 'rem' around to all the CSS functions. ugh. | 10:28.47 |
| | not a serious issue, we already pass in the browser window size (or old font size, or whatever the % scale is supposed to affect) | 10:29.39 |
| | but considering that 'rem' isn't part of the EPUB spec to begin with... not sure how much I care :) | 10:30.18 |
| Robin_Watts | tor8, right, but passing float *rem to all the new functions... I can't help feeling it'd be easier to pass css_env *environment around. | 10:30.53 |
| tor8 | but yeah, to support vw and vh and vmin we'd need to pass a bit more than just those | 10:31.01 |
| Robin_Watts | cos we're already having to pass scale and.... whatever the other one was too. | 10:31.08 |
| tor8 | a fz_css_env with 'rem', 'vw', 'vh', etc would be a better solution than adding more parameters ad infinitum | 10:31.36 |
| Robin_Watts | and ren/vw/vh/vmin/vmax etc don't change often (if at all. | 10:31.40 |
| tor8 | the rem/vw/vh/vmin/vmax would never change | 10:31.53 |
| | the scale depends on where the value is used though, so that should be a separate argument | 10:32.08 |
| Robin_Watts | if (ctx, idoc == NULL) ?! | 10:34.21 |
| tor8 | no idea where that came from! | 10:34.28 |
| | but I figured I'd fix it... | 10:34.41 |
| | even though it's the old viewer | 10:35.10 |
| Robin_Watts | ooh, you've gone further than I would have. | 10:36.35 |
| | fz_count_separations on page. | 10:36.42 |
| | coping with a null page may make sense. | 10:36.51 |
| | I agree that we can drop the ctx test, but we should continue to check page == NULL and page->count_separations == NULL, IMHO. | 10:37.44 |
| | I'd be tempted to keep the fz_{add,control}_separation test too. | 10:39.06 |
| | Continuing to run with things being NULL after errors is a fairly reasonable use case for everything except the context. | 10:39.57 |
| | If we can do 'sane' things and not crash on an exit path, that's good. | 10:40.08 |
| | I'm happy to drop my 2 commits and defer to yours for the css stuff. | 10:41.07 |
| tor8 | Robin_Watts: like the commit on the tip of tor/master? | 10:45.31 |
| | not sure I like that, but I can live with it I guess | 10:45.40 |
| | not crashing on the exit path is why we cope with NULL in fz_keep_xxx | 10:45.58 |
| Robin_Watts | Updated commits on robin/master | 10:46.01 |
| | let me look. | 10:46.04 |
| | tor8: Yeah, I like that more. | 10:46.58 |
| tor8 | I'll squash that in then | 10:47.48 |
| | I just changed the page == NULL || page->count_separations == NULL to be a positive test, did not remove it | 10:48.08 |
| | if (page && page->count_separations) page->count_separations(); | 10:48.18 |
| Robin_Watts | ok. | 10:48.41 |
| | reboot. | 10:48.44 |
| tor8 | Robin_Watts: "\t-B -\tminimum band_height (e.g. 32)\n" ought to say "band height" since it's human readable text | 10:55.51 |
| Robin_Watts | ooh, whoops. | 10:56.28 |
| tor8 | the const char **tailp thing in strtof doesn't work. const in C is just broken, and this is one of those weird cases. | 10:57.20 |
| Robin_Watts | tor8: does it not? | 10:58.03 |
| tor8 | it requires that the caller pointer variable be const as well | 10:58.33 |
| | your commit causes lots of warnings | 10:58.55 |
| | http://stackoverflow.com/questions/3874196/why-is-the-endptr-parameter-to-strtof-and-strtod-a-pointer-to-a-non-const-char-p#3874281 | 10:59.31 |
| Robin_Watts | ok, will fix. | 11:00.50 |
| tor8 | I wonder if we could just call fz_strtof() in xps_parse_real_num and not bother with the fz_safe_strtof function | 11:01.26 |
| Robin_Watts | I rather like the suggestion in that stackoverflow answer. | 11:02.18 |
| | That we have a wrapper function that uses size_t. | 11:02.48 |
| | tor8: I chose fz_safe_strtof cos I was sure it wouldn't change behaviour. | 11:03.19 |
| tor8 | yeah. but we're doing 'safe' versions of the standard libc. I think there's a benefit in staying with those APIs even if they're suboptimal | 11:03.20 |
| | changing behaviour here would be fine | 11:04.09 |
| | garbage in, garbage out, and for glyph offsets it's fine, IMO | 11:04.34 |
| | nothing is going to break by allowing a NaN to slip through here | 11:04.48 |
| | (famous last words...) | 11:05.13 |
| Robin_Watts | ok, will simplify that commit to just call fz_strtof. | 11:08.02 |
| tor8 | cool. I've squashed the null fix, and reworded the css commits on tor/master | 11:08.33 |
| Robin_Watts | tor8: All the commits on tor/master look good to me, except the last one. | 11:11.21 |
| | the last one being the consty buffer overflow thing. | 11:11.34 |
| | new commits on robin/master, clustering now. | 11:12.03 |
| | bah. the xps fix doesn't. | 11:40.01 |
| | But everything else on robin/master up to that point is good to go. | 11:40.29 |
| tor8 | Robin_Watts: the four commits up to 'fix mutool into -I flag'? fix the typo (info, not into) and those LGTM | 11:50.27 |
| Robin_Watts | tor8: Thanks. | 11:50.59 |
| | Fixed version of the xps fix online. clustering now. | 11:56.15 |
| | Yeah, that's solved it. | 12:02.00 |
| | If we get all these fixes in, I think we should be nearing release. | 12:03.56 |
| | bug 697151 is still missing a fix. sebras, you here? | 12:04.37 |
| tor8 | Robin_Watts: ah, yes. the xps fix LGTM now. | 12:08.00 |
| Robin_Watts | tor8: Thanks. | 12:08.29 |
| sebras | Robin_Watts: I'm here. | 12:08.38 |
| tor8 | Robin_Watts: I am working on a fz_css pool allocation fix that should prevent the css code from leaking memory on errors | 12:08.57 |
| | it will change the API a bit, so maybe we should try to sneak that in before the release | 12:09.20 |
| Robin_Watts | sebras: I'm about to redo the fix for bug 697151 fix based on my memory of yours, unless you have the patch to hand? | 12:09.20 |
| | tor8: OK. | 12:09.27 |
| sebras | Robin_Watts: I have it here. | 12:09.37 |
| | Robin_Watts: fetch sebras/wip and see cfa3cc3 | 12:11.15 |
| Robin_Watts | Thanks. | 12:11.35 |
| sebras | Robin_Watts: sorry for not fixing it. :-( | 12:13.01 |
| Robin_Watts | sebras: no problem, you've done 99% of the work! | 12:13.16 |
| | sebras: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=d6ed5d4f399aa2824e4510237b3576a5d414ced0 | 12:24.31 |
| sebras | reviews | 12:25.12 |
| | Robin_Watts: ok. I remeber asking when PACIFY_VALGRIND should be enabled. for debug builds? | 12:26.58 |
| | Robin_Watts: the patch itself LGTM. | 12:27.14 |
| Robin_Watts | sebras: With gs, it's not enabled for debug builds. It's only enabled specifically when we XCFLAGS="-DPACIFY_VALGRIND" | 12:27.42 |
| | tor8: I'm going to drag the bug 697012 (xps overflow) fix back to gs while I remember. | 12:29.34 |
| tor8 | Robin_Watts: go for ti | 12:30.49 |
| Robin_Watts | I note that the code isn't identical between gxps and muxps. Should I attempt to make it so? | 12:32.52 |
| tor8 | what's different? | 12:36.51 |
| Robin_Watts | In MuPDF xps_parse_glyph_metrics reads 3 components. | 12:48.03 |
| | In gxps xps_parse_glyph_offsets does the latter. | 12:48.51 |
| | the latter 2. | 12:48.56 |
| tor8 | Robin_Watts: huh. it's the same functionality, but I guess henry split the function in two when implementing the gxps fix and I didn't bother when porting it over | 12:51.06 |
| Robin_Watts | yeah. | 12:53.24 |
| | it's no big deal, but I would have thought that for sanity in future fixes we wanted to avoid such splits. | 12:53.45 |
| tor8 | do what you feel is best | 12:54.02 |
| | I have no strong opinion, but in general, keeping them in sync is for the better, yes | 12:54.19 |
| Robin_Watts | oh, ffs Microsoft. | 12:55.27 |
| | VS doesn't implement strtof. | 12:55.35 |
| | (until vs2013) | 12:55.45 |
| tor8 | that's why we have macros and fz_strtof innit? | 12:56.17 |
| | oh right, gxps. nevermind :) | 12:56.25 |
| | strtod will do in a pinch | 12:56.34 |
| | gs isn't as obsessed with keeping all floats single precision as fitz is | 12:56.49 |
| | Robin_Watts: two more commits on tor/master when you got ime | 12:58.24 |
| | time | 12:58.26 |
| Robin_Watts | Ah, strtod will do nicely. Thanks. | 12:58.40 |
| | fz_malloc_struct clears memory, fz_pool_alloc doesn't. | 13:02.02 |
| | Doesn't matter as long as we fully populate the things that are returned. | 13:02.32 |
| | fz_new_css should possibly have a css->next = NULL; ? | 13:04.29 |
| | Other than that, looks good to me. | 13:11.39 |
| | Given that css_values are now never freed (other than by their pool going away), we can now safely use static ones, right? | 13:12.34 |
| | Oh, no, we still have 'next' to worry about. | 13:13.08 |
| | And the last commit lgtm. | 13:13.32 |
| tor8 | fz_pool_alloc returns memory that has already been cleared by fz_malloc_struct | 13:31.45 |
| | fz_new_css has no next pointer; the diff is confusing so maybe you're misreading that? | 13:32.14 |
| sebras | hm.. fz_convert_pixmap() is one of those odd functions that cannot throw, right? | 13:39.27 |
| | hm.. in the standard case we create a hash table in fz_std_conv_pixmap(), so I guess not. | 13:41.48 |
| Robin_Watts | tor8: Yes, I misread the diff as having it return a fz_css_rule, sorry. | 14:04.59 |
| | and I see what you mean about fz_pool_alloc. | 14:06.00 |
| | So, all lgtm then. | 14:06.09 |
| tor8 | Robin_Watts: thanks. | 14:08.43 |
| Robin_Watts | source/html/css-apply.c:629:26: warning: variable 'head' set but not used [-Wunused-but-set-variable] | 14:16.00 |
| tor8 | Robin_Watts: oh crap. will fix. | 14:16.48 |
| | fix (and another minor fix) on tor/master | 14:18.01 |
| Robin_Watts | both lgtm. | 14:18.52 |
| tor8 | Robin_Watts: bug 697012. I'm having trouble understanding "part 2". | 14:18.58 |
| | Does he mean to optimize to not look up the metrics unless required? | 14:19.11 |
| Robin_Watts | tor8: Me too. That's why I left that for you. | 14:19.16 |
| tor8 | if that's all, then I'd just close it and not bother | 14:21.12 |
| | but maybe we should let henry weigh in first, in case there's something I've misunderstood | 14:21.52 |
| Robin_Watts | tor8: bug 697068: You say you have a fix in progress in the comments. | 14:23.31 |
| sebras | Robin_Watts: in bug 697151 how to you get bugzilla to reference the commit like that? | 14:24.54 |
| Robin_Watts | any sha is automatically rewritten by a crufty bugzilla hack into a link. | 14:30.58 |
| | unfortunately it's always assumed to be a gs commit :) | 14:31.12 |
| | actually, it goes through a script to resolve it now, but it ain't working for me. sigh. | 14:32.00 |
| | sebras: oh, ahem, I haven't pushed it yet :) | 14:35.19 |
| | Did you review it? | 14:35.22 |
| sebras | Robin_Watts: I did. | 14:35.36 |
| | Robin_Watts: I asked about PACIFY_VALGRIND vs build=debug | 14:36.08 |
| Robin_Watts | and I replied, and the conversation stopped. | 14:36.37 |
| sebras | Robin_Watts: you said PACIFY_VALGRIND would need to be set manually and that this was preferable (I'm not sure I agree, but I can set XCFLAGS in my makefile or something. :) | 14:36.41 |
| | Robin_Watts: I'd prefer if build=debug would set PACIFY_VALGRIND. | 14:37.14 |
| | Robin_Watts: the cluster builds with build=release? | 14:38.34 |
| Robin_Watts | yes (I think) | 14:38.45 |
| | yes, just checked. | 14:39.43 |
| sebras | right, so neither users nor the cluster will run with this thing. so if it is enabled by default in debug builds it hits us devs. | 14:41.07 |
| | and barely noticably so. given how often I run valgrind I'd prefer it to be enabled. otherwise I'd forget it. | 14:41.32 |
| | do other projects use XCFLAGS? i.e. is that a common pattern? | 14:42.00 |
| Robin_Watts | sebras: the use of XCFLAGS is because of the way gs makefiles work. | 14:43.16 |
| | I think we consider it to be 'not ideal, but too hard to change now'. | 14:43.31 |
| sebras | Robin_Watts: yes, and mupdf has something similar | 14:43.32 |
| | Robin_Watts: the question is wheter I can safely set XCFLAGS in my .bashrc and the compile random projects without that environment variable messing with my environment | 14:44.06 |
| | Robin_Watts: messing with my compilations. | 14:44.16 |
| tor8 | Robin_Watts: oh dear... I can't remember :/ | 14:47.07 |
| | that commit may have made it onto golden, but I have no recollection | 14:47.36 |
| Robin_Watts | No commit messages that refer to 697058 in golden. | 14:49.25 |
| tor8 | Robin_Watts: maybe commit b1a4f1a? | 14:50.30 |
| | the date matches, and the comment seems to be relevant | 14:51.06 |
| Robin_Watts | I shall leave it to you then :) | 14:53.02 |
| | gonna have a quick look at 697065 | 14:53.21 |
| | Stupid mistake. | 15:28.59 |
| | sebras: Looks like I've pushed a commit to golden that I didn't mean to. | 15:31.23 |
| | http://git.ghostscript.com/?p=mupdf.git;a=commitdiff;h=f9bab4a5e0caee3fcc1f8ab9d809ab23be1acf96 | 15:31.45 |
| | Did you have a "better fix" for that in the pipeline ? | 15:31.54 |
| sebras | Robin_Watts: I do, but no worries. | 15:33.48 |
| Robin_Watts | sorry. | 15:33.57 |
| sebras | Robin_Watts: this papers over the most blatant issue while you wait. | 15:34.00 |
| Robin_Watts | Is the full fix going to be in for this release? | 15:34.35 |
| sebras | Robin_Watts: I hope so. | 15:35.39 |
| | I keep getting side tracked. :-( | 15:36.12 |
| Robin_Watts | tor8, sebras: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=d05713020792a557d9ca147bf5f995c453c33f12 | 15:43.34 |
| | Silly mistake. | 15:43.39 |
| | bmpcmping now. | 15:44.03 |
| sebras | Robin_Watts: the bug number seems to be wrong. | 15:44.57 |
| Robin_Watts | oops. fixed. | 15:47.44 |
| sebras | Robin_Watts: do you mind moving the assignments fo CMYK to the top in the other cases too? | 15:48.12 |
| | Robin_Watts: just so it is more obivous for the next person reading it. :) | 15:48.27 |
| | Robin_Watts: it took me until now to understand what you did. :) | 15:48.34 |
| Robin_Watts | sebras: Sure. | 15:48.52 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=ded36857c6ac60808909454a06e021fafe151f41 | 15:51.54 |
| sebras | Robin_Watts: LGTM. | 15:52.45 |
| Robin_Watts | Thanks. I'm just doing a new bmpcmp (to get a more reasonable set of files to check) then will push. | 15:53.08 |
| | sebras: ping? (better group) | 17:14.59 |
| sebras | Robin_Watts: pong. | 17:15.14 |
| Robin_Watts | I just had a quick look at http://bugs.ghostscript.com/show_bug.cgi?id=696885 | 17:15.31 |
| | I now get http://ghostscript.com/~robin/out.png | 17:15.46 |
| | Annoying close but different to http://bugs.ghostscript.com/attachment.cgi?id=12659&action=edit | 17:16.20 |
| | Annoying*ly*, sorry. | 17:16.35 |
| | Is it possible that the bug attachment is wrong? | 17:16.53 |
| sebras | Robin_Watts: I started working on it as well, see 0001-Bug-696884-Handle-subsampled-components-in-JPX-image.patch on casper. | 17:18.18 |
| | Robin_Watts: but it gives me really weird stepped stair thingy in gs | 17:18.35 |
| Robin_Watts | sebras: in gs ? | 17:18.45 |
| sebras | Robin_Watts: yes. | 17:18.49 |
| Robin_Watts | I'm doing this in mupdf. | 17:18.56 |
| sebras | Robin_Watts: the patch is for gs | 17:18.56 |
| Robin_Watts | I *had* the stepped stair thing here. | 17:19.10 |
| | then I fixed my code. | 17:19.16 |
| | and was all smug until I checked the expected results, and now I'm utterly stumped. | 17:19.34 |
| sebras | Robin_Watts: oh! wait. | 17:19.35 |
| | Robin_Watts: so I'm using the luratech decoder locally. | 17:19.46 |
| | Robin_Watts: and gs also needs to be fixed. :) | 17:19.57 |
| | Robin_Watts: using luratech in mupdf I do indeed get an image similar to the one which is attached. | 17:20.45 |
| | not what you se. | 17:20.48 |
| Robin_Watts | brb. | 17:20.56 |
| | back. | 17:31.26 |
| | rats. so what am I doing wrong... | 17:31.37 |
| | So... this is a 3 component jpx. | 17:34.14 |
| | and the components for the top left pixel are 4d, 53, f5 | 17:34.37 |
| | ah, I suspect I'm interpreting them as rgb, when they should be SYCC. | 17:35.55 |
| | so I've fixed one bug and run into another. | 17:36.31 |
| sebras | Robin_Watts: the top left pixel according to mupdf shoudl be f2 08 00 in rgb | 17:37.20 |
| | Robin_Watts: and yes I'm doing a jpx_ycc_to_rgb() pass. | 17:37.47 |
| | . | 17:37.47 |
| Robin_Watts | sebras: I'm reading the values from jpx->comps[0,1,2]->data[0] | 17:37.50 |
| sebras | Robin_Watts: I think I run jpx_ycc_to_rgb on the image _after_ decoding. | 17:38.50 |
| | Robin_Watts: maybe we should simply have a ycc colorspace? | 17:39.00 |
| Robin_Watts | sebras: Well, we'd need to implement that somewhere. | 17:39.13 |
| sebras | Robin_Watts: or move this function to fit zsomewhere. | 17:39.15 |
| Robin_Watts | I am tempted to just hoist your jpx_ycc_to_rgb function to be shared between luratech and openjpeg. | 17:39.35 |
| sebras | I already have jpx_ycc_to_rgb() for the luratech case at line 198 in load-jpx.c | 17:39.36 |
| | Robin_Watts: that's a good first step. | 17:39.48 |
| Robin_Watts | jpx_ycc_to_rgb takes an fz_jpxd | 17:40.42 |
| | from which it accesses state->pix and state->width/height. | 17:41.01 |
| | is there a reason it couldn't take state->pix and then use pix->width/height ? | 17:41.14 |
| | oh, state->signs. | 17:41.56 |
| | Never mind, I'll just hack it :) | 17:42.08 |
| sebras | Robin_Watts: signs tells you whether the samples are signed or not. | 17:42.36 |
| | Robin_Watts: does openjpeg tell you? | 17:42.40 |
| Robin_Watts | I've already dealt with that. | 17:42.46 |
| sebras | Robin_Watts: note that the conversion function skips Y | 17:44.50 |
| | Robin_Watts: I'm not sure why that is. | 17:44.54 |
| | Robin_Watts: maybe it can never be signed, I don't know. | 17:45.13 |
| Robin_Watts | The Y U V spaces I've dealt with Y is always unsigned. | 17:45.33 |
| | Y is basically brightness. It ranges from 0 to 1. | 17:45.42 |
| | U and V are 'how far away' the colors are from the greyscale gradient. | 17:46.08 |
| sebras | Robin_Watts: I don't understand why it matters what range of values are used. | 17:46.10 |
| Robin_Watts | I now have a shiny purple mess :( | 17:46.25 |
| sebras | Robin_Watts: ah, that's a good explanation. | 17:46.30 |
| | Robin_Watts: I remember having that! you're on to something! :) | 17:46.44 |
| Robin_Watts | Now I get the results I want. | 17:49.08 |
| | Have you ever seen a file where depth can vary by component? | 17:49.57 |
| | (precision) | 17:50.24 |
| sebras | Robin_Watts: depth == bits per component? | 17:50.26 |
| | Robin_Watts: I'm not sure. | 17:50.57 |
| Robin_Watts | I'll make mupdf crash in that case and run a cluster :) | 17:51.35 |
| sebras | Robin_Watts: yeah, we have the openjpeg stuff in there now, so it ought to be ok. | 17:51.51 |
| | Robin_Watts: png_from_pixmap() drop is always 0 when it is called. | 17:54.19 |
| | Robin_Watts: which means we drop pix2 in fz_always. | 17:54.29 |
| | shouldn't we set pix2 == NULL at the end of the if-statement in fz_try()? | 17:55.02 |
| | Robin_Watts: if e.g. fz_write_pixmap_as_png() throws then we free both in png_from_pixmap() and in fz_new_buffer_from_image_as_png(), right? | 17:55.42 |
| Robin_Watts | We should definitely do pix2 = NULL; after the pix = pix2; | 17:57.06 |
| sebras | Robin_Watts: hm... I don't think that helps. | 17:57.20 |
| Robin_Watts | I think I was agreeing with you. | 17:58.43 |
| sebras | Robin_Watts: png_from_pixmap() drops pix in fz_try. this implies that it thinks it owns pix. | 17:58.55 |
| | Robin_Watts: but fz_new_buffer_from_image_as_png() also drops pix. | 17:59.04 |
| | so who owns it? the caller or the callee? | 17:59.12 |
| | fz_new_buffer_from_pixmap_as_png() apperas to indicate that it is the caller who owns it. | 17:59.24 |
| Robin_Watts | I think maybe it should do: if (drop) fz_drop_pixmap(ctx, pix); | 17:59.24 |
| | oh, wait, I understand. | 17:59.33 |
| | so, the code is fine as it is, maybe. | 17:59.44 |
| sebras | Robin_Watts: why do we have the drop argument at all? | 17:59.45 |
| Robin_Watts | The idea was, I think, that if the 'drop' argument is 1, it means "and drop pix after you've finished with it". | 18:00.20 |
| sebras | Robin_Watts: but we never use that..? | 18:00.37 |
| Robin_Watts | The point of getting this function to do that, was that if we had to do a conversion, we have to make a new pixmap (pix2) and convert into that. That takes a lot of memory. We can then drop pix early, and thus not have to hold 2 pixmaps AND a buffer. | 18:01.27 |
| sebras | Robin_Watts: that makes sense. | 18:01.40 |
| Robin_Watts | But, as you say, it appears that we don't currently use that feature. Presumably I did at some point. | 18:01.45 |
| | (or thought I was going to). | 18:01.56 |
| sebras | Robin_Watts: look at fz_new_buffer_from_image_as_png(). | 18:02.00 |
| | it creates a pix and always drops it. | 18:02.08 |
| | but it also gives it to png_from_pixmap() | 18:02.15 |
| | which might _also_ drop pix in the if-statement. | 18:02.32 |
| Robin_Watts | Ah!, so fz_new_buffer_from_image_as_png should pass 1, and lose the always. | 18:02.36 |
| sebras | (and replace the local pointer with another, newly allocated one and use that to produce the buffer). | 18:02.53 |
| Robin_Watts | sebras: Why do you say "it always drops it"? | 18:03.02 |
| sebras | fz_always(ctx) | 18:03.27 |
| | fz_drop_pixmap(ctx, pix); | 18:03.31 |
| | how can I interpret it otherwise? :) | 18:03.38 |
| Robin_Watts | png_from_pixmap only drops pix if we pass drop=1. | 18:03.39 |
| sebras | no, this is fz_new_buffer_from_image_as_png(). | 18:03.53 |
| | which always drops it. | 18:03.59 |
| Robin_Watts | Aaargh. Jumping about too much. | 18:04.07 |
| | Let's start again. | 18:04.14 |
| sebras | Robin_Watts: ok. | 18:04.16 |
| | Robin_Watts: sorry. :) | 18:04.19 |
| Robin_Watts | Which function do you see the problem with? | 18:04.22 |
| sebras | fz_new_buffer_from_image_as_png() creates pix and always drops it. | 18:04.37 |
| | that is ok. | 18:04.44 |
| | but then the functions it calls may not _also_ drop it. | 18:04.52 |
| Robin_Watts | Indeed. None of the functions it calls should drop it. | 18:05.06 |
| | Do you believe you have a case where those functions ARE dropping it ? | 18:05.16 |
| sebras | yes. | 18:05.41 |
| | png_from_pixmap() is called with drop == 0, which means that it will _not_ drop pix in _its_ fz_always() | 18:06.09 |
| | it _may_ however drop it inside the if-statement inside fz_try(). | 18:06.25 |
| | and that is not allowed. | 18:06.36 |
| Robin_Watts | Urm... You mean in the if (drop) fz_drop_pixmap(ctx, pix) bit ? | 18:07.05 |
| sebras | Robin_Watts: http://git.ghostscript.com/?p=mupdf.git;a=blob;f=source/fitz/output-png.c;h=960e5292dd4cb30a19ee37c16dc0a176cacf96ed;hb=HEAD#l246 | 18:08.15 |
| | line 289 allocates a pix, line 297 drops it. | 18:08.35 |
| | that is fine. | 18:08.39 |
| | this means that the function on line 295 may not drop the pix. | 18:08.49 |
| Robin_Watts | Indeed. Which it will not do, because it is called with drop = 0. | 18:09.02 |
| sebras | but it _is_ passed as an argument! | 18:09.03 |
| | Robin_Watts: oh... | 18:09.21 |
| Robin_Watts | pix is indeed passed to png_from_pixmap | 18:09.28 |
| | as is drop = 0. | 18:09.35 |
| | which means "don't drop pix" | 18:09.42 |
| sebras | Robin_Watts: right, but I have lost the if (drop) statement. | 18:10.29 |
| | and that is crucial. | 18:10.32 |
| | I only have fz_drop_pixmap() locally. | 18:10.48 |
| | so line 265 is gone. | 18:10.56 |
| | I probably messed up in some patch on sebras/wip. darn. :) | 18:11.11 |
| | Robin_Watts: but yes, I do agree that line 295 probably could supply drop == 1 and remove line 297. | 18:11.49 |
| | that way we actually use the function as it was intended. | 18:12.05 |
| | though I have to admit that the logic is easier to understand if we always assume drop == 0 and then let the called drop the pix if it wants to. | 18:12.31 |
| Robin_Watts | logic is easier, but memory use is greater. | 18:14.51 |
| sebras | Robin_Watts: true. | 18:15.04 |
| Robin_Watts | sebras: Ok, so I failed to find any j2k files in the cluster with different precision. | 18:28.09 |
| | I did however find one with 257 components in. | 18:28.18 |
| sebras | mmm, that one is probably from some fuzzing..? | 18:28.44 |
| Robin_Watts | Does luratech render: tests_private/j2k/openjpeg/input/conformance/p0_13.j2k ? | 18:28.51 |
| sebras | Robin_Watts: it doesn't error out. | 18:31.20 |
| | Robin_Watts: but both the old and new the code drop cause overlapping src/dest to memcpy(). | 18:31.38 |
| | according to valgrind. | 18:31.43 |
| Robin_Watts | sebras: Does what it renders look reasonable? | 18:31.52 |
| sebras | hard to say. I get a 1x1 black pixel. | 18:32.41 |
| | but I don't know what it _should_ look like. maybe it is correct..? | 18:33.00 |
| Robin_Watts | can you tell how many components luratech thinks it has? | 18:35.23 |
| sebras | Robin_Watts: 257 channels, out of which 3 are colors, and 0 are alphas (whether premultiplied or not) | 18:43.16 |
| Robin_Watts | ah, ok. | 18:43.42 |
| | I don't quite see why we're erroring on this now when we weren't before... | 18:43.58 |
| sebras | the remaining channels are labelled as undefined | 18:44.31 |
| Robin_Watts | oh, I see. | 18:45.20 |
| | We check all the components for validity, then never look at more than the first 4. | 18:45.54 |
| | or 5. | 18:46.03 |
| sebras | yes, for luratech I clamp the number of components to 4 and the number of alphas to 1. | 18:46.48 |
| | Robin_Watts: there's a oneliner on sebras/master. | 18:48.03 |
| | Robin_Watts: I noticed this when trying to render your testfile and it crashed. | 18:48.18 |
| Robin_Watts | lgtm. | 18:48.27 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=e53cc39cf3499dcab5bd283de5cef42efa3b9d6a | 18:48.42 |
| | I have to step away for a short while. | 18:48.52 |
| sebras | I should sleep. | 18:49.06 |
| Robin_Watts | Night! | 18:49.28 |
| sebras | Robin_Watts: lgtm. | 18:49.29 |
| Robin_Watts | Thanks. | 18:49.33 |
| sebras | Robin_Watts: mine clustered ok before so I pushed it. | 18:50.21 |
| | Forward 1 day (to 2016/10/13)>>> | |
Log of #ghostscript at irc.freenode.net.
