| | <<<Back 1 day (to 2016/08/14) | 20160815 |
| sebras | tor8: care to review sebras/master? | 14:28.36 |
| | tor8: two fixed for what you mentioned in review elsewhere | 14:28.56 |
| tor8 | sebras: LGTM | 14:39.37 |
| sebras | tor8: tnx. | 14:46.26 |
| izabera | http://lgtm.in/ | 14:48.39 |
| sebras | izabera: I think that site just got their first visit on port 80 using elinks | 14:50.25 |
| | tor8: NativeDevice_fillPath() e.g. fails silently if jpath == NULL. | 14:51.49 |
| izabera | you should make ascii versions for those | 14:52.29 |
| sebras | disappears for an hour. | 15:03.15 |
| Robin_Watts | tor8: Hey, you're back? | 15:08.30 |
| tor8 | Robin_Watts: yes. just been going over the commits from the past couple of weeks. | 15:12.45 |
| Robin_Watts | hides then. | 15:12.55 |
| Robin_Watts | is off on saturday morning for 2 weeks. | 15:13.14 |
| tor8 | Robin_Watts: in commit c3d9f08d70530ff83c8e08a664614d85d162efb4 ... that fz_var looks superfluous to me? | 15:13.26 |
| Robin_Watts | tor8: Yes, don't need buf = NULL; and don't need fz_var(buf). | 15:15.54 |
| | Except possibly to silence compiler warnings. | 15:16.05 |
| sebras | Robin_Watts: you about? (I'm guessing tor8 is logged in but not at the computer) | 17:35.06 |
| | I want to briefly discuss my findings about devices and fill_path() and what expectations different devices have on their arguments. | 17:35.53 |
| | and how to handle them. | 17:35.56 |
| Robin_Watts | sebras: Syre. | 17:36.45 |
| | sure. | 17:36.47 |
| sebras | I'm looking at NativeDevice_fillPath() | 17:38.08 |
| | in platform/java/mupdf_native.c | 17:38.21 |
| | and I see that it calls fz_fill_path() using path and cs. | 17:38.48 |
| | we already check for some of these, but tor wanted me to throw exceptions in some cases instead of failing silently. | 17:39.15 |
| | that makes sense (at least to java programmers). | 17:39.25 |
| Robin_Watts | ok, now, let me think this through. | 17:39.53 |
| sebras | and maybe the underlying devices are _meant_ to support path == NULL and treat that as a pathological case of a non-existant path. | 17:40.07 |
| | I could just do | 17:40.26 |
| | if (path == NULL) | 17:40.27 |
| Robin_Watts | NativeDevice is the wrapper used to to allow java callers to make device calls into a native C device. | 17:40.30 |
| sebras | (*env)->ThrowNew(env, cls_IllegalArgumentException, "path is null"); | 17:40.31 |
| | and be done with it. | 17:40.33 |
| Robin_Watts | Yes, I think that test/throw would be a good idea. | 17:40.54 |
| sebras | but I discovered that for path: | 17:40.58 |
| | draw-device.c: derefs NULL | 17:41.07 |
| | bbox-device.c: derefs NULL | 17:41.10 |
| | list-device.c: derefs NULL | 17:41.13 |
| | svg-device.c: derefs NULL | 17:41.16 |
| | test-device.c: ok, but if set passthrough will do whatever passthrough device does | 17:41.19 |
| | trace-device.c: derefs NULL | 17:41.22 |
| | however for cs NULL is treated very differently | 17:41.35 |
| Robin_Watts | sebras: Devices can reasonably expect to get a valid path in. | 17:41.42 |
| | a NULL cs may mean "no colorspace, we're working in alpha only". | 17:42.02 |
| sebras | draw-device handles it as rgb, bbox-device does not care, list-device asserts in case color is also set (which it is in this case), svg-device handles it as rgb, test-device checks if it is NULL and handles it "gracefully", | 17:42.26 |
| Robin_Watts | but that function will already crash if we have cs == NULL :) | 17:42.44 |
| sebras | (but might be in a non-desired way), and finally trace-device which will deref NULL | 17:42.46 |
| | Robin_Watts: where would it crash if cs == NULL? | 17:43.18 |
| Robin_Watts | from_jfloatArray(... cs->n ...) | 17:43.28 |
| sebras | Robin_Watts: cs ? cs->n... | 17:43.36 |
| Robin_Watts | D'Oj. | 17:43.38 |
| | D'Oh. | 17:43.41 |
| sebras | (Oj works too, means oops in swedish. :) ) | 17:43.59 |
| Robin_Watts | I think we shouldn't be testing for cs == NULL. | 17:44.02 |
| sebras | Robin_Watts: in that case trace-device is broken. | 17:44.16 |
| Robin_Watts | sebras: Or if we do want to test for cs = NULL, we need an fz_alpha_only colorspace :) | 17:44.49 |
| sebras | Robin_Watts: in fz_trace_color() it will deref NULL | 17:44.50 |
| Robin_Watts | sebras: Then possibly that's a bug. | 17:45.02 |
| sebras | Robin_Watts: right. | 17:45.16 |
| Robin_Watts | I would be tempted to fix that, accept cs == NULL, and throw an error on a NULL path. | 17:45.35 |
| sebras | are the valid ranges of values for devices documented somewhere? | 17:45.44 |
| Robin_Watts | sebras: Nope. They should be documented in the headers, but I don't think we're that thorough yet. | 17:46.09 |
| sebras | I haven't read the device code before so I'm not sure what the intent on tor's/your part has been. | 17:46.17 |
| | right. | 17:46.27 |
| | then you'll have to survive number of stupid questions over this. :) | 17:46.41 |
| | Robin_Watts: so trace-device should default to rgb in case cs == NULL? | 17:47.09 |
| | Robin_Watts: just like draw-device and svg-devices..? | 17:47.58 |
| Robin_Watts | draw-device defaults to rgb? :( | 17:48.07 |
| sebras | 3~ | 17:48.46 |
| Robin_Watts | Where? | 17:48.53 |
| sebras | Robin_Watts: passes it to fz_convert_color() which appears to assume rgb on NULL | 17:49.03 |
| Robin_Watts | Ah, right, but model = NULL -> n = 0. | 17:49.42 |
| sebras | Robin_Watts: basically it falls back to std_conv_color(). | 17:49.45 |
| | Robin_Watts: fz_convert_color() is still called. | 17:50.26 |
| Robin_Watts | so colorbv[0] gets sets to alpha, and none of the fz_convert_color stuff is used. | 17:50.42 |
| | yeah, we should stop it calling that if n == 0. | 17:50.48 |
| sebras | Robin_Watts: in the device or inside fz_convert_color()...? | 17:51.07 |
| Robin_Watts | I was thinking if (n > 0) { fz_convert_color() ; } | 17:51.29 |
| | but then ... we'd be scan converting with a dodgy colorbv. | 17:52.04 |
| | I suspect we need to have some sanitisation of state->dest and model. | 17:52.36 |
| sebras | Robin_Watts: let me tell you: DSL/fibre/whatever is sooo much better than 3G/4G-backed internet connections. :-/ | 18:00.56 |
| | Robin_Watts: so that means even if (n > 0) { fz_convert_color(); for (i = 0; i < n; i++) { ... } } else i = 0; colorbv[ | 18:01.52 |
| | i] = alpha * 255; wouldn't suffice..? | 18:02.01 |
| | right, because then the later code must cope with n == 0 and only colorbv[0] being set with alpha. | 18:03.20 |
| Robin_Watts | sebras: Yeah, I'd guess we should be checking that the input colorspace is only NULL in the case where state->dest is alpha only. | 18:05.22 |
| sebras | Robin_Watts: what do we do if colorspace != NULL? | 18:07.14 |
| | fz_throw()? | 18:07.22 |
| Robin_Watts | which colorspace? | 18:08.22 |
| | state->dest->colorspace = NULL, should imply that state->dest->n = 1, and state->dest->alpha = 1 | 18:08.50 |
| sebras | the input colorspace. | 18:09.40 |
| Robin_Watts | If colorspace = NULL, and state->dest->colorspace != NULL, then throw. | 18:10.00 |
| sebras | right, because we are attempting to draw an alpha-only path on a gray/rgb/cmyk destination..? | 18:10.38 |
| | IIUC | 18:10.46 |
| Robin_Watts | state->dest->colorspace != NULL means "It will need some color information for the call", so that requires colorspace != NULL | 18:11.09 |
| | yes. | 18:11.10 |
| sebras | if (colorspace == NULL && fz_pixmap_colorspace(state->dest) != NULL) | 18:13.09 |
| | fz_throw(ctx, FZ_ERROR_GENERIC, "destination requires non-alpha component"); | 18:13.16 |
| | at the top of the function in that case. | 18:13.22 |
| | oh, fz_pixmap_colorspace() is the same as model. hrm. | 18:14.03 |
| | Robin_Watts: but that still creates problem inside fz_scan_convert() if both colorspace == NULL and state->dest->colorspace == NULL | 18:15.50 |
| | i.e. please paint this alpha path on this alpha destination | 18:16.03 |
| Robin_Watts | Why? | 18:16.05 |
| sebras | Robin_Watts: because it will end up calling fz_get_span_color_painter() | 18:16.26 |
| | where n - da will be -1 | 18:16.38 |
| Robin_Watts | Urm. 0? | 18:17.01 |
| | dst->n = 1, dst->alpha = 1 | 18:17.17 |
| sebras | ah... n == 1! | 18:17.31 |
| Robin_Watts | n = number of components, alpha = number of the components that are alpha. | 18:17.36 |
| | yeah. | 18:17.41 |
| sebras | Robin_Watts: in template_span_with_color_N_general(), why do we test if sa == 256? isn't the max value 255? | 18:19.52 |
| Robin_Watts | sa = FZ_EXPAND(color[n1]) | 18:20.31 |
| | FZ_EXPAND(x) = x + (x>>7) | 18:20.42 |
| | so x in 0...255 gives sa in 0..256 | 18:21.00 |
| | It's so you can do (blah * x)>>8 and get back to 0...255 | 18:21.20 |
| sebras | hm, that's pretty smart. | 18:21.44 |
| | Robin_Watts: and the reason we compute ma is because we assume pre-multiplied..? | 18:22.47 |
| Robin_Watts | Yes. | 18:23.46 |
| | The really nice stuff about the x + (x>>7) trick is that 1) it's 1 instruction on ARM. | 18:24.36 |
| sebras | is now satisfied that there is no problem in fz_get_span_color_painter(). | 18:25.45 |
| | Robin_Watts: and 2)..? | 18:25.53 |
| Robin_Watts | 2) it means you can do all kinds of SWAR stuff, like taking an RGB value, splitting into RRRRRRRR00000000BBBBBBBB, and then doing a multiply and accumulate to blend several colors at once. | 18:26.06 |
| | s/colors/components/ | 18:26.23 |
| sebras | Robin_Watts: ok. | 18:27.08 |
| | Robin_Watts: do we do that kind of optimizations for ARM e.g.? | 18:27.22 |
| Robin_Watts | In the bitmap scaling code. | 18:27.44 |
| | (I think) | 18:27.48 |
| sebras | Robin_Watts: I think there are some ARM asm somewhere, but I don't know where (apparently not here). | 18:27.49 |
| | Robin_Watts: ok, so that took us from JNI bindings down to blending. :) | 18:28.26 |
| Robin_Watts | All the span painters have been carefully constructed so that they *should* optimise down to some pretty reasonable code cos of the inlining. | 18:28.46 |
| sebras | Robin_Watts: final question (for tonight) fz_trace_color() needs a colorspace name for it's xml-node. | 18:29.09 |
| | Robin_Watts: but if colorspace == NULL, what do we print? alpha? | 18:29.23 |
| Robin_Watts | "NULL"? :) | 18:29.30 |
| | "Alpha" would do. | 18:29.35 |
| sebras | Robin_Watts: would it make sense to implement a fz_device_alpha() and use that instead? | 18:30.47 |
| | and if colorspace == NULL just fail/throw/whatever. | 18:31.06 |
| | I know how to handle colorspace == NULL and path == NULL reliably now, but maybe the alternative is... better for some reason? | 18:31.53 |
| Robin_Watts | I kinda like NULL meaning "no colorspace" | 18:32.19 |
| sebras | Robin_Watts: alright. thanks for the discussion. I'll dig a bit further tomorrow and see if I can resolve a few more of these NativeDevice() functions. | 18:34.16 |
| Robin_Watts | fab | 18:34.26 |
| sebras | Robin_Watts: list-device.c: fill_path() calls fz_append_display_node() which assumes that colorspace == NULL --> fz_device_gray(). | 18:45.11 |
| Robin_Watts | sebras: That's a hangover from when we used gray interchangably with alpha. | 18:46.05 |
| sebras | Robin_Watts: ok, so that means it is a bug. | 18:46.55 |
| Robin_Watts | it's a bug in the displaylist code, yes. | 18:47.08 |
| | Crap. That's a pain. | 18:48.28 |
| sebras | Robin_Watts: so I'd remove that assignment, keep colorspace == NULL and then do the same dance with n = cs ? cs-> n : 0; and try to handle that later on. | 18:48.28 |
| | Robin_Watts: and no assert() | 18:49.04 |
| | Robin_Watts: why is that a pain? | 18:49.36 |
| Robin_Watts | We encode common colorspace values using bits. | 18:51.22 |
| sebras | Robin_Watts: what does that mean? | 18:52.13 |
| | is that the node.cs I'm seeing later on? | 18:52.21 |
| Robin_Watts | yeah. | 18:52.28 |
| | if we're grey, with 0, then we set node.cs = CS_GRAY_0, and we don't need to signal the color. | 18:53.02 |
| sebras | Robin_Watts: right, now I reached the same point where you said "pain!". :) | 18:53.03 |
| Robin_Watts | Actually, I don't think it's that bad. | 18:53.13 |
| | I think the final else there just needs to be updated to check for colorspace == NULL when calculating n./ | 18:53.35 |
| | I'd try removing the if (color && !colorspace) { .... } | 18:53.56 |
| | and make the next thing if (colorspace || color) | 18:54.05 |
| | and fix the two n = colorspace->n; to be n = colorspace ? colorspace->n : 0; | 18:54.38 |
| tor8 | Robin_Watts: sebras: yeah, colorspaces are generally allowed to be NULL to signify 'no color' | 18:55.16 |
| sebras | tor8: right. seems we have a few bugs in that area though. seems that is a rather new (at least untested) assumption. :) | 18:56.04 |
| tor8 | there are probably cases where we assume we have a valid color/colorspace and could get in trouble if the api is abused, but really we should be fz_throwing for those | 18:56.08 |
| | and not trying to patch that up in the JNI code | 18:56.28 |
| Robin_Watts | sebras: Mostly colorspaces being NULL is used *within* the draw device itself. | 18:56.36 |
| | but yes, this all got shaken up a bit recently when I added alphaless plotters. | 18:57.07 |
| tor8 | Robin_Watts: also when drawing soft masks | 18:57.18 |
| Robin_Watts | I think the code is better than it's been before, but there are still some cases where we survive by the skin of our teeth. | 18:57.41 |
| Robin_Watts | foods. | 18:58.30 |
| sebras | Robin_Watts: btw, node.cs is CS_UNCHANGED by default. if colorspace == NULL it should be set to...? CS_GRAY_? | 18:58.33 |
| Robin_Watts | CS_OTHER_0 I think... the final else. | 18:59.00 |
| | hmm. I'm confused by this code, and that's bad, cos I wrote it. | 18:59.55 |
| sebras | Robin_Watts: right, CS_OTHER_0 will be set because we do if (color || colorspace). I see. | 19:01.37 |
| | tor8: do you din looking at the top patch over at sebras/master? | 19:09.22 |
| | tor8: should be a no brainer, but I'd still like a lgtm. | 19:09.34 |
| tor8 | sebras: LGTM | 19:10.16 |
| sebras | tor8: tnx. | 19:10.23 |
| sebras | sleeps. | 19:24.48 |
| | Forward 1 day (to 2016/08/16)>>> | |
IRC Logs