| | <<<Back 1 day (to 2016/09/18) | 20160919 |
| deb75 | Hello, I use mupdf on android, I make hand written annotations on pdf with it, nevetheless I do not understand how you manage afterwards annotations, how to remove them ? also is it possible to tune the pen width ? | 08:20.34 |
| Robin_Watts | deb75: Annotation functionality is not quite as complete as we would hope at the moment. | 08:30.57 |
| paulgardiner | deb75: if I remember correctly, the app used to allow deletion of annotations. You could just tap to select and then a delete button would be displayed. | 08:44.22 |
| | Though the app has changed significantly since I last used it. | 08:44.57 |
| deb75 | thanks, annotation removal works well, anything about configuring the pen width ? not yet ? | 08:46.49 |
| paulgardiner | Width and color control is supported in the library but not yet in the app. We are currently in the process of completely rewriting the app. I believe that feature and many other new ones will be present | 08:51.32 |
| deb75 | Thanks for your complete answeer, I look forward to using the rewritten app ! Regards. | 08:58.49 |
| sebras | Robin_Watts why is it not enough to use the size of the store entries and only evict once the reapable size is over some threshold? | 11:06.16 |
| | I think the entries already keep their size..? | 11:06.38 |
| Robin_Watts | sebras: The issue is that we want the store to be able to clear 'just in time'. | 11:07.19 |
| | i.e. if someone tries for a malloc, and it fails, we want to be able to ditch as much as possible, quickly. | 11:08.04 |
| | Also, for cleanup purposes, we don't want stuff (however small) hanging round for an arbitrary length of time. | 11:09.17 |
| | I have a plan, and I may have avoided the need for a semaphore... | 11:09.32 |
| sebras_ | Robin_Watts: right. | 11:12.32 |
| Robin_Watts | All but the last 3 are fine. | 11:36.21 |
| | 3rd from the top, the change in svg_parse_transform... | 11:37.20 |
| sebras | Robin_Watts: yes..? | 11:37.36 |
| Robin_Watts | Could we move first = 0 to be done unconditionally after the if ? | 11:38.14 |
| | I reckon we could. | 11:38.19 |
| sebras | Robin_Watts: absolutely. | 11:38.33 |
| Robin_Watts | Which would then mean we could take a load of common code out of the if and lose the first clause. | 11:38.47 |
| | i.e. if (!first) {skip , skip whitespace } | 11:39.07 |
| sebras | and a preceeding skip whitespace before that. | 11:39.48 |
| | indeed. | 11:40.16 |
| Robin_Watts | yes, the "common" bit of the code before that. | 11:40.18 |
| | Is () a valid transform? | 11:41.10 |
| sebras | no. | 11:41.46 |
| Robin_Watts | cos as I read the new code, it will accept that. | 11:41.47 |
| sebras | Robin_Watts: they must be something like matrix(1,2,3,4,5,6) or skewX(42) | 11:42.02 |
| | separated by whitespace or commas. if there is a comma there must be another transform | 11:42.24 |
| Robin_Watts | keyword() would be accepted though with the new code ? | 11:42.53 |
| sebras | no. | 11:43.08 |
| | the nargs check will catch that. | 11:43.17 |
| Robin_Watts | I don't see a nargs check in the patch. | 11:43.38 |
| | Maybe that's outside the patch? | 11:43.47 |
| sebras | could be. | 11:43.51 |
| | else if (!strcmp(keyword, "translate")) | 11:44.16 |
| | { | 11:44.19 |
| | if (nargs != 2) | 11:44.19 |
| | fz_throw(ctx, FZ_ERROR_GENERIC, "wrong number of arguments to transl | 11:44.19 |
| | this happens after the nargs loop. | 11:44.33 |
| Robin_Watts | Right, so each individual thing checks it has the right number of args. | 11:44.57 |
| sebras | Robin_Watts: correct. | 11:45.11 |
| Robin_Watts | so we could in theory have: identity() and it would get through the top loop. | 11:45.14 |
| sebras | Robin_Watts: the longest one accepts 6 arguments, and e.g. skewX() accepts only one. | 11:45.34 |
| | no, becuase there is a finite list of different transformations and we do if (!strcmp(keyword, "matrix")) {} else if (!strcmp(keyword, "translate")) {} ... else fz_throw() | 11:46.14 |
| Robin_Watts | sebras: Yes, but in future if the list was every extended. Not important. | 11:46.57 |
| | That commit looks good to me too then. | 11:47.05 |
| | (with the optional optimisation if you feel like it) | 11:47.18 |
| sebras | yeah, that one reduces the amount of text so I'll take that one. | 11:47.41 |
| Robin_Watts | the jpx one is good too. | 11:47.46 |
| sebras | unless it produces some cluster issues. | 11:47.49 |
| | Robin_Watts: for %lu fz_print("%lu", 0); would print "0u | 11:48.17 |
| | "0u" | 11:48.21 |
| Robin_Watts | I don't understand the last one at all yet. | 11:49.31 |
| | %ll = long long = 64 bit signed. %lu = unsigned long. %llu = unsigned long long, right? | 11:51.25 |
| sebras | Robin_Watts: can you write just %ll without %lld or %llu? | 11:51.58 |
| | or %lli. | 11:52.03 |
| Robin_Watts | Ah, probably not. | 11:52.15 |
| sebras | printf(3) says that the conversion specified (d, u, i, ...) must be there. | 11:53.06 |
| Robin_Watts | ok, so %lld = 64 bit signed, %llu = 64bit unsigned, %lu = unsigned 32bit, unless we are on 64bit linux, in which case it's unsigned 64bit. | 11:53.08 |
| | so length = sizeof(long)*8 would appear to be better? | 11:54.12 |
| | (rather than length=32; ?) | 11:54.20 |
| sebras | is %lu 64 bit on 64-bit? hm... | 11:54.34 |
| | I changed this error messages as so: fz_throw(ctx, FZ_ERROR_GENERIC, "wrong number of arguments to matrix(): %lu", (unsigned long) nargs); | 11:55.00 |
| | error: wrong number of arguments to matrix(): 1u | 11:55.16 |
| | with the patch: error: wrong number of arguments to matrix(): 1 | 11:55.25 |
| Robin_Watts | sebras: AIUI, on 64 linux (not on 64bit windows), longs are 64bit. | 11:55.47 |
| | Hence %lu should be 64bit. | 11:55.54 |
| | (and %ld, %lx etc) | 11:56.15 |
| sebras | correct. | 11:57.06 |
| | Robin_Watts: ok, after your suggested changes it still clusters. want to take a final look before I push to golden? | 12:10.47 |
| Robin_Watts | fetches | 12:11.26 |
| | lgtm now, thanks. | 12:12.57 |
| sebras | Robin_Watts: pushed. | 12:25.39 |
| Robin_Watts | sebras: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=f12c089b455f43f73e9a8cc78d75565c97d582b0 | 13:13.17 |
| | Better version: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=c7737e7a7a4ba23fdd402b4df923c694b10cf192 | 13:14.51 |
| Robin_Watts | foods. bbs. | 13:15.09 |
| sebras | Robin_Watts: can fz_defer_reap_start()/_end() throw? | 14:06.52 |
| | I have only read the documentation for the functions as of yet. | 14:07.13 |
| Robin_Watts | no. | 14:07.17 |
| sebras | good. | 14:07.23 |
| Robin_Watts | Generally speaking, destructors shouldn't throw. | 14:07.38 |
| sebras | maybe we should document that (ideally also for all _drop() and _free() functions). | 14:07.54 |
| | I know. | 14:07.55 |
| | but I wonder if we should underscore that this is the case. | 14:07.58 |
| Robin_Watts | Yeah, maybe. | 14:08.02 |
| sebras | maybe it is implicit and client (and we) should just know this. | 14:08.18 |
| | but it wasn't apparent to me. | 14:08.29 |
| marcosw | chrisl: 9.20rc1 regression testing is almost complete, I'll have final results this evening (PDT). so far everything looks good. | 14:09.16 |
| Robin_Watts | Hi marcosw. How's the new job going? | 14:11.26 |
| chrisl | marcosw: thanks. And thanks for doing the testing | 14:18.13 |
| sebras | Robin_Watts: in fz_store_item() you check if size <= store->max | 14:18.23 |
| | after having reaped. | 14:18.28 |
| | what is it that updates size? | 14:18.39 |
| | wouldn't you need to do size = store->size + itemsize after reaping? | 14:18.48 |
| | because you can no longer want the old value for store->size..? | 14:19.01 |
| Robin_Watts | sebras: D'Oh. Good catch. | 14:19.52 |
| | I was thinking that store->max would have changed, but it's store->size of course. | 14:20.10 |
| sebras | yup. | 14:20.19 |
| | Robin_Watts: one thing I don't get though is why only images need the reaping callback. | 14:20.56 |
| | Robin_Watts: I think this has to do with only images being used as keys..? | 14:21.08 |
| | Robin_Watts: what is used for pdf objects? | 14:21.14 |
| | is that the object number? | 14:22.03 |
| | when applying your patch, rebuilding and running on Bug693416.pdf I get "Attempt to release lock 0 when not held!" | 14:24.21 |
| | let me nuke and retest. | 14:24.34 |
| | Robin_Watts: nope, reproducable. | 14:24.50 |
| | fz_drop_key_storable() is unlocking FZ_LOCK_ALLOC. | 14:26.39 |
| Robin_Watts | sebras: WTF? Why isn't that showing up for me? | 14:27.50 |
| sebras | but do_reap() has already unlocked it. | 14:27.53 |
| Robin_Watts | cos I tested exactly that file. | 14:27.59 |
| sebras | Robin_Watts: on linux? | 14:28.18 |
| | Robin_Watts: or windows? | 14:28.25 |
| | Robin_Watts: maybe the mutexes are recursive on windows? | 14:28.41 |
| Robin_Watts | Shouldn't affect the debugging. | 14:28.59 |
| sebras | I'm not sure why you cannot see it. I get it after 2-3 seconds of rendering. | 14:29.36 |
| | dodrawpage() -> fz_drop_display_list() -> fz_drop_storable() -> fz_drop_display_list_imp() -> fz_drop_image() -> fz_drop_key_storable() -> fz_lock_debug_unlock() -> fprintf() | 14:30.35 |
| Robin_Watts | sebras: Yes, I can see that I should see it... | 14:30.48 |
| sebras | Robin_Watts: actually that ties into my next question about the code. you are only adding fz_defer_reap_start() to pdf_drop_document_imp() but as you can see that function is not in my backtrace so I'm thinking that deferal (sp?) should be done in drop_display_list() too..? | 14:32.28 |
| Robin_Watts | sebras: Yes, probably, will fix. | 14:32.59 |
| | I tested that file, and it completed in less than 2-3 seconds for me. | 14:33.22 |
| sebras | gdb --args ./build/debug/mutool draw -s t ./Bug693416.pdf | 14:33.47 |
| | same? | 14:33.49 |
| Robin_Watts | Windows for me. | 14:34.26 |
| | but I've spotted a negated test here, so... | 14:34.38 |
| sebras | Lock ordering violation: Attempt to take lock 1 when 0 held already! | 14:34.38 |
| | Attempt to release lock 0 when not held! | 14:34.38 |
| | those are the two first errors. | 14:34.45 |
| | where? | 14:34.50 |
| | oh.. in fz_defer_reap_end(). | 14:36.57 |
| | if reap_count == 0 we should be reaping, not otherwise. | 14:37.08 |
| | . | 14:37.10 |
| Robin_Watts | yes. | 14:45.01 |
| sebras | even if I change that I still get the locking issues though. | 14:45.40 |
| Robin_Watts | sebras: Yeah. Stupid error here. | 14:45.54 |
| sebras | Robin_Watts: I wish I would pick up on that in the review. :-/ | 14:46.47 |
| Robin_Watts | ok, NOW I get the locking order stuff. | 14:48.33 |
| sebras | Robin_Watts: ok, why didn't you see it? | 14:48.48 |
| Robin_Watts | stupid error. running the wrong binary, | 14:49.01 |
| sebras | :-D | 14:49.13 |
| | when testing the luratech stuff I'm in constant fear of doing exactly that. | 14:49.48 |
| Robin_Watts | I *could* defer start/end around every fz_drop_storable. | 15:05.27 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=74e6990a097dba4bd4040feb4d340b0d0dc60c42 | 15:06.48 |
| | That now runs with no warnings for me. | 15:06.59 |
| sebras | Robin_Watts: are we allowed to unlock ALLOC and REAP in arbitrary order? | 15:13.45 |
| | in defer_reap_end() you lock first alloc and then reap then, in case of reaping, unlock alloc and then reap. | 15:14.04 |
| Robin_Watts | sebras: Yes, unlocking can be done in any order. | 15:14.05 |
| | Yup. | 15:14.13 |
| | The only restriction that we apply is that locks should always be taken in decreasing order. | 15:14.43 |
| | (i.e. that lock n should never be taken if lock m is already held (for m <= n)) | 15:15.04 |
| | And that is sufficient to guarantee safety. | 15:15.15 |
| sebras | ah, this is why you moved REAP up a bit..? | 15:15.23 |
| Robin_Watts | sebras: Yeah. | 15:15.32 |
| sebras | Robin_Watts: nothing clears to clear store->needs_reaping | 15:17.29 |
| | I guess that should be done somewhere close to unlock = 0; ? | 15:17.40 |
| Robin_Watts | sebras: ooh. | 15:17.42 |
| sebras | or possibly inside do_reap(). | 15:17.44 |
| | I think inside do_reap() makes more sense. | 15:17.57 |
| | now I can't find more. :) | 15:18.53 |
| Robin_Watts | No, not inside do_reap. | 15:19.30 |
| | actually, yes, why not. | 15:19.54 |
| | sebras: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=db3db17a3c5b3a8c721ffcaf0abf38fbba634a4a | 15:27.24 |
| sebras | Robin_Watts: LGTM. | 15:30.24 |
| Robin_Watts | Thanks. | 15:30.48 |
| sebras | Robin_Watts: I missed the check for needs_treaping in fz_defere_reap_end(). I should have realized that one. | 15:30.56 |
| Robin_Watts | So... why when I run my tests through the cluster does it tell me that Bug693416.pdf has *Started* timing out ? | 15:40.53 |
| | hmm. But only in the non displaylist modes. | 15:41.26 |
| | Urgh... I see why. | 15:42.51 |
| sebras | Robin_Watts: what did we miss this time? | 15:43.08 |
| Robin_Watts | sebras: Nothing. It's just that with PDF files inline images are created, rendered, dropped when no displaylist happens. | 15:43.50 |
| | So they don't get batched. | 15:43.55 |
| sebras | Robin_Watts: right. | 15:44.05 |
| | Robin_Watts: so we batch around the content stream processing? | 15:44.19 |
| Robin_Watts | mmm. | 15:44.26 |
| sebras | the fact that we can't figure out all these cases worries me. | 15:45.09 |
| | what other cases exist that we haven't thought of/tested yet? | 15:45.18 |
| Robin_Watts | sebras: Well, this area has been a massive thinko for ages. | 15:45.53 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=9d430e88deff3e7f0c88ae14edfd0f11331fb53d | 15:57.21 |
| | That passes the cluster now. | 15:57.27 |
| | sebras: And http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=352efd644b2398f11f74e267f262f05969752141 (my bmpcmp shows the progressions) | 16:02.29 |
| | lots of problems with the files still, but the corners look right. | 16:02.44 |
| sebras | Robin_Watts: still bandwidth constrained so I'll refrain from looking at the bmpcmp | 16:03.06 |
| Robin_Watts | pops out. back inna bit. thanks. | 16:03.28 |
| sebras | Robin_Watts: code LGTM. | 16:04.09 |
| | Forward 1 day (to 2016/09/20)>>> | |
Log of #ghostscript at irc.freenode.net.