| | <<<Back 1 day (to 2018/09/03) | 20180904 |
| sebras | mvrhel: tor8: so fz_new_icc_colorspace() would actually set profile->bgr depending if the name it is given is "DeviceBGR". could gsview rely on this? I don't know in what situations gsview calls fz_set_icc_bgr() today..? | 09:18.12 |
| avih | tor8: your mujs getopt change broke mujs at least on msys2's mingw (gcc8). apparently optind and friends were already declared as extern elsewhere, and it clashes with your new definitions. i'd suggest to rename these vars/function | 09:20.30 |
| | (fwiw, i thought my args patch was fairly useful and unintrusive) | 09:21.58 |
| tor8 | sebras: yeah, that's what I'm hoping we can do instead | 09:22.20 |
| sebras | tor8: I got that, but mvhrel hasn't responded and I don't know where that code is located so I can't take a look at what it does. | 09:24.49 |
| | thus I still have a set of patches waiting on my branch. :-/ | 09:25.15 |
| kens | yesterday was a public holiday in the US | 09:25.30 |
| sebras | kens: I know. | 09:25.58 |
| kens | Ah OK | 09:26.04 |
| sebras | kens: I'm not really complaining, it's just the normal "urgh, I still have a set of commits that are not in proper yet." :) | 09:28.06 |
| kens | :-) | 09:28.15 |
| avih | tor8: the getopt thingy also breaks with mingw cross from linux (gcc 5.4) | 09:28.17 |
| | (just tried) | 09:28.35 |
| tor8 | avih: I guess maybe because I'm using -std=c99 and -pedantic it doesn't clash | 09:28.58 |
| | will fix | 09:29.00 |
| avih | i think c99 should stay. pedantic possibly too | 09:29.27 |
| tor8 | avih: try tor/master | 09:30.09 |
| avih | aren't there non-windows setups where getopt is part of libc? | 09:30.19 |
| | (or glibc?) | 09:30.30 |
| tor8 | getopt is part of libc, but only if you have _POSIX_C_SOURCE >= 2 || _XOPEN_SOURCE | 09:30.54 |
| | which are not true with the 'ansi only' compiler flags | 09:31.10 |
| avih | tor8: d4655ae ? | 09:32.20 |
| tor8 | avih: yes. d4655ae, 924649a, 84858c3, and a2b62c0. | 09:33.39 |
| avih | oh, didn't notice the getopt clashing thingy was some commits earlier. | 09:34.20 |
| | checking. | 09:34.23 |
| | tor8: works on both cases where it previously failed | 09:36.04 |
| | s/cases/setups/ | 09:36.12 |
| Robin_Watts | tor8, sebras: I hate the idea of doing different things by detecting names. | 09:43.33 |
| tor8 | Robin_Watts: says you, who added the function that takes a name to do different things that we're talking about... :) | 09:47.01 |
| | or approved it, when michael did :) | 09:47.19 |
| | FZ_ICC_PROFILE_BGR is the string we magically trigger on | 09:48.48 |
| | but the magical triggering only happens if you pass in the magic string, not if you pass in a fz_buffer, in the two-faced fz_new_icc_profile function that does different things depending on which of the two arguments is null | 09:49.23 |
| | Robin_Watts: maybe a plain enum to fz_new_icc_colorspace rather than a string would be better | 09:51.26 |
| | and then we check that it either matches the number of colorants in the buffer loaded (if a buffer is passed in) | 09:51.43 |
| | or that it is FZ_ICC_PROFILE_UNKNOWN | 09:51.50 |
| | rather than using magic strings | 09:52.06 |
| sebras | tor8: similar to the num argument where we complain if profile->num_devcomp is not the same..? | 09:59.49 |
| | I believe this is the number of compoments..? | 09:59.54 |
| | tor8: to me it seems like what we need is to differentiate between all the 3 component colorspaces (lab, bgr and rgb) | 10:01.30 |
| tor8 | sebras: yeah. I'm going to prod a bit at this. | 10:01.52 |
| sebras | tor8: wouldn't fz_colorspace_type be the enum we would want? | 10:02.05 |
| | and then possibly skip the num argument completely..? | 10:02.54 |
| | yeah, yeah, APIs changing, I know. I just want to know what the ideal interface would be. | 10:03.37 |
| tor8 | nobody should be using this api themselves | 10:04.02 |
| sebras | tor8: gsview does. | 10:04.24 |
| tor8 | sebras: bitrot through missing filename LGTM | 10:20.20 |
| sebras | tor8: then I'll go ahead and push those immediately. | 10:24.21 |
| | done. | 10:25.37 |
| tor8 | the commit "WIP structured text walker callback interface" could do with some fleshing out if you want to dig into some JNI hacking, but there's no pressing need | 10:28.26 |
| sebras | tor8: noted. | 10:30.42 |
| | if "Add fz_new_stext_page_from_annot utility function." and "Add missing fz_var declarations." the former commit could correctly add the fz_var() immediately. the end result would of course look the same. | 10:48.53 |
| | tor8: ^^ other than that "Fix 699177: Don't clamp line width to 1 after scaling by matrix expansion." to "Add missing fz_var declarations." om tor/master LGTM | 10:49.18 |
| tor8 | sebras: like now? | 11:00.29 |
| | Robin_Watts: you may want to look at "Use colorspace type enum instead of magic profile names." and express an opinion | 11:01.29 |
| Robin_Watts | tor8: I would prefer an enum, yes, or a bitfield. | 11:15.00 |
| | Let me look. | 11:15.14 |
| | I thought that "using an enum rather than strings" was something you wanted to do after the original commits went in. | 11:15.35 |
| | fz_lookup_icc(fz_context *ctx, int name, size_t *size) | 11:17.18 |
| | fz_lookup_icc(fz_context *ctx, enum fz_colorspace_type type, size_t *size) | 11:17.29 |
| | The NO_ICC branch has a different type. | 11:17.38 |
| | ok, so fz_new_colorspace already takes name and type. | 11:20.40 |
| sebras | tor8 yes. | 11:20.56 |
| | tor8: png_read_icc() seems like it might be able to supply a better FZ_COLORSPACE_enum..? | 11:21.09 |
| Robin_Watts | It's just fz_new_icc_colorspace that currently takes just a name, and now takes just a type. | 11:21.09 |
| | tor8: If you fix fz_lookup_icc in the NO_ICC #ifdeffery, then that looks good to me. | 11:21.33 |
| sebras | tor8: I'm looking at the PNG spec to see if there is a limited list of ICC profile names. | 11:22.05 |
| tor8 | Robin_Watts: oops, forgot to fix the NO_ICC ifdef section | 11:23.56 |
| | yeah, we've lost the name (that is only ever used for debugging purposes, and as the magic enum) | 11:24.15 |
| | sebras: we could do better, but we already check that the colorspace matches the profile towards the end "embedded ICC profile does not match PNG colorspace" | 11:25.52 |
| sebras | tor8: it just says it is a convient name for the profile, doesn't seem to actually use it. | 11:26.22 |
| tor8 | sebras: yes, from what I've seen it's a human readable description of the profile's name (like "Adobe sRGB" or similar) | 11:26.52 |
| sebras | tor8: oh, I didn't realize that the check was elsewhere. | 11:26.54 |
| tor8 | sebras: yes, from what I've seen it's a human readable description of the profile's name (like "Adobe sRGB" or similar) | 11:27.28 |
| | sebras: we can possibly pass in info->type as the enum to fz_new_icc_profile | 11:27.45 |
| | since the IHDR must come first, and fills in that value we can do so | 11:28.10 |
| sebras | tor8 yes. | 11:28.11 |
| | tor8: I think that might be a good idea. as iHDR must come before iCCP, so we ought to already know what we expect. | 11:28.36 |
| | tor8: I like those comma expressions instead of blocks. :) | 11:29.32 |
| tor8 | sebras: hm, do we double-check that the ICC profile matches the number of components in the tiff loader? | 11:30.33 |
| sebras | tor8: I don't know. | 11:30.56 |
| | tor8: probably not..? | 11:31.00 |
| | but we should. | 11:31.06 |
| tor8 | sebras: ah, the TIFF code is smart enough to handle mismatches by padding or dropping extra components | 11:35.05 |
| | in fz_unpack_tile- | 11:35.19 |
| sebras | tor8: ehm. seems like we don't handle large icc profiles in jpeg or at all in gif. | 11:45.25 |
| | tor8: there's a patch on sebras/master for the first issue. | 13:48.08 |
| Robin_Watts | "large" ? | 13:48.52 |
| sebras | Robin_Watts: yes, they may be split between multiple JPEG APP-markers as they are each limited to 65519 bytes or there about. | 13:56.36 |
| | Robin_Watts: I guess it depends on your definition of large of course. | 13:56.45 |
| Robin_Watts | Ah, right. | 13:56.46 |
| | No, that makes perfect sense. | 13:56.52 |
| | sebras: Is 256 a limit of yours? | 13:58.14 |
| | or is it in the spec? | 13:58.19 |
| sebras | Robin_Watts: it is in the spec. the part number is stored in a byte. | 13:58.34 |
| Robin_Watts | 256 and 14 seem like magic numbers. | 13:58.40 |
| sebras | Robin_Watts: 14 is the size of idseq + the part byte + the parts byte. | 13:58.57 |
| Robin_Watts | enum { MAX_ICC_PARTS=256} ? | 13:58.58 |
| sebras | sure. | 13:59.03 |
| Robin_Watts | parts = fz_mini(256, marker->data[13]); | 14:00.32 |
| sebras | could be: parts = fz_mini(MAX_ICC_PARTS, marker->data[nelem(idseq) + 1]); | 14:00.58 |
| | if you prefer that. | 14:01.01 |
| Robin_Watts | Why not just parts = marker->data[13] ? | 14:01.04 |
| | marker->data[13] is guaranteed to be < 256, right? | 14:01.27 |
| sebras | Robin_Watts: yes. | 14:01.36 |
| | sure. | 14:02.10 |
| | that makes it parts = marker->data[nelem(idseq) + 1]; | 14:02.19 |
| | if you so prefer. | 14:02.24 |
| Robin_Watts | My sensibilities are less offended by [13] etc when that's obviously indexing into a structure. | 14:03.47 |
| | (My sensibilities are evidently not consistent :) ) | 14:04.00 |
| | A random thought... | 14:04.19 |
| sebras | so &marker->data[14] ? :) | 14:04.26 |
| Robin_Watts | I have a niggling thing in my head that says that I've seen JPEGy things where stuff like "parts" aren't written correctly until the last part rolls around. | 14:05.06 |
| sebras | Robin_Watts: the ICC spec states: All chunks in the sequence should indicate the same total number of chunks. | 14:05.57 |
| Robin_Watts | Ah, cool. | 14:06.16 |
| sebras | if they don't we warn now, but continue with the original number. | 14:06.17 |
| Robin_Watts | I was going to say we could have accepted any value for parts as long as it was > part. | 14:06.56 |
| | That's almost what your code does... | 14:07.43 |
| | but anyway, seems like a nice fix to me. | 14:08.28 |
| sebras | Robin_Watts: I updated sebras/master LGTY? | 14:09.51 |
| Robin_Watts | sure, thanks! | 14:10.40 |
| sebras | Robin_Watts: excellent, I'll run it through the cluster and see what falls out. | 14:11.16 |
| tor8 | avih: test ch08/8.12/8.12.4/8.14.4-8-b_1.js is invalid (it sets "unwritable" in the property descriptor, a value that is not supported by any specification) | 14:17.51 |
| avih | tor8: ? (maybe unrelated, the pushed commits don't try to match the negative string. i'll soon push a commit which does. it adds few new failures) | 14:19.03 |
| tor8 | avih: nvm, I'm not reading the source correctly | 14:19.13 |
| avih | e.g. sometimes it expects a SyntaxError but mujs throws TypeError. till now it only expected any error on @negative | 14:20.08 |
| sebras | Robin_Watts: I had to change the commit a little bit because it causes issues. | 14:24.37 |
| Robin_Watts | sebras: The difference being? part = 1 ? | 14:25.47 |
| sebras | also initing marker to NULL and checking if init_marker == NULL and returning early. | 14:26.14 |
| | Robin_Watts: otherwise if there are not markers you end up in an eternal loop, dooh. | 14:26.35 |
| Robin_Watts | sebras: Ah, I hadn't spotted the nested loops. | 14:28.17 |
| sebras | I like the cluster. | 14:28.31 |
| Robin_Watts | You're allowing for parts being sent in the wrong order? | 14:28.38 |
| sebras | Robin_Watts: btw, can you un-idle watts? | 14:28.47 |
| | Robin_Watts: yes. | 14:28.52 |
| Robin_Watts | It shouldn't be idle. It's running. | 14:29.13 |
| | Someone has disabled it. | 14:29.25 |
| sebras | Robin_Watts: there was an issue with the cluster ecently, perhaps at that time? | 14:29.43 |
| Robin_Watts | I've reenabled it. | 14:30.06 |
| StevePhillips | #join /artifex | 14:31.53 |
| avih | tor8: pushed an enhancement to also compare the @negative regex (and took your variant of the "compile" commit, so it should apply cleanly at your repo) | 16:03.06 |
| | tor8: fwiw, the getopt issue also happens on alpine linux (gcc 6.4). mind pushing the fix to master? | 17:29.40 |
| Robin_Watts | sebras, tor8, mvrhel: http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=d0ccc6e2494600c231ea2d4ccc336a821400c4cd | 18:34.15 |
| | and http://git.ghostscript.com/?p=user/robin/mupdf.git;a=commitdiff;h=a083fa2b3ecf8db5d2fe1b1f6d1162ce93da5ac2 | 18:34.29 |
| sebras | Robin_Watts: I can move the missing variable into my commit. that was just sloppy of me. sorry. | 18:42.27 |
| Robin_Watts | sebras: No worries, we've all done it :) | 18:42.49 |
| sebras | s/I can/I should have/ | 18:43.24 |
| | Robin_Watts: "GPROOF: Move away from unlink to remove." and "Update VS solution to cope with PKCS7 build changes." lgtm. | 18:45.04 |
| | Robin_Watts: so this time the unistd.h thing prevented compilation on msvc, and I didn't see it because we never compile gproof. ugh. | 18:47.15 |
| Robin_Watts | sebras: No, the unistd.h prevented it building on MSVC even without Gproof being enabled. | 18:57.02 |
| | cos the inclusion is outside of the #ifdef FZ_ENABLE_GPROOF | 18:57.18 |
| sebras | Robin_Watts: so I never saw this in the cluster because neither ergs nor watts were online? | 18:57.47 |
| Robin_Watts | I think this slipped through cos someone disabled watts the other day without me knowing, and ergs doesn't appear to be online either. | 18:57.56 |
| sebras | right | 18:58.05 |
| | Forward 1 day (to 2018/09/05)>>> | |
Log of #mupdf at irc.freenode.net.