| | <<<Back 1 day (to 2018/04/02) | 20180403 |
| vtorri_ | hello | 09:06.49 |
| mubot | Welcome to #mupdf, the channel for MuPDF. If you have a question, please ask it, don't ask to ask it. Do be prepared to wait for a reply as devs will check the logs and reply when they come on line. | 09:06.49 |
| vtorri_ | when should fz_always be used, which cases ? | 09:07.10 |
| tor8 | vtorri: https://mupdf.com/docs/coding-overview.html#error-handling | 09:07.58 |
| vtorri_ | thank you | 09:09.55 |
| | another question : | 09:10.01 |
| | let's say i write : | 09:10.09 |
| | fz_try(ctx) { st = fz_open_memory(****); } | 09:10.39 |
| | if there is an error, is st set to NULL ? | 09:10.52 |
| moolc | that'd be the day | 09:11.09 |
| tor8 | no. if there is an error, st will be undefined. | 09:11.16 |
| vtorri_ | (so in the catch block) | 09:11.18 |
| | tor8 so if necessary in my code, i shouold set it to NULL in the catch block, right ? | 09:11.52 |
| moolc | tor8: have you received my mail regarding layout with zero height and count_pages returning -1? | 09:16.36 |
| tor8 | moolc: yes, haven't had time to investigate yet. | 09:17.14 |
| paulgardiner | vtorri: better to initialize st to NULL where you declare it, and add fz_var(st) before the try block. | 09:17.15 |
| vtorri_ | paulgardiner ok, thank you | 09:18.10 |
| tor8 | moolc: can you reproduce with mutool draw or mupdf-x11/mupdf-gl with "-H 0"? | 09:18.55 |
| moolc | sec | 09:22.50 |
| | tor8: hmm.. i can't even reproduce it with llpp now.. weird.. could be something about that particular epub.. gimme a moment | 09:24.56 |
| | after pagedowninga bit in mupdf-gl i get: | 09:25.42 |
| | warning: texture size (600 x 17222) exceeds implementation limit (16384) | 09:25.44 |
| | warning: glGetError(swap buffers): GL_INVALID_VALUE | 09:25.44 |
| vtorri_ | another question : if i want to replace an old fz_page with a new fz_page, i do something like : http://codepad.org/mOGyssWZ | 09:27.06 |
| tor8 | moolc: I'm not surprised, mupdf-gl assumes a page will be sanely sized and able to fit in a single texture | 09:27.17 |
| vtorri_ | is it the correct way to do this, or is there a better way ? | 09:27.28 |
| moolc | tor8: yep.. i have VERY weird behavior with that particular epub | 09:27.53 |
| | even in mupdf-gl | 09:27.56 |
| | tor8: https://boblycat.org/~malc/scratch/em.epub | 09:29.36 |
| tor8 | vtorri: that code will work. fz_drop_* functions are guaranteed to never throw though. | 09:29.54 |
| vtorri_ | tor8 thank you | 09:30.08 |
| | that will simplify a bit the code | 09:30.24 |
| tor8 | vtorri: fz_try() { new_page = fz_load_page(); fz_drop_page(old_page); old_page = new_page; } fz_catch() { return ERROR; } should be enough | 09:31.29 |
| vtorri_ | tor8 thank you | 09:31.57 |
| tor8 | you need fz_var if you both (a) set the variable in the try-block, AND (b) read the variable in the always or catch blocks | 09:32.34 |
| vtorri_ | tor8 ok, so no need here | 09:33.03 |
| | thank you | 09:33.06 |
| tor8 | there's no need in your pasted example too, but it can't do any harm so if you're unsure it's better to have it than not to | 09:34.40 |
| | moolc: yes, that is a strangely behaving epub | 09:35.07 |
| moolc | tor8: maybe fitz doesn't like the content? (russian translation of https://en.wikipedia.org/wiki/Epistulae_Morales_ad_Lucilium) | 09:35.52 |
| tor8 | moolc: if you unzip the epub and look at each chapter by its own | 09:38.06 |
| | you find that the 'OPS/cover.xhtml' file is the one causing trouble | 09:38.22 |
| | mupdf thinks it's -MAX_INT tall | 09:38.27 |
| moolc | -MAX_INT is asking for trouble indeed ;) | 09:38.53 |
| tor8 | I suspect it's a divide by zero problem | 09:39.39 |
| moolc | tor8: sigh.. | 09:40.43 |
| | tor8: unless you are using ppc or arm (/0 is 0) i don't see how it's a posibility | 09:41.01 |
| tor8 | moolc: but even without -H 0 that file shows as a weird number of pages | 09:41.21 |
| | page 1 / 0 is not right... | 09:41.37 |
| moolc | tor8: point being, on x86_64 (which, i assume, is what you are using will trap on /0) | 09:42.10 |
| | +it | 09:42.17 |
| tor8 | yeah. it's the zero sized document causing trouble. | 09:44.19 |
| | it thinks it is zero pages long. | 09:44.25 |
| | moolc: floating point division | 09:46.09 |
| moolc | ah | 09:46.38 |
| | that would explain it | 09:46.42 |
| tor8 | moolc: top commit on tor/master should fix it | 09:53.07 |
| moolc | min | 09:56.50 |
| | tor8: it does, tack | 09:59.25 |
| Robin_Watts | I need a reviewer for: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=b7895f158741f791d0a6211ad955bc8bce0f3452 | 11:11.40 |
| | please. | 11:11.42 |
| | tor8, sebras, (and in this instance, kens): ^ | 11:12.04 |
| kens | Well it looks fine to me, but I wouldn't trust me. | 11:12.18 |
| | I'll just boot my laptop and see that its the same as what I have | 11:12.35 |
| Robin_Watts | kens: Yes, but you've actually tested it, which makes you a good choice :) | 11:12.39 |
| kens | LOL | 11:12.44 |
| | 1 secodn while I find the file and check | 11:12.58 |
| Robin_Watts | Ta. | 11:13.01 |
| | I don't have 2008 installed on any current machine, as far as I know. | 11:13.26 |
| kens | Yes that's exactly what I have, and that compiles fine for me. | 11:13.34 |
| Robin_Watts | Ta. | 11:13.41 |
| kens | FWIW the MS documentation seems to indicate tha it *should* work on 2008, but it really didn't. | 11:14.01 |
| Robin_Watts | I'll give tor/sebras a bit to see if they complain, and then push it. | 11:14.03 |
| tor8 | Robin_Watts: LGTM. | 11:18.25 |
| Robin_Watts | tor8: Ta. | 11:19.15 |
| sebras | Robin_Watts: in "Android: Scavenge on Bitmap.lockPixels() failure." why can't you set size == info->width * info->height * 4? | 13:55.07 |
| | Robin_Watts: we know that it is ARGB, do w not? | 13:55.29 |
| Robin_Watts | info->height doesn't exist? | 13:55.30 |
| | If you want to make it so info->height DOES exist, I won't complain. | 13:56.05 |
| sebras | Robin_Watts: that seems like a good idea. :) | 13:56.44 |
| | Robin_Watts: your patch uses ANDROID_BITMAP_RESULT_SUCCESS, but I believe that the result code is ANDROID_BITMAP_RESULT_SUCCESS | 14:03.12 |
| | let me try that again... | 14:03.24 |
| Robin_Watts | I know, and yes, you're right. | 14:03.30 |
| kens | was struggling to see the difference there.... | 14:03.36 |
| Robin_Watts | there was a stray "ALLOCATION" in there. | 14:03.37 |
| sebras | yes. | 14:03.42 |
| Robin_Watts | sebras: Yeah, as I said at the time, I wasn't in a position to test my patch. | 14:03.56 |
| sebras | I pasted the wrong thing the first time around. I'm happy that you were able to see what I meant. :) | 14:04.03 |
| | Robin_Watts: that's fine. I just want us to get that patch into master so that fred isn't waiting for us. | 14:04.38 |
| Robin_Watts | sebras: yeah. You're doing an updated version with info->height, right ? | 14:05.14 |
| sebras | Robin_Watts: I can. | 14:05.36 |
| Robin_Watts | If you could, I'd be grateful. | 14:05.53 |
| | I'm swearing at gs internals at the moment. | 14:06.07 |
| sebras | Robin_Watts: I can help you with that too if you want...? :) | 14:06.22 |
| Robin_Watts | hehe :) | 14:06.31 |
| | I have the swearing well in hand. | 14:06.53 |
| tor8 | Robin_Watts: sebras: tor/master could do with a review, if you want some distraction :) | 14:08.17 |
| sebras | tor8: let me first improve robin's patch, get you blessing and get it in..? :) | 14:08.45 |
| tor8 | sebras: sure. | 14:10.01 |
| HenryStiles | hmm I added the "potential" customer number to the mupdf form problem with hopes it would show up on the bug report. maybe it has to be p2 to show up | 14:10.16 |
| tor8 | and then I think we ought to consider whether we should do a release with what we've got, or skip this spring's release | 14:10.30 |
| HenryStiles | tor8: oh you fixed it, wonderful thanks | 14:11.22 |
| Robin_Watts | tor8: pdf_store_item(ctx, obj, foo, sizeof(foo)); | 14:11.32 |
| | That takes the reference to foo ? | 14:11.40 |
| | No, ignore me. being dim. | 14:12.03 |
| | First 2 commits lgtm. | 14:12.56 |
| | The next one, the change to fz_drop_compressed_buffer looks precisely wrong to me. | 14:13.16 |
| | It's functionally identical, and suffers righthand drift. | 14:13.44 |
| | fz_open_image_decomp_stream needs fz_var(head), I think. | 14:15.24 |
| | oh, I see why that's not needed. | 14:16.43 |
| tor8 | I've forgotten why it's not needed... I wrote this before a long weekend :) | 14:17.53 |
| | I think that function may leak body if head=fz_open_foo throws | 14:19.14 |
| Robin_Watts | tor8: I think you may be right. | 14:20.05 |
| | tor8: On the next one... do we count empty chapters as blank pages now then ? | 14:20.25 |
| tor8 | yes. each chapter should begin on a new page. | 14:20.59 |
| Robin_Watts | tor8: OK, all lgtm except for the fz_open one. | 14:21.25 |
| tor8 | only two problems? the right-hand drift of the two lines being worse than negated logic, and the cleanup booboo in fz_open_image_decomp_stream? | 14:22.22 |
| Robin_Watts | tor8: I lost the will to live reading through it. | 14:22.46 |
| | Those were the 2 I know of. | 14:22.53 |
| | I'll try again later :) | 14:22.57 |
| | after the imminent meetings. | 14:23.51 |
| Robin_Watts | fetches drink. | 14:23.58 |
| sebras | Robin_Watts: tor8: are you both happy with what is on sebras/master? (compared to robin/master) | 14:38.00 |
| | I haven't tested it proper, but it does compile under android without errors. | 14:38.15 |
| Robin_Watts | sebras: lgtm. | 14:42.32 |
| tor8 | sebras: no objections from me. | 14:43.26 |
| sebras | then I'll merge it..? | 14:43.43 |
| Robin_Watts | go for it. | 14:43.53 |
| tor8 | sebras: you may want to expose Pixmap.gamma(float e) at the same time as adding Pixmap.invert(). | 14:44.59 |
| | you really want to flip the gamma curve when inverting colors | 14:45.13 |
| sebras | tor8: good point. I'll get to that next. | 14:45.14 |
| | after reviewing tor/master that is. | 14:45.23 |
| | Robin_Watts: tor8: pushed. | 14:46.00 |
| | fredross-perry: that means that the solution that robin gave you now is on mupdf origin/master. | 14:46.22 |
| | fredross-perry: part way solution for 699048 | 14:46.48 |
| fredross-perry | ok, i'll go and look, thanks. i'll take a look. | 14:49.11 |
| | I'm from the department of redunancy department | 15:00.06 |
| sebras | tor8: I think I may have reviewed the first commits on tor/master before..? | 15:20.38 |
| | tor8: they LGTM. | 15:20.41 |
| | tor8: the third patch "Don't implicitly drop in fz_open_* chained filters." seems like you fixed a bug in in a later commit..? | 15:22.46 |
| | tor8: why aren't these squashed? | 15:22.54 |
| | tor8: in fz_open_image_decomp_stream(), why do you check if the flate.predictor > 0? | 15:29.13 |
| tor8 | sebras: they are not squashed so that robin can look at the fix | 15:29.34 |
| | sebras: if predictor == 0 then there is no predictor filter | 15:30.10 |
| sebras | tor8: as I read the predictor values in table 3.8 on page 76 of pdfref17 it seems that value == 1 no prediction..? | 15:30.28 |
| tor8 | oh, should that be 1 maybe? | 15:30.35 |
| Robin_Watts | fz_drop_stream can never throw, right? (no fz_drop can throw) | 15:30.58 |
| tor8 | yeah, it should. I probably changed from > 1 to == 0 and then changed my mind | 15:31.00 |
| | Robin_Watts: correct, no fz_drop should be able to throw | 15:31.12 |
| Robin_Watts | tor8: ok, lgtm. | 15:36.13 |
| sebras | tor8: why can fz_open_image_decomp_stream() return body? | 15:37.36 |
| | tor8: or even tail. | 15:37.50 |
| | tor8: seems to me it should only ever return head..? | 15:38.09 |
| | tor8: because if head is not set properly then we have had and exception that caused us to throw. | 15:38.41 |
| tor8 | sebras: case FZ_IMAGE_FLATE. no predictor. we set 'body' but not head. | 15:39.17 |
| sebras | tor8: ah, yes. correct. | 15:39.35 |
| | tor8: and tial..? | 15:39.43 |
| | even if params->type is awkward head is set..? | 15:40.00 |
| tor8 | tail is not possible (but defensive programming) | 15:40.03 |
| Robin_Watts | tor8: Might it not be neater to make the no-predictor case in the FZ_IMAGE_FLATE bit set head and null body? | 15:41.07 |
| | Then we never need to return body, and that makes everything simpler. | 15:41.27 |
| tor8 | Robin_Watts: I have a separate head and body to cope with the need to drop the 'body' intermediate stream | 15:42.10 |
| | I think I have a formulation that is easier to understand, one minute while I test | 15:44.14 |
| sebras | tor8: sounds nice, nested ternary expressions are interesting. :) | 15:44.52 |
| tor8 | sebras: Robin_Watts: okay, fetch tor/master again | 15:45.39 |
| | 'fixes for drop in chain commit' commit | 15:45.50 |
| sebras | tor8: I like that commit. :) | 15:49.57 |
| Robin_Watts | I dislike the repeated function calls. There must be a better way.... | 15:51.42 |
| | head = fz_open_flated(ctx, tail, 15); | 15:55.10 |
| | if (params->u.flate.predictor > 1) | 15:55.11 |
| | { | 15:55.13 |
| | body = head; head = NULL; | 15:55.14 |
| | head = fz_open_predict(...); | 15:55.16 |
| | } | 15:55.17 |
| sebras | tor8: when you changed how fz_open_dctd() keeps the jpegtables streams you didn't fix the call in source/fitz/load-tiff.c right..? | 15:57.16 |
| tor8 | sebras: probably not, no | 16:00.39 |
| sebras | tor8: also there is an assymetry between jpegtables and the jbig2 globals in that the latter reference is passed on to the jbig2-streams while the jpegtables is not. | 16:10.42 |
| tor8 | sebras: can you fix that one? I worry about breaking luratech and stuff if I fiddle around too much with the jbig2dec filters. | 16:11.36 |
| sebras | I can, but not today. | 16:11.54 |
| tor8 | I have pushed a fix for the jpegtables | 16:12.06 |
| | Robin_Watts: ugh. I like that less... | 16:12.17 |
| Robin_Watts | tor8: Well, go with what you like. My preference would be for avoiding repeated function calls so they don't fall out of sync, but... | 16:13.28 |
| sebras | tor8: wouldn't pdf_load_compressed_inline_image() need to always drop bc->buffer, and not only in the error case..? | 16:14.46 |
| | tor8: since fz_open_leecher now keeps its own reference. | 16:15.08 |
| tor8 | Robin_Watts: hm, setting head = NULL there serves no purpose. without that it looks good enough. new formulation on tor/master. | 16:15.49 |
| Robin_Watts | tor8: If we don't set head = NULL, and the second call fails, we get to always with head = body. | 16:16.38 |
| sebras | tor8: in fz_open_predict() in fz_catch() you don't need to test if state is set, you know that it is. | 16:17.28 |
| tor8 | Robin_Watts: head is never used if we throw, only 'body', and then we'll drop the 'body' as expected | 16:17.33 |
| Robin_Watts | tor8: OK. | 16:17.45 |
| | Yeah, that gets my vote. | 16:18.15 |
| sebras | tor8: you can however not be sure if state->in and state->out have been set, so they would presumably need to be initialized to NULL..? | 16:18.21 |
| tor8 | sebras: back a minute to the pdf_load_compressed_inline_image please | 16:18.42 |
| sebras | refetches tor. | 16:19.40 |
| tor8 | sebras: if we succeed, bc escapes through the fz_set_compressed_image_buffer. why would we want to free bc->buffer then? | 16:20.36 |
| | compressed buffers are not reference counted | 16:20.50 |
| sebras | tor8: oh, I see. I didn't notice that. | 16:21.51 |
| tor8 | sebras: correct re: state in fz_open_predict. fz_malloc_struct zeroes the fields, so in/out fields need no explicit setter. | 16:22.08 |
| | sebras: yeah. I put a comment to that effect in the fz_set_compressed_image_buffer function body. | 16:22.34 |
| sebras | tor8: ah, yes, that's the calloc in disguise. | 16:22.46 |
| | tor8: in pdf_open_filter(), why are you dropping chain in the error case? isn't the point of the build_filter_drop()/build_filter_chain_drop() functions that they themselves drop chain in case of error? | 16:42.00 |
| | tor8: I think you missed that one. | 16:42.35 |
| tor8 | sebras: 'chain' is the intermediate stream from pdf_open_raw_filter | 16:43.28 |
| | it is not the input argument | 16:43.33 |
| sebras | tor8: I know, but if pdf_open_raw_filter() succeeds then chain has a reference, but that is passwed on to build_filter_drop() as the second argument, and if its internal call to build_filter fails, then build_filer_drop() will drop this chains (but renamed tail) | 16:45.19 |
| | (btw, the comment to build_filter_drop ought to say tail, not chain I think) | 16:45.40 |
| | tor8: ping? | 16:51.35 |
| | tor8: I've managed to review that commit until its end now. | 16:51.50 |
| | tor8: I'm still not convinced about what you said last. | 16:52.06 |
| tor8 | sebras: nor am I :) | 16:52.12 |
| sebras | tor8: ok, does that mean that you have now sided with me and there will be a new commit soon, or that we are still both confuse some how? :) | 16:52.50 |
| tor8 | yes. there's a new commit on tor/master now. | 16:54.02 |
| sebras | tor8: ok, that's one way of doing it. | 16:57.03 |
| | tor8: the other would have been to keep build_filter_drop() and build_filter_chain_drop() and not drop the result from pdf_open_raw_filter() in fz_catch(). | 16:57.33 |
| | (instead leaving this to build_filter_drop()/build_filter_chain_drop()). | 16:57.53 |
| tor8 | sebras: but then if pdf_array_len throws we'll leak | 16:57.59 |
| sebras | correct! | 16:58.13 |
| | tor8: ok, pleaes squash these two now. | 16:59.12 |
| | tor8: after that I have reviewed up to "Update Freetype submodule to version 2.9." | 16:59.30 |
| | they lgtm, but I have no idea if the harfbuzz/freetype updates create subtle cluster differences. I assume you have already clustered this and now..? | 17:00.03 |
| | know. | 17:00.06 |
| | the namedump thing I'm not at liberty to understand tonight. | 17:00.20 |
| | I'll wait with that until later. | 17:00.25 |
| | and the jbig2globals implementation as it is is fine, I can update that part in a later commit. | 17:00.46 |
| | tor8: ok, I'll go to bed now. | 17:09.41 |
| | tor8: up until "Update Freetype submodule to version 2.9." LGTM. | 17:10.17 |
| tor8 | sebras: thanks. | 17:14.43 |
| fredross-perry | robin - one thing about handling failing locks. in NativeDevice_close(), if lockNativeDevice fails, the device is not closed. Is this an issue, or a leak perhaps? | 17:19.18 |
| Robin_Watts | let me look. | 17:19.35 |
| fredross-perry | I am testing this by injecting random failures, and I came across this. | 17:19.39 |
| Robin_Watts | Urm... IIRC the rule for this stuff is that you create a device, use it, close it, drop it. | 17:20.51 |
| | using it, and closing it can both throw exceptions. | 17:21.05 |
| | dropping it cannot. | 17:21.10 |
| | That was the reason for introducing close, in fact. | 17:21.23 |
| | so that we could have a "flush" effectively without it needing to be in 'drop'. | 17:21.37 |
| | So, I think it's fine that close can throw and not drop the device. | 17:21.57 |
| fredross-perry | drop happens later in Device_finalize | 17:22.17 |
| | right now NativeDevice_close() does not drop. | 17:22.44 |
| Robin_Watts | In the same way that if a "use" of the device fails, then the contents of the device are undefined, and the caller needs to call drop, the same thing applies to "close". | 17:22.50 |
| fredross-perry | ok, sounds like we're ok then as log as I arrange for a drop (finalize) | 17:23.20 |
| Robin_Watts | yeah. | 17:23.27 |
| fredross-perry | k | 17:23.32 |
| | thanks | 17:23.34 |
| Robin_Watts | no worries. | 17:23.38 |
| | So, where were we with silver on this? | 17:23.49 |
| | From re-reading the bug it seems that the fixes that have gone in have largely got rid of the problems. | 17:24.23 |
| | He could still get problems, but only after a few hours of testing. | 17:24.36 |
| fredross-perry | I am making a build with these changes and sending it to him today. | 17:24.48 |
| Robin_Watts | And when he did get problems, it was because we were failing to lockBitmaps, trying to scavenge and not managing to save anything. | 17:25.09 |
| fredross-perry | Right. and that was without catching/retrying exceptions. | 17:25.13 |
| Robin_Watts | And in those cases, there should be a java exception thrown where we can maybe try something else. | 17:25.29 |
| | cool. | 17:25.31 |
| fredross-perry | cheers | 17:25.42 |
| moolc | sebras: https://i.imgur.com/CcdxgdR.jpg | 22:25.13 |
| | Forward 1 day (to 2018/04/04)>>> | |
Log of #mupdf at irc.freenode.net.
