| | <<<Back 1 day (to 2016/10/31) | 20161101 |
| ray_laptop | all these people on this channel with nothing to say (testing ghostbot) | 15:50.19 |
| sebras | tor8: we spoke about pdf_dict_put_null() earlier. don't you think that would be too specific? | 16:10.26 |
| | tor8: I'm not sure whether it is reasonable to expect pdf_dict_put_val_drop() to be used to put anything else than null, but it doesn't seem farfetched..? | 16:11.13 |
| | Robin_Watts: ok, there are four small patches on sebras/master that I believe are the least controversial. | 16:19.42 |
| Robin_Watts | sebras: The removal of Image_newImageFromBitmap... surely we want to have that in the API? | 16:39.10 |
| | The idea is to allow people to call MuPDF with java generated bitmaps. | 16:39.27 |
| sebras | Robin_Watts: it has never been added there though..? | 16:39.40 |
| Robin_Watts | sebras: Right, so the bug is not having the java declaration of this function. | 16:40.03 |
| | i.e. real bug, wrong fix. IMHO. | 16:40.23 |
| sebras | Robin_Watts: np, I did this to trigger the discussion. :) | 16:40.36 |
| Robin_Watts | sure. | 16:40.42 |
| sebras | it is easier to dicuss an example. | 16:40.49 |
| Robin_Watts | sebras: In the "Do not drop compressed bugger twice in case of error" one... | 16:42.09 |
| | could we not do: | 16:42.15 |
| | bc2 = bc; bc = NULL; image = fz_new_image_from_compressed_buffer(.... bc2, ...) | 16:42.43 |
| | That would avoid the need to have a second fz_try/fz_catch. | 16:42.53 |
| | (the time to set up a fz_try/fz_catch is non trivial) | 16:43.12 |
| | or bc_passed = bc; if you want to use clearer naming. | 16:43.37 |
| sebras | Robin_Watts: yes, that would also work. | 16:44.55 |
| Robin_Watts | sebras: I think that would also be clearer. | 16:45.22 |
| sebras | I agree. | 16:45.33 |
| Robin_Watts | well, "bc_passed = bc; bc = NULL; /* Avoid freeing bc on error as ownership is passed in */" would certainly be clearer :) | 16:46.07 |
| | but well done for spotting these. | 16:46.16 |
| | Last one looks good to me. | 16:46.24 |
| sebras | Robin_Watts: I have recently started to use awk to try to identify issues with how we use fz_{always,catch,rethrow,try,var}(). have found a number of issues recently. | 16:49.12 |
| | the return inside the fz_try() was one of them. | 16:49.27 |
| Robin_Watts | nice. I really wish I could think of a compile time detection method for that. | 16:52.53 |
| sebras | Robin_Watts: I plan to read up on modeling files for coverity. I think it might be able to help us with this kind of thing. | 16:53.27 |
| | maybe. | 16:53.30 |
| | Robin_Watts: hm... the bitmp that Image_newImageFromBitmap() takes is an android.graphics.Bitmap is it not? | 16:54.17 |
| | Robin_Watts: we have previously isloated everything android to AndroidDrawDevics | 16:54.47 |
| Robin_Watts | hmm. Maybe that's why it's not in the java currently. | 16:55.31 |
| sebras | adding this new interface means we can no longer build for jdk. | 16:55.32 |
| | seems likes. | 16:55.38 |
| | Robin_Watts: I wonder if it would be possible to have and AndroidImage subclass. | 16:55.59 |
| | that is compiled only for android. | 16:56.08 |
| Robin_Watts | We'd probably need an AndroidImage class that could derive from Image, and add that single method. | 16:56.18 |
| | Yeah, you type faster. :) | 16:56.26 |
| | But at any rate, deleting the code feels bad for now. | 16:56.44 |
| sebras | Robin_Watts: not in chinese! I write sooo slooowwlyyyy! | 16:57.27 |
| | Robin_Watts: better patch online. | 17:27.37 |
| tor8 | sebras: we could just pdf_dict_del where we put null instead. we only do it for 'bad' indirect references. | 17:30.51 |
| Robin_Watts | sebras: AIUI, we don't need fz_var(buf) any more now. | 17:31.43 |
| | Other than that, all look good. | 17:31.57 |
| sebras | Robin_Watts: fz_var(bc)? no, that's true. | 17:33.12 |
| | tor8: a pdf_dict_del_null()? | 17:37.10 |
| Robin_Watts | 1 commit on robin/master: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=2f9fd3dfd4344a28b817061dbc7bc33abdd8dda3 | 17:38.15 |
| | With this being the useful commit to look at: http://git.ghostscript.com/?p=thirdparty/jpeg.git;a=commitdiff;h=e238e70182f86465f3b598d486088c47935b3043 | 17:38.49 |
| | The bmpcmp (with a small threshold applied) in my bmpcmp area shows the differences. | 17:39.26 |
| | All in halftoned stuff (so can be ignored) except for the last few which are progressions. | 17:39.56 |
| sebras | Robin_Watts: is jpeg_integer_overflow_and_read_AV.xps a progression? | 17:41.34 |
| Robin_Watts | sebras: Yes. | 17:41.42 |
| | You can see the bar does flip over from grey to black anymore. | 17:41.56 |
| | That's when it overflowed. | 17:41.59 |
| | s/does/doesn't/ | 17:42.08 |
| sebras | Robin_Watts: from red to black..? | 17:42.12 |
| Robin_Watts | I was talking about the pgm rendering :) | 17:42.31 |
| | but yes :) | 17:42.37 |
| sebras | Robin_Watts: oh.. wait... the order left to right is: New Old Diff, right? | 17:43.08 |
| Robin_Watts | NOD. | 17:43.33 |
| | yes :) | 17:43.36 |
| sebras | Robin_Watts: then I agree that it's a progresison. :) | 17:43.47 |
| | Robin_Watts: if CLAMP_DC internally was a ternary expression you could still combine it with << PASS1_BIT and do everything on a single line..? | 17:45.03 |
| | Robin_Watts: not sure if that is better in anyway though. | 17:45.29 |
| Robin_Watts | sebras: That would mean doing something like: | 17:45.46 |
| | tmp = CLAMP_DC(DEQUANTISE(.....)) ? | 17:46.05 |
| sebras | yea. | 17:46.11 |
| Robin_Watts | And the CLAMP_DC macro would have to evaluate DEQUANTISE() multuple times. | 17:46.40 |
| | Which is a) only fine if you have a compiler with a decent enough CSE operation, and b) breaks horribly if any of the calls have side effects. | 17:47.38 |
| sebras | Robin_Watts: would it make sense to make the clamping part of DEQUANTIZE() itself? | 17:47.53 |
| Robin_Watts | sebras: Possibly, but that would end up clamping ALL the coefficients. | 17:48.18 |
| sebras | Robin_Watts: also I'm a bit worried that this patch is sitting on a branch next to v9..? | 17:48.21 |
| Robin_Watts | which would be slower. | 17:48.26 |
| | I believe this can only happen with the DC coefficient, cos that's the only one that's coded as a difference from the previous one. | 17:49.02 |
| | and we only overflow by the differences adding up - values can't be directly specified that are too large. | 17:49.32 |
| | Now, possibly, the *correct* place for the clamping check is actually post the DCT, but... that would be much more work to do. | 17:50.07 |
| | Until I hear back from ijg, I reckon I can live with this - it certainly doesn't hurt us. | 17:50.37 |
| ray_laptop | Robin_Watts: in your jpeg patch, why don't you have the CLAMP_DC after all the DEQUANTIZE operations ? | 17:51.31 |
| Robin_Watts | It has to be on a branch, or a tag, or git will helpfully remove it after 30 days. | 17:51.36 |
| | ray_laptop: I just answered that :) | 17:51.42 |
| ray_laptop | oh, sorry. I was behind in the lofs | 17:51.55 |
| Robin_Watts | It's only an issue for the DC coefficient, I believe. | 17:51.55 |
| ray_laptop | logs | 17:51.57 |
| Robin_Watts | np :) | 17:51.58 |
| sebras | Robin_Watts: ok, so we are not using jpegv9 (if we were then I'd expect your patch to be on top of it..?) | 17:53.19 |
| Robin_Watts | sebras: The parent of my commit is v9. | 17:53.54 |
| | artifex~1 == v9 | 17:54.13 |
| sebras | artiefx~1 == v9-pre here, what's the difference between v9-pre and v9? | 17:54.47 |
| Robin_Watts | v9 = tag not branch. | 17:54.58 |
| | I have no v9-pre tag that I can see. | 17:55.16 |
| sebras | Robin_Watts: mine sits on 219d59d | 17:55.30 |
| Robin_Watts | v9-pre and v9 are the same, now I've fetched. | 17:55.46 |
| sebras | so what is the difference between origin/master and origin/artifex? | 17:55.49 |
| Robin_Watts | ooh. and I have an origin/master now. | 17:56.17 |
| | Hold on. | 17:56.19 |
| sebras | grabs on to something. | 17:57.02 |
| Robin_Watts | Let rebase onto the latest. | 17:57.38 |
| | ok, new versions online. | 17:59.11 |
| | http://git.ghostscript.com/?p=thirdparty/jpeg.git;a=commitdiff;h=f063390dca811d9f6b7cbb853b97a58cab36b440 | 17:59.21 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=946f5530b3e3f212875b12b3ad5e82fd0bef4458 | 18:00.07 |
| sebras | Robin_Watts: I don't understand _why_ the dc coeff must be in the interval 1024..-1023, or how it can accidentally overflow yet, but the patch looks plausible. | 18:02.12 |
| | Robin_Watts: I'm happy to ignore the details if I may. :) | 18:02.39 |
| | Robin_Watts: is a similar patches necessary for jidctfst.c too? | 18:06.14 |
| | it also appears to refer to DEQUANTIZE(...DCTSIZE*0...) by the name dcval. | 18:06.42 |
| | Robin_Watts: and that file is included in Makethird. | 18:07.07 |
| | Robin_Watts: jidctfst.c:205 (and possibly jidctfst.c:224) | 18:07.35 |
| | Robin_Watts: I'm assuming jidctflt.c:104 is not a problem due to being float..? | 18:07.59 |
| Robin_Watts | sebras: We only use jidctslow :) | 18:08.22 |
| sebras | Robin_Watts: oh. why do we include jidctfst.c in that case?! | 18:09.01 |
| Robin_Watts | sebras: JPEG provides a way to code each coefficient in a reasonable range. | 18:09.23 |
| | with smaller (nearer to zero) values taking fewer bits. | 18:09.46 |
| | because each DCT block will tend to have a similar DC value (the 'average' value of the block) to the one before it, they therefore code the difference between DC values. | 18:10.33 |
| | So if you code: big value, big value, big value... you can quickly move from 'sane' values to 'insane' values. | 18:11.13 |
| | If you then do a DCT operation with those insane values, you get insane values throughout the DCT block. | 18:11.45 |
| | and when you clip them you get 'sane' values back again. | 18:12.00 |
| sebras | Robin_Watts: right, that seems plausible. | 18:12.26 |
| Robin_Watts | BUT... the way the jpeglib code does it's clamping is by assuming that you'll never get batshit-crazy-insane values as input. | 18:12.55 |
| | It'll cope with 'slightly eccentric' values, but not utterly bonkers ones. | 18:13.16 |
| | And the test file for this bug codes 'big value, big value, big value' lots of times so we overflow, then a while later 'big -ve value, big -ve value, big -ve value' etc, so we move back to sanity. | 18:14.07 |
| sebras | Robin_Watts: makes sense to me. | 18:15.05 |
| | Robin_Watts: how do we know that we don't call jidctfst.c? | 18:15.14 |
| Robin_Watts | The DCT we use is decided by the method set, which can be JDCT_ISLOW, or JDCT_IFAST or JDCT_FLOAT | 18:15.41 |
| sebras | Robin_Watts: we never appear to refer to that so we get the default which is ISLOW I presume..? | 18:16.38 |
| Robin_Watts | And we set JDCT_DEFAULT | 18:16.53 |
| sebras | Robin_Watts: where? | 18:17.03 |
| Robin_Watts | which is set to JDCT_ISLOW as we don't configure it otherwise. | 18:17.06 |
| | In jpeglib.h line 240 | 18:17.19 |
| sebras | Robin_Watts: ah... jpeglib sets JDCT_DEFAULT unless we configure it. I see. | 18:18.08 |
| | Robin_Watts: so now we have a good reason to use JDCT_ISLOW rather thatn JDCT_IFAST (since the bug isn't fixed there yet) | 18:18.42 |
| | what is the reason for not choosing JDCT_IFAST in the first place though? | 18:19.00 |
| | less correct rendering? | 18:19.22 |
| | Robin_Watts: final LGTM on sebras/master before I push? | 18:22.12 |
| Robin_Watts | I didn't make that choice. | 18:22.24 |
| sebras | ? | 18:22.34 |
| Robin_Watts | (which of FAST or SLOW we use) | 18:22.50 |
| sebras | right. | 18:23.06 |
| Robin_Watts | so I can't comment on why. I'd guess because we'd rather have correctness over speed. | 18:23.10 |
| sebras | btw, I see no 1.10 tag so maybe I should save my patches until 1.10 is tagged..? | 18:23.24 |
| Robin_Watts | SOT uses FAST if memory serves me, and I think has ARM code implementations. | 18:23.57 |
| | sebras: Anything you push now will go into 1.10, AIUI. | 18:24.10 |
| sebras | Robin_Watts: right. then I'll hold off the patches until it is out. | 18:24.41 |
| Robin_Watts | sebras: lgtm. | 18:24.49 |
| sebras | thanks for the help and zzz. | 18:24.56 |
| Robin_Watts | You can push them, I guess. | 18:25.03 |
| | cos if we don't want them, tor can make a branch for 1.10 and cherry-pick? | 18:25.22 |
| | sebras: So, did you lgtm the commits? | 18:25.59 |
| | (the jpeg fix I mean?) | 18:26.12 |
| | Forward 1 day (to 2016/11/02)>>> | |
Log of #ghostscript at irc.freenode.net.