Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
892413005:
Add bgra (z,y,x) sampled transform lookup table api (Closed)
|
Created:
5 years, 10 months ago by Noel Gordon Modified:
5 years, 10 months ago CC:
chromium-reviews, Stephen White, Justin Novosad, bsalomon_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd bgra (z,y,x) sampled transform lookup table api
Add an API to extract a color transform lookup table in BGRA format, with
(z,y,x) sampling order (B changes the slowest, R the fastest).
See r314425 for speed results, noting that qcms_chain_transform takes 8ms
for a 32-cube since floating-point is used to compute the color transform
to maintain transform precision.
The float data produced by qcms_chain_transform() is stored in the output
in BGRA order (color components are rounded to the nearest 8-bit integer)
with time cost 0.000031 secs. Note that if the data needed re-indexing to
(x,y,z) sampled, RGBA order on writing to the output, the time cost would
be 0.00056 secs (18 times slower) [1].
[1] See r314425 review comments about sampling and pixel order.
BUG=443863
Committed: https://crrev.com/86316166abe8d0bc597f4482109d7afcac1f0fe2
Cr-Commit-Position: refs/heads/master@{#315188}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Comment the api function. Add rendering intent. #
Messages
Total messages: 18 (3 generated)
noel@chromium.org changed reviewers: + kbr@chromium.org, mikelawther@chromium.org, reed@google.com, sugoi@chromium.org
Anyone comfortable with the review, should (even those cc-ed).
Generally looks good, but someone else more familiar with the transforms should review it. One comment. https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h File third_party/qcms/src/qcms.h (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h#... third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_skia( Let's remove the Skia reference from this function name, document it and contribute it upstream. https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... File third_party/qcms/src/transform.c (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * 255.0f + 0.5f); // b Not sure, but I thought the way to do this conversion was clamp(ubyte*256.0f, 0, 255).
https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h File third_party/qcms/src/qcms.h (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h#... third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_skia( On 2015/02/04 22:23:05, Ken Russell wrote: > Let's remove the Skia reference from this function name, document it changed skia -> zyx_bgra to indicate cube sampling order and output format. > and contribute it upstream. This new function you mean? https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... File third_party/qcms/src/transform.c (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * 255.0f + 0.5f); // b On 2015/02/04 22:23:05, Ken Russell wrote: > Not sure, but I thought the way to do this conversion was clamp(ubyte*256.0f, 0, 255). Interesting thought, not sure of the correct answer here. The LUT data produced by qcms_chain_transform is clamped on [ 0.0 .. 1.0 ] for reference. I've mapped that to 0 .. 255 with rounding. Need the rounding, but I think your question is should the multiplier be 256.0f or 255.0f as I've used? Do you know how GPU map there [ 0.0 .. 1.0 ] pixel data onto 0 .. 255 when writing pixels? Does it round somehow?. Also, I assume clamp() is the GLSL one? It returns a float, computed as min(max(x, minVal), maxVal) per [1] [1] https://www.opengl.org/sdk/docs/man/html/clamp.xhtml
LGTM overall; I won't hold this patch up over the discussion about the exact float->ubyte mapping. https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h File third_party/qcms/src/qcms.h (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h#... third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_skia( On 2015/02/05 16:28:52, noel gordon wrote: > On 2015/02/04 22:23:05, Ken Russell wrote: > > Let's remove the Skia reference from this function name, document it > > changed skia -> zyx_bgra to indicate cube sampling order and output format. Great. Thanks. > > and contribute it upstream. > > This new function you mean? Yes, this new function. https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... File third_party/qcms/src/transform.c (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * 255.0f + 0.5f); // b On 2015/02/05 16:28:52, noel gordon wrote: > On 2015/02/04 22:23:05, Ken Russell wrote: > > Not sure, but I thought the way to do this conversion was clamp(ubyte*256.0f, > 0, 255). > > Interesting thought, not sure of the correct answer here. > > The LUT data produced by qcms_chain_transform is clamped on [ 0.0 .. 1.0 ] for > reference. I've mapped that to 0 .. 255 with rounding. Need the rounding, but I > think your question is should the multiplier be 256.0f or 255.0f as I've used? My understanding is that the formulation above (multiply by 256.0 and clamp to between 0..255) provides a more equal mapping of the values between 0.0 ... 1.0 and 0..255. > Do you know how GPU map there [ 0.0 .. 1.0 ] pixel data onto 0 .. 255 when > writing pixels? Does it round somehow?. Good question. This was actually changed in the OpenGL ES 3.0 spec. See OpenGL ES 3.0.3, appendix F.2 "Differences in Runtime Behavior", and the reference to section 2.1.6 of that spec. That formulation is actually different than both yours and the one I suggested above. > Also, I assume clamp() is the GLSL one? It returns a float, computed as > min(max(x, minVal), maxVal) per [1] No, I was just talking about an abstract clamp operation. https://codereview.chromium.org/892413005/diff/20001/third_party/qcms/src/qcms.h File third_party/qcms/src/qcms.h (right): https://codereview.chromium.org/892413005/diff/20001/third_party/qcms/src/qcm... third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_zyx_bgra( Please document what the new function does.
https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... File third_party/qcms/src/transform.c (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * 255.0f + 0.5f); // b On 2015/02/06 02:49:54, Ken Russell wrote: > On 2015/02/05 16:28:52, noel gordon wrote: > > On 2015/02/04 22:23:05, Ken Russell wrote: > > The LUT data produced by qcms_chain_transform is clamped on [ 0.0 .. 1.0 ] for > > reference. I've mapped that to 0 .. 255 with rounding. Need the rounding, but > I > > think your question is should the multiplier be 256.0f or 255.0f as I've used? > > My understanding is that the formulation above (multiply by 256.0 and clamp to > between 0..255) provides a more equal mapping of the values between 0.0 ... 1.0 > and 0..255. My understanding was the same as Ken's. With "mult by 255 and round", 0 and 255 have half as many values that map to them as do 1-254. > > Do you know how GPU map there [ 0.0 .. 1.0 ] pixel data onto 0 .. 255 when > > writing pixels? Does it round somehow?. > > Good question. This was actually changed in the OpenGL ES 3.0 spec. See OpenGL > ES 3.0.3, appendix F.2 "Differences in Runtime Behavior", and the reference to > section 2.1.6 of that spec. That formulation is actually different than both > yours and the one I suggested above. Interesting! The difference mentioned was for signed floating point conversions rather than unsigned? I interpreted eqn 2.3 as being "mult by 255 and prefer to round" which sounds like what Noel originally proposed?
(I also do not intend to hold this patch up over the exact conversion)
https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... File third_party/qcms/src/transform.c (right): https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * 255.0f + 0.5f); // b On 2015/02/06 03:20:06, Mike Lawther (Google) wrote: > On 2015/02/06 02:49:54, Ken Russell wrote: > > On 2015/02/05 16:28:52, noel gordon wrote: > > > On 2015/02/04 22:23:05, Ken Russell wrote: > > > The LUT data produced by qcms_chain_transform is clamped on [ 0.0 .. 1.0 ] > for > > > reference. I've mapped that to 0 .. 255 with rounding. Need the rounding, > but > > I > > > think your question is should the multiplier be 256.0f or 255.0f as I've > used? > > > > My understanding is that the formulation above (multiply by 256.0 and clamp to > > between 0..255) provides a more equal mapping of the values between 0.0 ... > 1.0 > > and 0..255. > > My understanding was the same as Ken's. With "mult by 255 and round", 0 and 255 > have half as many values that map to them as do 1-254. > > > > Do you know how GPU map there [ 0.0 .. 1.0 ] pixel data onto 0 .. 255 when > > > writing pixels? Does it round somehow?. > > > > Good question. This was actually changed in the OpenGL ES 3.0 spec. See OpenGL > > ES 3.0.3, appendix F.2 "Differences in Runtime Behavior", and the reference to > > section 2.1.6 of that spec. That formulation is actually different than both > > yours and the one I suggested above. > > Interesting! The difference mentioned was for signed floating point conversions > rather than unsigned? That's correct. The unsigned conversions were unchanged. > I interpreted eqn 2.3 as being "mult by 255 and prefer to > round" which sounds like what Noel originally proposed? I'm not a floating point expert and am not sure whether "add 0.5 and floor" is the same as "round to nearest".
On 2015/02/06 03:31:35, Ken Russell wrote: > > I interpreted eqn 2.3 as being "mult by 255 and prefer to > > round" which sounds like what Noel originally proposed? > > I'm not a floating point expert and am not sure whether "add 0.5 and floor" is > the same as "round to nearest". Fair enough - it's not precisely the same.
New patchsets have been uploaded after l-g-t-m from kbr@chromium.org
> third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_skia( > On 2015/02/05 16:28:52, noel gordon wrote: > > On 2015/02/04 22:23:05, Ken Russell wrote: > > Let's remove the Skia reference from this function name, document it > > > > changed skia -> zyx_bgra to indicate cube sampling order and output format. > > On 2015/02/04 22:23:05, Ken Russell wrote: > Great. Thanks. Also changed color_cube to LUT in the function name, and added a rendering intent argument (unused). Removed skia from the change title and description. https://codereview.chromium.org/892413005/diff/20001/third_party/qcms/src/qcms.h File third_party/qcms/src/qcms.h (right): https://codereview.chromium.org/892413005/diff/20001/third_party/qcms/src/qcm... third_party/qcms/src/qcms.h:161: qcms_bool qcms_transform_create_color_cube_zyx_bgra( On 2015/02/06 02:49:54, Ken Russell wrote: > Please document what the new function does. Done. Added comment in the transform.c file.
On 2015/02/06 02:49:54, Ken Russell wrote: > LGTM overall; I won't hold this patch up over the discussion about the exact > float->ubyte mapping. > > https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h > File third_party/qcms/src/qcms.h (right): > > https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/qcms.h#... > third_party/qcms/src/qcms.h:161: qcms_bool > qcms_transform_create_color_cube_skia( > On 2015/02/05 16:28:52, noel gordon wrote: > > On 2015/02/04 22:23:05, Ken Russell wrote: > > > Let's remove the Skia reference from this function name, document it > > > > changed skia -> zyx_bgra to indicate cube sampling order and output format. > > Great. Thanks. > > > > and contribute it upstream. > > > > This new function you mean? > > Yes, this new function. > > https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... > File third_party/qcms/src/transform.c (right): > > https://codereview.chromium.org/892413005/diff/1/third_party/qcms/src/transfo... > third_party/qcms/src/transform.c:1206: cube[index++] = (int)floorf(lut[l + 2] * > 255.0f + 0.5f); // b > On 2015/02/05 16:28:52, noel gordon wrote: > > On 2015/02/04 22:23:05, Ken Russell wrote: > > > Not sure, but I thought the way to do this conversion was > clamp(ubyte*256.0f, > > 0, 255). > > > > Interesting thought, not sure of the correct answer here. > > > > The LUT data produced by qcms_chain_transform is clamped on [ 0.0 .. 1.0 ] for > > reference. I've mapped that to 0 .. 255 with rounding. Need the rounding, but > I > > think your question is should the multiplier be 256.0f or 255.0f as I've used? > > My understanding is that the formulation above (multiply by 256.0 and clamp to > between 0..255) provides a more equal mapping of the values between 0.0 ... 1.0 > and 0..255. One interesting way think about that is value * 255.0 * M, where M = 256.0 / 255.0. This stretches the data some. For the clamp, you still need rounding of some sort. qcms uses mul * 255 + 0.5 rounding to convert transformed color float values to 0..255 when it writes the output. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/qcms/s... The clamp_u8 routine implements the floor + 0.5 rounding [1] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/qcms/s...
So I think we should just match what qcms does for now. On 2015/02/06 04:21:28, Mike Lawther (Google) wrote: > On 2015/02/06 03:31:35, Ken Russell wrote: > > > I interpreted eqn 2.3 as being "mult by 255 and prefer to > > > round" which sounds like what Noel originally proposed? The spec says "cast" though, which could be interpreted as truncate by some implementors sadly. > > I'm not a floating point expert and am not sure whether "add 0.5 and floor" is > > the same as "round to nearest". > > Fair enough - it's not precisely the same. However, "round to nearest" is not precisely defined unless one states how it break ties. Take 1.5 as a test. Nearest integers are 1 _and_ 2. If you break such ties by rounding away from 0, the implementation is "add 0.5 and floor" I believe.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892413005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
On 2015/02/07 06:10:13, noel gordon wrote: > So I think we should just match what qcms does for now. > > On 2015/02/06 04:21:28, Mike Lawther (Google) wrote: > > On 2015/02/06 03:31:35, Ken Russell wrote: > > > > I interpreted eqn 2.3 as being "mult by 255 and prefer to > > > > round" which sounds like what Noel originally proposed? > > The spec says "cast" though, which could be interpreted as truncate by some > implementors sadly. I wonder if the * 255.0 * M, M = 256.0 / 255.0 method might compensate for that. Something to revisit.
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/86316166abe8d0bc597f4482109d7afcac1f0fe2 Cr-Commit-Position: refs/heads/master@{#315188} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
