| | <<<Back 1 day (to 2016/09/22) | 20160923 |
| sebras | Robin: I'm not sure I like the changes in PDFObject_getDictionary() | 02:38.09 |
| | Robin_Watts: I want as few policy decisions in the JNI-bindings as possible. | 02:38.27 |
| | Robin_Watts: so if the underlying pdf_dict_gets() accepts NULL and returns "" or NULL or throws I want the JNI-bindings to behave accordingly. | 02:38.50 |
| | Robin_Watts: I also want to avoid having them to change in lockstep. | 02:38.59 |
| | Robin_Watts: now you changed it so that if name is null the JNI-binding itself will decide that returning null is the best alternative. | 02:39.45 |
| | Robin_Watts: and pdf_dict_gets() current decides to return NULL if either the object or the key is NULL or if the key cannot be found. | 02:41.51 |
| | Robin_Watts: also I'd like to see the old to_PDFObject_safe() removed and to_PDFObject_null() renamed to to_PDFObject_safe(). | 02:49.33 |
| | Robin_Watts: it seems like it could be done. | 02:49.43 |
| | Robin_Watts: hm... if obj == NULL then to_PDFObject_null() will call the constructor to create a new PDFObject whose .pointer == 0, but PDFObject.isNull will return false because NULL != PDF_OBJ_NULL. | 02:52.03 |
| | Robin_Watts: so if obj == NULL we need to call pdf_new_null and put that as an argument to NewObject(). | 02:53.23 |
| | Robin_Watts: maybe I'm missing something. | 02:54.10 |
| ago | Robin_Watts: why 697020 is a duplicate of 697022 ? they are 2 different issues but fixed by the same commit. | 07:14.38 |
| sebras | ago: I might have pointed to the wrong big, sorry. | 07:23.34 |
| | Bug. | 07:23.39 |
| ago | sebras: good morning | 07:27.58 |
| Robin_Watts | sebras: I agree about NULL. | 08:38.05 |
| | I don't see how null.isNull can ever have worked though. | 08:38.17 |
| | tor8 has suggested that we try to only keep a single null object around, and I vaguely like that idea. | 08:38.39 |
| | I have to pop out for a bit. Talk more in 90 mins or so maybe? | 08:39.01 |
| tor8 | sebras: PDF_OBJ_NULL == NULL | 09:36.22 |
| | oh, wait, no it isn't | 09:36.32 |
| | it's *after* the strings | 09:36.54 |
| | my suggestion was to add a static final PDFObject Null field in PDFObject | 09:37.28 |
| | so PDFObject.Null is the 'null' object with pointer field PDF_OBJ_NULL | 09:37.42 |
| | Robin_Watts: I recently changed something with pdf_obj that exposed a few bugs and broke some tools | 09:38.16 |
| | where code was trying to copy null dictionaries | 09:40.37 |
| | it was before my vacation so I can't remember what it was, but it feels like it should be important for this discussion :( | 09:41.11 |
| sebras | tor8: right. Since robin started doing something about this in mupdf_native.c I'll let him finish though. | 09:53.08 |
| | no point in having me compete in the code with _both_ of you at the same time. | 09:53.50 |
| tor8 | sebras: PDFDocument extends Document | 09:55.26 |
| | sebras: PDFPage extends Page, and PDFPage.createAnnotation still don't exist? | 09:55.39 |
| sebras | tor8: nope, true. I want to have a look second look at the fuzzing bugs from yesterday evening though since ago asked me. | 09:56.47 |
| tor8 | yes, crash bugs have the highest priority | 09:57.02 |
| sebras | ago: I actually do consider the two bugs 697020 and 697022 to be the same since when I run your file in mujstest using valgrind I get a complaint at the exact same location. | 10:05.14 |
| | ago: also the fix is identical: to remove the otherwise unused global filename variable. | 10:06.11 |
| Robin_Watts | sebras: Hi back. | 10:11.39 |
| sebras | Robin_Watts: hi. | 10:12.12 |
| | Robin_Watts: me too. | 10:12.13 |
| Robin_Watts | sebras: I'm happy to make changes to the JNI code, but you've very much refined the style in there, so I want to be sure I'm consistent with your intentions. | 10:12.26 |
| | just looking over your comments again to try to completely understand them. | 10:13.58 |
| sebras | Robin_Watts: the only thing I noticed that you and I did differently were some if (!ctx) return NULL; if (!something) return NULL; I think I combined then into one test in the other functions. | 10:15.43 |
| Robin_Watts | sebras: No. | 10:16.08 |
| | or at least, I don't think so. | 10:16.18 |
| | in PDFObject_getDictionary you used to do: | 10:16.29 |
| | if (!ctx || !jname || !dict) return NULL; | 10:16.39 |
| | I now do: if (!ctx || !jname) return NULL; if (!dict) return self; | 10:16.57 |
| | cos if self is a null (but not NULL) object on entry, we should return the same. | 10:17.27 |
| | Or is there something else ? | 10:17.41 |
| sebras | Robin_Watts: oh! I read self as NULL yesterday evening! :) | 10:17.53 |
| Robin_Watts | sebras: You say you want to move to_PDFObject_safe to always do what to_PDFObject_null does, and I think I have no objections to that. | 10:19.51 |
| sebras | Robin_Watts: but wait... how does that make sense? I have a pdf array object and do get(42) on that array. now the array object is a null object in reality so we return self because it happens to be null object? | 10:20.35 |
| | Robin_Watts: in that case i agree with tor8 that it's better to have a global PDFObject.Null that can be returned instead. | 10:20.54 |
| Robin_Watts | sebras: Yes, I want us to have a global null object. | 10:21.12 |
| | but frankly any null object is equal to any other null object. | 10:21.23 |
| | So we need to make to_PDFObject_safe return the global null. | 10:22.24 |
| sebras | Robin_Watts: true, which is why we do if (!ctx) on our pointers in C... ;) | 10:22.33 |
| | Robin_Watts: yeah, that would make sense I think. | 10:23.11 |
| | Robin_Watts: I did mention that loadPage and getTrailer() might need to be changed if you update to_PDFOBject_safe(), right? | 10:23.40 |
| Robin_Watts | sebras: If you did, I had missed it. | 10:24.11 |
| | why loadPage? | 10:24.46 |
| sebras | Robin_Watts: findPage(), sorry. | 10:25.15 |
| Robin_Watts | Why findPage? :) | 10:25.37 |
| | If we lookup a page object and we can't find an object for it, we return the null object.... | 10:27.06 |
| sebras | Robin_Watts: yes, and I'm not sure if that is a good or a bad idea. | 10:27.24 |
| | Robin_Watts: is it better to reutrn NULL? | 10:27.30 |
| | Robin_Watts: pdf_lookup_page_obj() doesn't return any null object if it cannot find the page..? | 10:27.58 |
| Robin_Watts | sebras: It's better to return a null PDFObject, cos then people can do things like findPage(1).get("Foo") etc without needing to error check. | 10:28.21 |
| tor8 | pdf_lookup_page_obj throws if it can't find | 10:28.26 |
| Robin_Watts | oh, so we'll throw too in the java and never reach to_PDFObject_safe | 10:28.56 |
| tor8 | Robin_Watts: btw, what you said yesterday about dict.get("DoesntExist") returning 'dict' itself would be very dangerous | 10:28.57 |
| Robin_Watts | tor8: Only if dict is the null object. | 10:29.09 |
| | cos all null objects are equivalent. | 10:29.34 |
| tor8 | oh! phew. | 10:29.36 |
| sebras | if that's the case with pdf_lookup_page_obj() then we cannot distinguish missing pages from syntax errors. | 10:29.49 |
| | do we want to treat them differently in Java? I'm guessing yes...? | 10:30.02 |
| Robin_Watts | sebras: We want the java to reflect the C, and I believe it should. Are you saying the C behaviour is bad? | 10:30.26 |
| tor8 | Robin_Watts: sebras: I think anywhere the current pdf_obj code returns NULL, the JNI should return PDFObject.Null if we're going to do anything about this at all | 10:31.15 |
| sebras | Robin_Watts: if the C version throws both for missing pages and syntax errors then shouldn't the Java code do the same? | 10:31.17 |
| Robin_Watts | sebras: And I believe it will. Where do you believe it won't? | 10:31.37 |
| tor8 | and where the C code throws, we'll also throw | 10:31.44 |
| | pdf_lookup_page_obj throws an exception if the page can't be found so will never in practice return NULL. it may return the 'null' PDF object if the PDF is maliciously misconstructed, but that's okay. | 10:32.27 |
| sebras | Robin_Watts: because as tor8 wrote above if pdf_lookup_page_obj() throws for pages it cannot find then we'll never reach to_PDFObject_safe() and then we will never return the null-object. | 10:32.29 |
| Robin_Watts | if pdf_lookup_page_obj throws, so will PDFDocument_findPage. We'll never reach the to_PDFObject_safe, so we will never return null. | 10:32.39 |
| tor8 | I'm arguing that to_PDFObject_safe/null should be the same | 10:33.02 |
| Robin_Watts | sebras: Right, so the C and the Java will do the same thing. | 10:33.07 |
| sebras | tor8: I agree. :) | 10:33.09 |
| Robin_Watts | tor8: The plan is to make to_PDFObject_safe do what to_PDFObject_null does in my new code, and lose to_PDFObject_null. | 10:33.37 |
| sebras | maybe I misunderstood, but at first I assumed that you advocated for removing the jni_throw and returning a null object instead..? | 10:33.42 |
| Robin_Watts | So I think we're all in agreement. | 10:33.43 |
| tor8 | Robin_Watts: good :) | 10:33.47 |
| Robin_Watts | sebras: I had misread the code, but swiftly reversed my position :) | 10:33.58 |
| tor8 | and use a global Null object so we don't need to call New so much? | 10:34.06 |
| Robin_Watts | tor8: Yes, just got to figure that out. | 10:34.20 |
| sebras | tor8: sounds good to me. | 10:34.20 |
| Robin_Watts | If we have a private static final PDFObject Null = new PDFObject(0); | 10:34.59 |
| | i.e. a global null object... | 10:35.09 |
| | and I look that up at JNI startup, will that have been setup before the JNI runs? | 10:35.33 |
| tor8 | might be able to get it using a getField call | 10:36.01 |
| Robin_Watts | Yeah, I'd need to getReference each time I think, but the field lookup should be safe. | 10:36.32 |
| | Ok. I will rejig my commit and then you can poke holes in it in a bit. Thanks. | 10:36.57 |
| tor8 | GetStaticFieldID and GetStaticObjectField | 10:37.16 |
| | sebras: how far did you get with the reviews on tor/master yesterday/ | 10:41.03 |
| sebras | tor8: up to 92132c0 looks fine | 10:42.48 |
| | tor8: the clean up annot commit. | 10:42.59 |
| | tor8: the js-bindings I don't understand yet. | 10:43.15 |
| | tor8: and the tmp/deleted/changed annot list looked fine. | 10:43.33 |
| | tor8: now that Document.isPDF() exists though. does that mean the Document.isUnencryptedPDF() can move down into PDFDocument? | 10:44.24 |
| tor8 | I suspect the tmp/deleted/changed commit may break the android/iOS viewer subtly somehow | 10:44.37 |
| | but since we're focusing on the newer viewers I wouldn't worry too much, but we should make sure to test them thoroughly before releasing | 10:45.06 |
| | isUnencryptedPDF doesn't exist in JS | 10:45.40 |
| sebras | tor8: should it? | 10:45.48 |
| | tor8: I think fred added/asked for it in Java. | 10:46.03 |
| tor8 | needsPassword, authenticatePassword, hasPermission, and getMetaData should be more than sufficient | 10:46.53 |
| | I don't understand why we would want such a specific 'isUnencryptedPDF' function | 10:48.42 |
| sebras | tor8: the old viewer had it. | 10:48.59 |
| | tor8: I'm guessing that's the reason it was being asked for. | 10:49.11 |
| tor8 | the old viewer uses it to check whether we can annotate the file and it should show the Annotate button | 10:49.37 |
| | hasPermission(ANNOTATE) should be used instead | 10:49.46 |
| | or even isPDF && hasPermission(ANNOTATE) | 10:49.57 |
| | so delete it | 10:50.07 |
| sebras | tor8: ok. | 10:50.14 |
| | tor8: btw, isPDF in Java is that instanceof or IsInstanceOf() in the JNI or is it a JNI-binding checking if pdf_specifics() returns TRUE? | 10:53.26 |
| tor8 | isPDF can be pure java, using instanceof operator | 10:54.06 |
| sebras | tor8: would instanceOf for a null object be TRUE?! | 10:54.07 |
| tor8 | since when we create the java object wrapper, we check using pdf_document_from_fz_document (same as pdf_specifics, but with a more explicit name that also works for pdf_page and pdf_annot) | 10:55.09 |
| sebras | tor8: so basically in Document I'd write public boolean isPDF() { return this instanceof PDFDocument; } ? | 10:58.18 |
| | tor8: that doesn't compile because Document cannot be converted to PDFDocument claims javac. | 10:58.43 |
| Robin_Watts | public boolean isPDF() { return this.to_PDFDocument() != null; } | 10:59.12 |
| tor8 | sebras: you'll need to make PDFDocument a subclass of Document first | 10:59.52 |
| | sebras: or cheat. in Document isPDF() { return false; } in PDFDocument override it with { return true; } | 11:00.41 |
| Robin_Watts | having PDFDocument be a subclass of Document is superficially attractive. | 11:01.33 |
| | but then we run the risk of people doing new PDFDocument("fred.xps"); | 11:01.52 |
| | For years, I have used 'fred' as an example variable, etc cos it's quick to type. I now find that this habit may cause confusion. | 11:02.40 |
| | I vote we get Mr Ross-Perry to change his name for the sake of my muscle memory. | 11:03.00 |
| tor8 | IMO, the only public PDFDocument constructor should be one that takes no arguments and creates a blank PDF document | 11:03.25 |
| sebras | Robin_Watts: I felt the same a few moments ago when I read ago's alias. | 11:03.32 |
| tor8 | if we're going with the subclass | 11:03.35 |
| sebras | tor8: I'm not sure you're allowed to do that in the subclass. | 11:03.55 |
| Robin_Watts | yeah, what sebras said. | 11:05.03 |
| tor8 | then we can make a PDFDocument() constructor that throws an IllegalArgumentException... | 11:05.08 |
| Robin_Watts | That's the downside of subclassing. | 11:05.12 |
| tor8 | PDFDocument(String noYouDont) | 11:05.27 |
| | or we keep the toPDFDocument hack | 11:05.50 |
| | Robin_Watts: actually, if making subclasses we *can't* keep new Document("fred.xps") constructor at all | 11:06.21 |
| | it MUST be Document.openDocument("fred.xps") or we won't be able to construct the proper subclass | 11:06.47 |
| Robin_Watts | Oh, yes. | 11:07.01 |
| | So maybe that removes my objection. | 11:07.06 |
| | That does seem more in keeping with java. | 11:07.20 |
| tor8 | and Document.createPDFDocument() for a blank PDF document, then we can keep *all* Document and PDFDocument constructors private | 11:07.40 |
| | to reduce the risk of confusion | 11:07.45 |
| | JS still has constructors, but JS is a *lot* more flexible in what gets returned from a 'new Foo()' constructor | 11:08.09 |
| | though I do sort of like the idea of new PDFDocument() to create a blank | 11:08.56 |
| Robin_Watts | Urgh. | 11:38.02 |
| | null pdf objects in the C aren't NULL. | 11:38.12 |
| | If I mirror that in the java, then from_PDFObject_safe doesn't need to cope with NULL pointers. | 11:40.10 |
| tor8 | Robin_Watts: yeah, the C code makes a distinction between NULL (non-existent) and the 'null' PDF object | 11:43.50 |
| Robin_Watts | Then the java will do the same. | 11:44.19 |
| tor8 | they both behave identically though | 11:44.25 |
| | and I'm not sure the distinction is meaningful | 11:44.30 |
| | other than querying for whether a dict has a specific key | 11:44.47 |
| Robin_Watts | In our JNI code elsewhere, pointer=0 means 'object has been destroyed'. | 11:45.44 |
| | So it would be nice to be consistent. | 11:45.48 |
| tor8 | if we give up on the idea that we should be able to do fee.get("Fie").get("Foe").get("Foo").asReal() without checking that all the links in the chain exist | 11:46.12 |
| | I'd make toPDFObject_safe return the PDF_OBJ_NULL object for pointer 0 | 11:46.51 |
| | and add a dict.has(entry) function for when you really need the distinction | 11:47.20 |
| | biab, lunch | 11:47.55 |
| Robin_Watts | We really don't want to give up on the fee.get("Fie").get("Foe").get("Foo").asReal() idea. | 11:48.22 |
| | tor8, sebras: OK, here is the result of my hacking. Let me know what you think. | 12:46.40 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=221e28469b74a65363be8f5da20b81fd7090ea01 | 12:46.42 |
| | In the end NULLs aren't nulls. They are destroyed objects. | 12:47.08 |
| | so the changes are much smaller. | 12:47.15 |
| tor8 | Robin_Watts: speaking of, all but one of the pdf_new_xxx stuff don't really need a pdf_doc argument | 12:51.50 |
| | pdf_new_indirect is the only one where we need a pdf_doc | 12:52.06 |
| Robin_Watts | tor8: Yes. But ISTR you arguing for them all taking it for consistency. | 12:52.29 |
| tor8 | if we drop that argument, we can simplify a fair bit of the pdf annotation editing code, where we need to dig around to get a pdf_document to create dictionaries | 12:52.44 |
| | Robin_Watts: I think a lot of that was back when we used pdf_document as a stand-in for fz_context | 12:53.03 |
| Robin_Watts | tor8: Ah, maybe. | 12:53.10 |
| | I can't come up with a convincing argument against dropping them, other than "oh god, more API upheaval". | 12:53.37 |
| tor8 | indeed | 12:53.44 |
| | public static final PDFObject Null = new PDFObject(newNull) and we can drop the private PDFObject() constructor | 12:55.17 |
| | new PDFObject(newNull()) | 12:55.27 |
| | and newNull should probably be a static method too | 12:55.37 |
| Robin_Watts | oh, nicer, yes. | 12:55.43 |
| | will fix. | 12:55.53 |
| tor8 | for that to work | 12:55.59 |
| Robin_Watts | better version then: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=705fc73ffa42f92b82ec0a6088149f1643fd221c | 13:04.57 |
| Robin_Watts | lunches. | 13:05.03 |
| tor8 | Robin_Watts: yeah, looking good | 13:05.58 |
| | if (intent == null || !intent.isDictionary()) should probably be changed | 13:06.49 |
| | if (!intent.isDictionary()) should suffice | 13:07.02 |
| | knowing that foo.get(i) will never return null | 13:07.14 |
| | Robin_Watts: getEmbeddedProfileName, at the end you call toString where I suspect you mean asString | 13:18.54 |
| Robin_Watts | Hmm. intent.get("Info").asString(). What will that return for "Info" not existing? | 14:02.52 |
| | get("Info") would return the null object. | 14:03.08 |
| | Do we want null.asString() to "" or null ? | 14:03.30 |
| | s/to/to be/ | 14:03.51 |
| | In C we return "". | 14:04.55 |
| tor8 | in C we return "" | 14:05.40 |
| | ahem, echo chamber :) | 14:05.52 |
| | in C we return "" so we don't segfault on any strcmp's etc afterwards | 14:06.12 |
| Robin_Watts | We currently do the same in JNI. | 14:06.38 |
| | but it means my code that tests for id != null is wrong | 14:06.55 |
| | it should be !id.equals("") | 14:07.07 |
| tor8 | idObj = intent.get("Info").get("ID") | 14:07.31 |
| | if (idObj.isString()) do stuff with idObj.asString(); else null-case | 14:07.47 |
| | is how I'd structure the code | 14:07.57 |
| Robin_Watts | tor8: Yeah, except that's more typing. | 14:09.07 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=5f3ea2adb4058c0b560813542fd153c07a798522 | 14:09.51 |
| tor8 | it's java, you're supposed to type a lot ;) | 14:11.02 |
| | String id = intent.get("Info"); if (id.isString()) return id.asString(); etc. | 14:11.43 |
| | very little extra typing, and fewer magic constants (!id.equals("") is magic to me) | 14:12.05 |
| | in fact, it's probably less typing... | 14:12.41 |
| Robin_Watts | ok. | 14:28.29 |
| | http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=fded387b22029e94a59c17dba23a3b42ff344cdd | 14:29.52 |
| | If you're happy with that, I can pull freds commits in and rebase them on top. | 14:32.46 |
| tor8 | Robin_Watts: LGTM | 14:38.16 |
| Robin_Watts | Thanks. | 14:38.21 |
| sebras | reads logs. | 14:59.10 |
| | Robin_Watts: : | 15:07.37 |
| Robin_Watts | sebras: ? | 15:08.00 |
| sebras | Robin_Watts: if (!jname) name = (*env)->GetStringUTFChars(env, jname, NULL); | 15:08.38 |
| | Robin_Watts: is that safe? | 15:08.42 |
| | Robin_Watts: aren't we testing for NULL to avoid sending NULL? and now we send it intentionally? | 15:09.10 |
| Robin_Watts | Crap typo. | 15:09.16 |
| | Will fix. Thanks. | 15:09.24 |
| | Did I mention I hate if (!ptr) ? | 15:10.00 |
| sebras | Robin_Watts: you did. | 15:10.08 |
| | Robin_Watts: I'm still reviewing. | 15:11.05 |
| Robin_Watts | sebras: Sure. | 15:11.15 |
| | 5 more commits on robin/master, including a fix for that. I'll roll fixes for anything else you find into them. | 15:11.51 |
| | 4 of them are freds (or rebased versions of freds at least). | 15:12.12 |
| sebras | Robin_Watts: didn't you and tor8 agree that we should return "" if the arguments are NULL? | 15:12.29 |
| Robin_Watts | sebras: Can you be more specific? | 15:12.50 |
| sebras | Robin_Watts: PDFObject_asString() appears to return NULL if obj == NULL. | 15:12.59 |
| Robin_Watts | obj should never be null now. | 15:13.13 |
| sebras | Robin_Watts: oh... obj is self. | 15:13.15 |
| Robin_Watts | null = destroyed. | 15:13.22 |
| sebras | Robin_Watts: yeah, sorry for the noise. | 15:13.32 |
| Robin_Watts | np. | 15:13.46 |
| sebras | Robin_Watts: hm... I just want to confirm my reasoning. we do want to_PDFObject_safe_own() to return NULL upon obj == NULL because we want e.g. PDFDocument.createObject() to return NULL if pdf_create_object() fails. | 15:17.31 |
| | well, if it returns NULL. | 15:17.39 |
| | the same would go for PDFDocument.addPageBuffer(). | 15:17.58 |
| Robin_Watts | sebras: presumably we throw in those cases? | 15:18.16 |
| | i.e. ind should never be NULL in the last line of PDFDocument_createObject | 15:19.07 |
| sebras | Robin_Watts: it depends on what the C level APIs do. | 15:19.30 |
| Robin_Watts | pdf_new_indirect throws on failure. | 15:19.52 |
| sebras | Robin_Watts: that function, to_PDFObject_safe_new(), is used in several place. | 15:20.40 |
| Robin_Watts | pdf_graft_object looks like it throws on failure. | 15:20.55 |
| sebras | Robin_Watts: but all seem to be adding new object and then that behaviour is like waht we want. | 15:21.04 |
| Robin_Watts | pdf_add_stream looks unsafe to me. | 15:21.26 |
| sebras | Robin_Watts: how so? | 15:21.43 |
| Robin_Watts | tor8: shouldn't the catch in pdf_add_stream rethrow? | 15:21.48 |
| | (or set ind = NULL, but I prefer rethrow) | 15:22.09 |
| sebras | Robin_Watts: yes, it should. we can't drop and then return ind! | 15:22.15 |
| | Robin_Watts: agreed. | 15:22.21 |
| Robin_Watts | sebras: I believe that in every case where we call to_PDFObject_safe_own you should never get ind = NULL. | 15:30.10 |
| sebras | Robin_Watts: yeah, I tend to agree. | 15:30.27 |
| Robin_Watts | OK, another commit on robin/master with that rethrow added. | 15:31.46 |
| sebras | Robin_Watts: ok, I canb't | 15:34.37 |
| | can't find anything further. | 15:34.43 |
| Robin_Watts | Fab. | 15:35.05 |
| sebras | Robin_Watts: wait! | 15:35.27 |
| | Robin_Watts: I haven't looked at the new patches. | 15:35.34 |
| | Robin_Watts: just so you don't misunderstand me. :) | 15:35.43 |
| Robin_Watts | no, was waiting for either you or fred to look at them :) | 15:35.49 |
| sebras | Robin_Watts: two first patches on robin/master are ok at least. | 15:36.26 |
| Robin_Watts | sebras: Fred has OKd my rejigs of his commits, which means both I and him have looked at them. | 15:44.34 |
| | If you want to look too, great, but otherwise I think we can count them as reviewed. | 15:44.57 |
| | i.e. don't feel pressured if you have better things to do. | 15:45.08 |
| sebras | Robin_Watts: hm.. I must be doing something wrong: /mupdf_native.c:2558:71: error: 'ANDROID_BITMAP_RESULT_SUCCESS' undeclared (first use in this function) | 15:48.05 |
| Robin_Watts | sebras: No, it might be me... | 15:48.31 |
| sebras | oh... th local.properties thing agina... | 15:48.38 |
| Robin_Watts | Bah. There is at least one typo in there. | 15:51.17 |
| sebras | Robin_Watts: ? | 15:52.20 |
| Robin_Watts | yeah, the gs invocation had typos in it. | 15:56.19 |
| | Fix commit for that online now. | 15:56.38 |
| | The problem came in when I rebased at the last moment. It's not in the commits you were just looking at. | 15:56.58 |
| sebras | Robin_Watts: https://developer.android.com/ndk/reference/group___bitmap.html shows that there's a macro to fix the issue..? | 15:57.01 |
| Robin_Watts | sebras: I see no issue. What is it that you're seeing? | 15:57.27 |
| sebras | Robin_Watts: because you are using a later version of the ndk then I am you have: | 15:59.26 |
| | #define ANDROID_BITMAP_RESUT_SUCCESS ANDROID_BITMAP_RESULT_SUCCESS | 15:59.29 |
| | where as I do not. | 15:59.31 |
| Robin_Watts | I am using r10d. | 15:59.48 |
| sebras | Robin_Watts: how do I check the verion? | 16:00.27 |
| Robin_Watts | sebras: How did you install it? Through AS ? | 16:00.43 |
| sebras | Robin_Watts: ah.. RELEASE.TXT says r8b | 16:00.43 |
| Robin_Watts | OK. | 16:00.48 |
| sebras | Robin_Watts: a looong time ago. I don't have AS. | 16:00.59 |
| Robin_Watts | sebras: Ah, you'll need AS to build freds example app. | 16:01.29 |
| | Presumably you queue up all your downloads for when you go to somewhere with wifi? :) | 16:01.59 |
| | sebras: So, are you happy for me to push those? Or are you still looking? | 16:02.54 |
| sebras | Robin_Watts: still looking. give me 2min. | 16:04.50 |
| Robin_Watts | Hmm. pdf strings can have 0 bytes in them, but pdf_to_ucs2 assumes null terminated. | 16:08.25 |
| | No, ignore me. | 16:09.32 |
| sebras | Robin_Watts: all Android example changes LGTM too. | 16:10.47 |
| Robin_Watts | sebras: Thanks. | 16:10.55 |
| sebras | Robin_Watts: trivial patch at sebras/master so I can build without installing AS for now. | 16:14.11 |
| Robin_Watts | sebras: We never check in local.properties. | 16:15.05 |
| | and I suspect if I commit that, fred won't be able to build. | 16:15.34 |
| fredross-perry | local.properties is not meant to be checked in | 16:15.57 |
| | it contains info about the local installation | 16:16.09 |
| sebras | Robin_Watts: *waves hand* those were not the the commits you are looking for... | 16:16.10 |
| Robin_Watts | Ah, the previous commit? | 16:16.36 |
| sebras | Robin_Watts: yeah, the JNI:-one. | 16:16.43 |
| | Robin_Watts: not the WIP-ones. | 16:16.46 |
| | Robin_Watts: those I keep around to make it easier for me to build on my computer. | 16:16.58 |
| | Robin_Watts: with my personal installation. | 16:17.04 |
| Robin_Watts | sebras: r8 is REALLY old. Can we not just persuade you to update? :) | 16:17.09 |
| sebras | s/personal/particular/ | 16:17.13 |
| Robin_Watts | Even r10d (which I use) is fairly old. | 16:17.23 |
| fredross-perry | What's the newest on that supports the oldest OS we claim to support? Maybe we should build/test with that? | 16:21.59 |
| Robin_Watts | fredross-perry: I can't make any recent one work for me. | 16:22.48 |
| | Trying to target old versions fails. | 16:23.07 |
| | (At least, I think that's why it fails). | 16:23.21 |
| fredross-perry | Like API 8? | 16:24.10 |
| Robin_Watts | fredross-perry: Yeah, when I let AS update the NDK I suddenly find that stuff I build using ndk-build won't link. | 16:25.23 |
| | Now, I am entirely prepared to believe it's me doing something wrong, but I haven't had the time/energy to invest in tracking it down yet. | 16:25.44 |
| | So I am keeping r10d around. | 16:25.55 |
| fredross-perry | you mean ndk-build results in link errors? | 16:26.16 |
| Robin_Watts | fredross-perry: Yes (subject to usual "I have a crap memory" disclaimers) | 16:26.55 |
| fredross-perry | Sorry, what was your name again? | 16:27.12 |
| Robin_Watts | I've got that on a piece of paper somewhere. | 16:27.36 |
| jogux | I can't remember what Robin's problem was, but the usual problem that ndk stupidness causes is a failure for the library to load on older devices, usually moaning about a math function like atof etc. | 16:30.04 |
| | Android is just a total mismash, it's worse than the mac app store xcode reverse-rusian roulette. :) | 16:31.45 |
| Robin_Watts | 1 more commit on robin/master to update all the JNI String creation to convert from PDFDocEncoding etc first. | 16:34.45 |
| | And another one updating some comments in headers. | 16:40.38 |
| sebras | Robin_Watts: still reviewing the first one. | 16:40.53 |
| fredross-perry | I'm suggesting we re-figure out what our min platform and ndk is, and then assert those at build time so that we and customers don't wander off. | 16:41.13 |
| Robin_Watts | fredross-perry: Our min platform is still 8, AIUI. Unless you've added stuff that needs a later version? | 16:42.26 |
| sebras | Robin_Watts: and this is unrelated to the NDK version, right? | 16:43.05 |
| Robin_Watts | sebras: It's not directly correlated, no. | 16:43.17 |
| sebras | Robin_Watts: well, there's one version that introduces API level 8, so snothing before that. | 16:43.24 |
| fredross-perry | probably. I can hunt those down. But, is 8 still what we want? How many 8 devices might still be out there in use? | 16:43.34 |
| Robin_Watts | sebras: For x86 and mips, we need API 9. | 16:43.40 |
| sebras | Robin_Watts: but even the latest version _ought_ to build using API level 8 (even if it might have issueS) | 16:43.43 |
| Robin_Watts | sebras: Indeed. | 16:43.51 |
| | fredross-perry: I have an API 8 device that I use daily. | 16:44.19 |
| | My nook :) | 16:44.23 |
| fredross-perry | outstanding. | 16:44.33 |
| sebras | Robin_Watts: does it use mupdf? | 16:44.50 |
| fredross-perry | I have a nook that I use for swatting flies. | 16:45.04 |
| Robin_Watts | paulgardiner had one that he used as an airbag. | 16:45.22 |
| sebras | Robin_Watts: you have to help me understand the text conversion stuff. | 16:46.35 |
| | Robin_Watts: NewString() takes jchar * which is an array of unicode chars. | 16:46.51 |
| | Robin_Watts: oh... jchar is 16bits! | 16:47.08 |
| | Robin_Watts: but wht encoding does the input string to pdfenc_to_unicode() have? | 16:48.07 |
| Robin_Watts | sebras: PDF strings are assumed to be in PDFDocEncoding. | 16:48.19 |
| | UNLESS they start with 2 specific bytes that are BOMs to indicate that the rest of the string is BE or LE unicode. | 16:48.52 |
| | Most of that code is lifted from pdf_to_ucs2, which is sadly in a form we can't reuse. | 16:49.33 |
| sebras | Robin_Watts: oh, and they are never UTF8 and we don't convert it anywhere? /me feels daft. | 16:51.16 |
| | Robin_Watts: that means this code was severely broken before. | 16:51.33 |
| Robin_Watts | sebras: They are never utf8, and I don't believe we ever convert it. | 16:51.38 |
| | sebras: We have made similar assumptions for years :) | 16:51.48 |
| | And even if they were utf8, they wouldn't have been javas not quite utf8 encoding. | 16:52.15 |
| sebras | Robin_Watts: right so we do pdf_to_utf8() when exporting the string outside of mupdf. | 16:52.30 |
| Robin_Watts | Yeah. | 16:52.44 |
| | pdf_to_utf8 will hit problems if the buffers contain 0's. | 16:54.03 |
| sebras | Robin_Watts: is this the time were we ignore you again? ;) | 16:55.39 |
| Robin_Watts | sebras: It is. | 16:55.53 |
| sebras | Robin_Watts: just checking.. | 16:56.09 |
| Robin_Watts | I don't propose we make any particular changes there at the moment. | 16:56.14 |
| sebras | Robin_Watts: in pdfenc_to_unicode(), where are you setting the trailing NUL? | 17:00.52 |
| Robin_Watts | I don't. No trailing NUL. | 17:01.12 |
| sebras | Robin_Watts: ok, so why srclen + 1 in the case of pdf_doc_encoding? | 17:01.32 |
| Robin_Watts | I return the length of the converted string, and that's passed into CreateString. | 17:01.33 |
| | cut and paste. | 17:01.45 |
| sebras | Robin_Watts: and in the case of UTF16-[BL]E? | 17:02.04 |
| Robin_Watts | paste and cut. | 17:02.34 |
| sebras | Robin_Watts: ok, those. | 17:02.43 |
| Robin_Watts | Updated version online. | 17:03.23 |
| sebras | Robin_Watts: JNI-patch LGTM. | 17:04.48 |
| Robin_Watts | Thanks. | 17:05.26 |
| | So, we have a staff meeting next thursday | 17:05.46 |
| | In Florida, which is GMT-4. | 17:06.23 |
| sebras | Robin_Watts: ok. | 17:06.37 |
| Robin_Watts | Hmm. That's UTC-5 vs UTC+8. | 17:07.32 |
| | So midday florida time will be 1am your time. | 17:08.12 |
| | That's not going to be helpful is it :) | 17:08.18 |
| sebras | Robin_Watts: yeah, so I'm guessing we'd pretty much be out of contact. | 17:08.32 |
| Robin_Watts | If you have anything you'd like to raise at the meeting, send an email to tech@artifex.com, I guess. | 17:08.51 |
| | Don't feel you have to, cos we're all pretty much up to speed on the work you've been doing. | 17:09.12 |
| | But if you have particular thoughts on things that you want to do over the next quarter, that might be good to get 'em read into the record, as it were. | 17:09.36 |
| sebras | Robin_Watts: you and tor8 definitely ought to be at least. | 17:12.30 |
| | Robin_Watts: besides JNI and misc small stuff you've seen I have mostly spent time on luratech. | 17:13.20 |
| | Robin_Watts: and that is still not finished. | 17:13.24 |
| | Robin_Watts: I'd like to reply to them with the remaining issues. | 17:13.46 |
| Robin_Watts | Yeah, and you found a bunch of problems they still haven't fixed. | 17:13.49 |
| | That'd be good. | 17:13.52 |
| sebras | Robin_Watts: reading/writing outside of buffers and also overlapping buffers to strcpy/memcpy. | 17:14.08 |
| | Robin_Watts: I didn't know what to use for testing so I used openjpeg for that. | 17:14.24 |
| | Robin_Watts: openjpegs testsuite. | 17:14.31 |
| | Robin_Watts: maybe we can merge that into the cluster? | 17:14.37 |
| Robin_Watts | Sure. | 17:15.00 |
| sebras | Robin_Watts: but all of those are not eliglible for embedding inside PDF accoridng to kens. | 17:15.07 |
| Robin_Watts | Can we run a given file from the command line? | 17:15.15 |
| sebras | Robin_Watts: I haven't looked into exactly which ones. | 17:15.23 |
| | Robin_Watts: sure. mutool draw -s t ought to work. | 17:15.34 |
| Robin_Watts | sebras: ah, so mudraw will open a standalone openjpeg file? | 17:15.59 |
| sebras | Robin_Watts: yes. | 17:16.21 |
| | Robin_Watts: both .jp2 and .j2k | 17:16.27 |
| Robin_Watts | Do you have a copy of their test suite? | 17:16.28 |
| | (that I can grab onto casper) | 17:16.39 |
| sebras | Robin_Watts: yeah, but it is better if you grab it directly from github..? | 17:16.54 |
| kens | sees my name go past...... | 17:17.03 |
| sebras | Robin_Watts: https://github.com/uclouvain/openjpeg-data | 17:17.19 |
| kens | Oh, not all JPEG2K images are embeddable ? | 17:17.24 |
| sebras | kens: you mentioned that not all .jp2 and .j2k can be embedded in PDF and adhere to the specs, I forget the requirements though. | 17:17.42 |
| kens | Its certainly true that PDF (1.x at least) only supports a subset | 17:17.46 |
| | That may well change in PDF 2.0 | 17:18.00 |
| Robin_Watts | sebras: So there are a load of .ppms etc in that. | 17:19.12 |
| | I'll just grab the j2k and jp2 files. | 17:19.24 |
| sebras | Robin_Watts: oh, one thing that you could check is whether Artifex (Henrys? marcosw?) has access to the updated version of the PDF 2.0 spec from ISO. | 17:19.26 |
| | Robin_Watts: that's what I have done. | 17:19.41 |
| Robin_Watts | sebras: We don't, AIUI. | 17:19.45 |
| sebras | Robin_Watts: because we did buy an older revision of PDF 2.0, unless one has access to the account used to buy that revision it is hard to know if we can access the later revisions freely (until 2.0 is released proper) | 17:20.48 |
| | Robin_Watts: and it seems like marcosw has access to that account. | 17:21.20 |
| | Robin_Watts: I have upgraded to r10d. the newfangled r10d script doesn't handle symlinks very well though. :-( | 17:30.45 |
| | Robin_Watts: I can now build without that RESUT patch. | 17:31.10 |
| Robin_Watts | sebras: Ok, so in theory the cluster should check those files now. | 17:33.12 |
| sebras | Robin_Watts: let me start a run and test it. | 17:33.47 |
| Robin_Watts | sebras: I have just pushed the JNI stuff. | 17:34.07 |
| | so we'll see. | 17:34.10 |
| sebras | Robin_Watts: btw... with ndk r10d I'm missing adb. | 17:35.01 |
| | Robin_Watts: how do i push/pull files from my phone or install the apks without it? | 17:35.25 |
| Robin_Watts | sebras: You should not be missing adb. | 17:35.41 |
| jogux | sebras: that's confusing. adb isn't part of ndk. | 17:35.44 |
| Robin_Watts | adb is... what jogu said. Part of the sdk I believe. | 17:35.59 |
| sebras | jogux: in r8b I see it in platform-tools/adb | 17:36.41 |
| | oh, they seem to have removed it completely in marshmallow and later. | 17:37.23 |
| jogux | sebras: in my setups adb is in Android/sdk/platform-tools/adb | 17:37.37 |
| | admittedly I have little idea where it came from, but my ndks are in other locations | 17:38.01 |
| sebras | ok, so maybe my r8b directory might contain a happy combination of ndk _and_ sdk. who knew?! :) | 17:39.51 |
| jogux | sebras: if you can get into 'android sdk manager', I suspect you want 'android sdk platform-tools'. | 17:41.06 |
| | I find it's 50:50 whether I improve the sitatuion or make it worse when I dabble in sdk manager :) | 17:41.45 |
| sebras | jogux: :) | 17:41.53 |
| | Robin_Watts: trivial patch over at sebras/master for pbm format. https://linux.die.net/man/5/pbm | 17:58.44 |
| | Robin_Watts: 1 is black, 0 is white in binary images according to the link above. the same is true for ascii images. | 17:59.07 |
| Robin_Watts | sebras: Urgh. Adding those j2k files has upset the cluster. | 18:00.10 |
| | logs too big. I guess it must be spewing error messages? | 18:00.21 |
| sebras | Robin_Watts: not that I remember, but it was some time since I ran them all in openjpeg | 18:01.21 |
| | Robin_Watts: 33017 lines in total for my latest openjpeg run for all the files. | 18:02.19 |
| Robin_Watts | 1252520 temp/tests_private__j2k__openjpeg__input__nonregression__issue823.jp2.pgmraw.200.1.log | 18:02.51 |
| | The pbm fix seems good. | 18:03.31 |
| sebras | Robin_Watts: I don't have any of the files in the issue8*.jp2-series. | 18:05.25 |
| | Robin_Watts: they were added 17 days ago. | 18:05.38 |
| Robin_Watts | ok. | 18:05.59 |
| | let me rename those files on the cluster for now. | 18:07.09 |
| sebras | Robin_Watts: malloc attempts to alloc -1073740800 for that file according to valgrind | 18:09.22 |
| | Robin_Watts: after that it errors out for me. | 18:09.32 |
| | Robin_Watts: but that's inside valgrind. | 18:09.38 |
| Robin_Watts | sebras: OK, hopefully the cluster should retry the last mupdf commit and not barf this time. | 18:10.52 |
| sebras | Robin_Watts: hm... looks like that file reports a height of 0 | 18:14.01 |
| | Robin_Watts: are image filters supposed to throw on dimensions == 0? | 18:14.17 |
| Robin_Watts | sebras: They could do. | 18:18.54 |
| sebras | Robin_Watts: but fz_new_pixmap_with_data() only fails for < 0 | 18:19.15 |
| Robin_Watts | zero sized images don't seem terribly useful to me. | 18:21.35 |
| sebras | Robin_Watts: not to me either. | 18:21.44 |
| | Robin_Watts: hm.. so we throw in fz_load_jpx_info() already? | 18:25.05 |
| | Robin_Watts: oh... png throws if dimensions <=0 in fz_load_png_info(). | 18:25.54 |
| Robin_Watts | http://ghostscript.com/regression/cgi-bin/clustermonitor.cgi?report=8e6c5ce4b12af165ef6bebd69c65829095e6edd5&project=mupdf | 18:33.41 |
| sebras | Robin_Watts: let me check. | 18:34.21 |
| | Robin_Watts: issue420.jp2 and issue432.jp2 doesn't crash for me here. | 18:39.18 |
| | Robin_Watts: the rest causes various errors in valgrind using openjpeg. | 18:39.33 |
| | Robin_Watts: haven't checked luratech. | 18:39.39 |
| | Robin_Watts: seems like all these bugs have been fixed upstreams but we are using an older version of openjpeg (or haven't cherry-picked this bugfixes) | 18:42.05 |
| Robin_Watts | Can we cherry pick those in easily? | 18:52.17 |
| sebras | Robin_Watts: I don't know yet. | 18:52.24 |
| | Robin_Watts: probably some of them at least. | 18:52.44 |
| | Robin_Watts: do you mind taking a look at fz_cached_color_convert()? | 19:06.04 |
| | Robin_Watts: I think it is safe that it doesn't rethrow. | 19:06.19 |
| | Robin_Watts: in load_html_image() I'm not sure it will return NULL upon error..? | 19:18.20 |
| sebras | sleeps | 19:18.51 |
| tor8 | Robin_Watts: oops. yes, pdf_add_stream should rethrow instead of drop... | 22:08.09 |
| | or something... I must have been tired when I wrote that function. | 22:08.44 |
| | Forward 1 day (to 2016/09/24)>>> | |
Log of #ghostscript at irc.freenode.net.