| | <<<Back 1 day (to 2018/06/21) | 20180622 |
| tor8 | paulgardiner: "Allow signature saving using pdf_write_document" LGTM | 11:07.37 |
| paulgardiner | Great. Thanks Tor | 11:07.56 |
| Robin_Watts | tor8: Ok, so I'm gradually popping jobs off the stack here... | 13:38.02 |
| | are you still waiting for me to review stuff on tor/master? | 13:38.16 |
| tor8 | Robin_Watts: yes. | 13:46.55 |
| Robin_Watts | I can live with the first few, but I'm looking at "Create appearance streams for annotations" now. | 13:52.35 |
| | pdf_parse_default_appearance | 13:52.56 |
| | You only look at the first 100 chars of the given da. | 13:53.14 |
| | And then you're assuming it's got spaces in. | 13:53.35 |
| | rather than \n or \t or \r etc. | 13:53.46 |
| tor8 | yes. it's very hacky. | 13:53.50 |
| Robin_Watts | or even stuff like /Tf/Fred | 13:54.02 |
| | These are things we'll be reading from a file, rather than our own generated streams, right? | 13:54.53 |
| tor8 | the DA string is ... problematic | 13:55.25 |
| | the spec says to copy&paste it verbatim when creating new appearance streams | 13:55.38 |
| | but the files adobe create will not work if that approach is taken | 13:56.01 |
| | /Tf/Fred cannot occur. "/Fred Tf" can | 13:56.39 |
| | the only case where we can have a non-whitespace separated token is between a keyword and the /fontname | 13:57.20 |
| Robin_Watts | Tf/Fred could occur though. | 13:57.21 |
| | pdf_print_default_appearance doesn't send a color if it's black. | 13:58.31 |
| tor8 | the default color is black | 13:58.44 |
| Robin_Watts | can we ever have to use pdf_print_default_appearance twice? (Once to set it to something other than black, then once to set it to black again) ? | 13:58.59 |
| | (I've not got a feeling for this stuff yet). | 13:59.16 |
| | Are we happy hardwiring rgb into stuff here? What if people want to be working in a different space? | 13:59.37 |
| tor8 | nope. it's only used to create a new /DA string for writing in the annotation object | 13:59.49 |
| Robin_Watts | Is it only ever called from pdf_set_annot_default_appearance? | 14:00.37 |
| | (If so, should it be static?) | 14:00.44 |
| tor8 | newer adobe versions don't use the DA, they use the CSS syntax DS instead, which only supports RGB color synatx | 14:01.58 |
| Robin_Watts | ok. | 14:02.20 |
| tor8 | it's currently also used in the form code; given some reshuffling of that code I might be able to make it static | 14:03.21 |
| | but I haven't started digging deeply into that yet | 14:03.30 |
| | pdf_field_set_text_color also touches the DA string | 14:03.42 |
| Robin_Watts | ok. | 14:04.09 |
| tor8 | and I've never seen a freetext annotation with non-rgb or non-grayscale colors; I figure we can deal with it when we see it. | 14:04.23 |
| | Robin_Watts: it might help if you just look at the final pdf-appearance.c than try to make sense of the diff | 14:08.53 |
| Robin_Watts | The very last chunk of that commit seems suspect. | 14:11.09 |
| | Commented out code. | 14:11.14 |
| tor8 | hm, yeah. | 14:12.10 |
| | signature appearances are on my TODO list | 14:12.18 |
| | but the place where this is called is suspect to begin with, but I expect that's because it's where it is feeding in the string to print in it. | 14:12.52 |
| | I'll make a stub for it and fix the commented out code | 14:13.04 |
| Robin_Watts | Add a /* FIXME: This is disabled pending signature appearance rewrite */ or something. | 14:13.16 |
| | or that. | 14:13.32 |
| | OK, I'm up to the gl ones, and finding nothing new to complain about. | 14:24.36 |
| | I can't claim that it's a perfect review, but it all looks plausible. | 14:24.48 |
| tor8 | cool. you can probably skip all the 'gl' ones if you want; I'm confident enough that they work well enough. | 14:25.17 |
| Robin_Watts | enum side side; ew. | 14:25.44 |
| | typedef enum { ... } side_t; | 14:26.09 |
| | side_t side; would be preferable IMAO. | 14:26.17 |
| | tor8: OK, so you're right. The gl ones look good in the depth to which I can be bothered to look at them (which is very shallowly indeed!) | 14:35.09 |
| tor8 | Robin_Watts: yeah. it's a rather large chunk of code to drop in your lap. if you want to go over it later, eventually, just do so with the latest code and we can tweak things from there. | 14:36.16 |
| | it's also got several somewhat subtle intricacies that aren't immediately obvious in certain ways it works | 14:36.42 |
| Robin_Watts | Yeah, it's certainly a large lump of code. I've avoided anything to do with the gl stuff so far, so hopefully that can stay the case :) | 14:37.34 |
| | So PDF strings are now held in pdf_obj's with 2 copies of the data? | 14:38.12 |
| tor8 | Robin_Watts: in the case where they are 'text strings' and used as such (which only really happens with annotation contents and widget values, and other display in a gui type strings) | 14:39.47 |
| | I figured holding on to the utf-8 converted string is easier on everyone than asking to try/catch alloc/free every time one is accessed | 14:40.42 |
| | and since the pdf_obj strings are immutable, we should never run into problems with the decoded copy being out of sync. | 14:41.18 |
| Robin_Watts | I'm still trying to understand that. give me a mo : | 14:42.22 |
| | :) | 14:42.24 |
| | So the decoded form is decoded just once and 'cached' in the pdf_obj. | 14:43.13 |
| tor8 | yes. | 14:43.27 |
| Robin_Watts | so any caller might be the first, and might have to cope with the failure case. | 14:43.44 |
| | oh, but they don't need to free-on-failure in their own code. | 14:44.13 |
| | And as you say, we can never change a pdf_string once we have it. We always make new ones, so the cached value never becomes stale. | 14:44.53 |
| | Are we ever going to get into the situation where we want to get "other than utf8" decoded strings out? Presumably not. We're adopting utf8 as our "decoded" format then ? | 14:45.56 |
| tor8 | nope. all our APIs for handling text is utf-8 | 14:46.15 |
| | and there is always the byte string access level for digging around non-text pdf strings | 14:46.28 |
| Robin_Watts | tor8: We should fix restrict rather than removing it. | 14:49.09 |
| | fz_restrict maybe. | 14:49.21 |
| tor8 | Robin_Watts: either way works for me. I did some benchmarks and there is no noticeable performance difference with the restrict keyword in a release build in the (admittedly few) tests I ran | 14:50.01 |
| | FZ_RESTRICT would be ideal (but SHOUTY like all macros) | 14:51.07 |
| Robin_Watts | ok, so other than the restrict commit, all lgtm. | 14:51.43 |
| | (and the FIXME/stub thing I mentioned earlier) | 14:51.57 |
| tor8 | running pdfref17.pdf at 600dpi, 44 vs 45 seconds | 14:51.59 |
| Robin_Watts | So 2% :) | 14:52.15 |
| tor8 | and on another file with more images, 35.7 vs 35.4 (so -1% slowdown) | 14:53.09 |
| | I'm happy to do a FZ_RESTRICT, definitely leaving them on the plotter functions | 14:53.29 |
| | Robin_Watts: a FIXUP commit for the stub and a tokenize thing on tor/master | 14:55.02 |
| Robin_Watts | fails to see how restrict can cause a slowdown :( | 15:00.58 |
| | It's "you can assume this extra thing if it will make you go faster". | 15:01.21 |
| tor8 | Robin_Watts: it probably can't ... just saying 2% is within normal runtime variance... | 15:01.34 |
| | machines are too complicated :( | 15:01.39 |
| | or sometimes code that in theory should be faster, ends up being slower | 15:02.04 |
| | Robin_Watts: http://git.ghostscript.com/?p=user/tor/mupdf.git;a=commitdiff;h=82a27c965a08ad610b865b8103cf09d67d2e76fb could you nod at that commit, then I'll do a final clusterpush and if nothing goes wrong I can push this | 15:04.13 |
| Robin_Watts | lgtm. | 15:04.51 |
| tor8 | Robin_Watts: Thanks. | 15:05.13 |
| Robin_Watts | I'll pull that in, and see if it solves Cody's issues. | 15:05.29 |
| | tor8: I get build failures here. | 15:18.02 |
| | NimbusSans-Oblique.cff no longer exists. | 15:18.17 |
| | just NimbusSans-Oblique.t1 | 15:18.52 |
| | should the t1's be in the repo ? | 15:19.05 |
| | Ah, should it now be NimbusSans-Italic ? | 15:20.00 |
| tor8 | Robin_Watts: oh crap. yes. URW changed the name of the font. | 15:36.38 |
| Robin_Watts | I'll fix here. | 15:44.12 |
| | And there is another build fix too. | 15:45.35 |
| tor8 | Robin_Watts: okay. | 15:45.54 |
| Robin_Watts | Should I squash the fixes in and push ? | 15:45.57 |
| tor8 | Robin_Watts: let me review the bmpcmp that's running first | 15:47.19 |
| Robin_Watts | Cody's problem still occurs. | 15:55.47 |
| | I'll dig into it. | 15:55.50 |
| tor8 | Robin_Watts: I'm happy with the bmpcmp results up to just before the URW font change. | 16:02.06 |
| | Robin_Watts: is your other build fix earlier than that or after? | 16:02.25 |
| Robin_Watts | earlier than that. | 16:02.32 |
| | See my pending branch. | 16:02.51 |
| | the build fix was just a variable-not-defined-at-the-start-of-a-block thing. | 16:03.14 |
| | tor8: In pdf_update_appearance... | 16:04.01 |
| | if (!ap_n) ... ap_n = pdf_new_xobject. | 16:04.23 |
| | Then pdf_dict_put_drop(xtc, ap, PDF_NAME(N), ap_n); | 16:04.37 |
| tor8 | right. go ahead and push up to "Size singleline widget text to fit both width and height.". | 16:04.37 |
| Robin_Watts | so that means the ap_n reference is now invalid. | 16:04.50 |
| | But you then fz_keep_obj it a few lines later. | 16:05.07 |
| tor8 | yeah... it's living on borrowed time | 16:05.24 |
| Robin_Watts | That's bad. M'kay. | 16:05.40 |
| tor8 | I can sort of agree with that. If you can reformulate it without tripping up on error handling, please do! :) | 16:06.51 |
| | in my defense however, everything we ever get from a pdf_dict_get also lives on the same borrowed time. | 16:07.48 |
| | if you added a ap_n = pdf_dict_get(ctx, ap, PDF_NAME(N)) right after we put_drop it, we'd be following our normal convention | 16:08.37 |
| Robin_Watts | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=5fa6149ef7facbcec1c9774700a9c7f33abf19b4 | 16:09.31 |
| moolc | Robin_Watts: _t prefix is reserved by POSIX | 16:09.58 |
| | suffix | 16:10.01 |
| Robin_Watts | POSIX can bite me. | 16:10.10 |
| | I shall reserve all names beginning with 'r' then. | 16:11.31 |
| moolc | Robin_Watts: POSIX shares the same sentiment | 16:11.31 |
| tor8 | Robin_Watts: LGTM. | 16:12.25 |
| Robin_Watts | tor8: Cool. Should that get rolled in somewhere? | 16:12.35 |
| tor8 | Nah. The code as was worked as intended, this is just cleaner code. | 16:12.58 |
| moolc | FWIW http://pubs.opengroup.org/onlinepubs/007908799/xsh/compilation.html | 16:13.13 |
| Robin_Watts | tor8: Cody's code is trying to create an appearance stream for Btn widgets. | 16:20.00 |
| | tor8: Bah. My fix is broken. | 16:43.33 |
| | I have a fixed fix :) | 16:45.58 |
| | tor8: pdf_add_stream... what does that do with 'obj'? | 16:53.31 |
| | Presumably it doesn't have to drop the reference or anything? | 16:54.11 |
| | tor8: OK, so top commit here solves Cody's leaks: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=shortlog;h=refs/heads/pending | 17:05.33 |
| tor8 | it adds the buffer to a stream, with an optional dictionary, and returns a new indirect reference object. | 17:05.48 |
| Robin_Watts | tor8: What remains to be done to get the rest in? | 17:06.02 |
| tor8 | Robin_Watts: that commit LGTM. | 17:07.03 |
| | I just want to review the bmpcmp of the font update before pushing. the rest is good to go. | 17:07.17 |
| Robin_Watts | Can I rebase the font update to the end? | 17:07.31 |
| tor8 | yes, that should work | 17:07.40 |
| Robin_Watts | tor8: Cody's latest bug is interesting. | 17:15.27 |
| | gs accepts the file with no problems, we claim that the flate filter is broken. | 17:15.38 |
| Robin_Watts | steps back to gs to answer a question for Steve for a bit. | 17:16.21 |
| | Forward 1 day (to 2018/06/23)>>> | |
Log of #mupdf at irc.freenode.net.