Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494473003:
[qcms] Add a color profile white point transform api (Closed)
|
Created:
5 years ago by Noel Gordon Modified:
5 years ago CC:
chromium-reviews, radu.velea Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[qcms] Add a color profile white point transform api
Use the chain tool to compute how a given color profile transforms white
input to XYZ PCS (profile connection space).
No change in behavior, no new tests. Tested with an iMac P3 profile: the
returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249].
BUG=564355
Committed: https://crrev.com/cba2c8d41687786279d539071b3b957423f43cb1
Cr-Commit-Position: refs/heads/master@{#362927}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Ditch the static #
Messages
Total messages: 19 (6 generated)
Description was changed from ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). BUG=564355 ========== to ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white. BUG=564355 ==========
noel@chromium.org changed reviewers: + junov@chromium.org, robert.bradford@intel.com
https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.c File third_party/qcms/src/chain.c (right): https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1013: XYZ[0] *= 1.999969482421875f; Note: qcms_modular_transform_data internally scales XYZ by one on this number. Here we unscale it back to produce conventional unscaled XYZ output. For example, D50 in XYZ [0.9642, 1.0000, 0.8249]. If the profile is valid under the white point test, the output of this routine would be [0.9642, 1.0000, 0.8249], viz., D50 in unscaled XYZ.
Description was changed from ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white. BUG=564355 ========== to ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249]. BUG=564355 ==========
https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.c File third_party/qcms/src/chain.c (right): https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1001: static qcms_profile xyz_pcs_profile; What's the rationale for this? Is this a performance critical path where we're happy to increase our memory usage by 264 bytes for everyone (irrespective of using this function?) https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1004: memset(&xyz_pcs_profile, 0, sizeof(xyz_pcs_profile)); static variables are initialised to zero always, no need to memset. https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1006: } Alternative (using C99 initialisers): static const qcms_profile xyz_pcs_profile { .pcs = XYZ_SIGNATURE }; In this case, static const would go in the text segment vs BSS.
https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.c File third_party/qcms/src/chain.c (right): https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1003: if (xyz_pcs_profile.pcs != XYZ_SIGNATURE) { Is qcms not concerned about thread safety? https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1013: XYZ[0] *= 1.999969482421875f; Readability nit from a qcms outsider: put this number in a constant with a meaningful self-explanatory name. Ex: "ReciprocalTransformGain". Also, could you express this factor in terms of existing constants? Actually, I just peeked at qcms_modular_transform_create_input and saw the same literal there. I find this constant very odd. I am guessing it is somehow related to the epsilon of the fixed-point type 2*(1-1/(2^16)), but there is no explanation, which is a bit disconcerting.
PTAL https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.c File third_party/qcms/src/chain.c (right): https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1001: static qcms_profile xyz_pcs_profile; Nope, not perf critical. The chrome memory usage +264 bytes, per renderer, is probably not a worry. Better still, how about we ditch it? I only needed it to provide the existing code with an output profile, since that what it wanted. I'm gonna change that existing code so I don't need to do that at all. https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1003: if (xyz_pcs_profile.pcs != XYZ_SIGNATURE) { On 2015/12/02 15:40:08, Justin Novosad wrote: > Is qcms not concerned about thread safety? Yes it is, always. We currently create profiles on the Blink main thread only. And that'd be where we call this code, so we'd be thread-safe in our use-case. More generally, profiles and transforms are created on our main thread. But these profiles and transforms may be used on any thread _when they are being applied_ to do color correction. And there we need thread-saftey, and qcms is indeed thread-safe in application (when being applied), in all cases. https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1004: memset(&xyz_pcs_profile, 0, sizeof(xyz_pcs_profile)); On 2015/12/02 15:25:39, robert.bradford wrote: > static variables are initialised to zero always, no need to memset. Ack, but ditched the static anyway. https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1006: } On 2015/12/02 15:25:39, robert.bradford wrote: > Alternative (using C99 initialisers): > > static const qcms_profile xyz_pcs_profile { .pcs = XYZ_SIGNATURE }; > > In this case, static const would go in the text segment vs BSS. Ditched the static profile, but thanks for the tip. C99 is so ... so 1994 :) https://codereview.chromium.org/1494473003/diff/1/third_party/qcms/src/chain.... third_party/qcms/src/chain.c:1013: XYZ[0] *= 1.999969482421875f; On 2015/12/02 15:40:08, Justin Novosad wrote: That constant factor is interesting. > Also, could you express this factor in terms of existing constants? > Actually, I just peeked > at qcms_modular_transform_create_input and saw the same literal there. I find > this constant very odd. I am guessing it is somehow related to the epsilon of > the fixed-point type 2*(1-1/(2^16)), but there is no explanation, which is a bit > disconcerting. Exactly, it is indeed very odd, and thanks for looking into it. I'm unsure of what it's about, I found no qcms change log that describes it "why", etc. I added what we do know to a comment. Then I remove the static profile, patch #2 uploaded.
On 2015/12/03 03:14:51, noel gordon wrote: > > Yes it is, always. We currently create profiles on the Blink main thread only. > And that'd be where we call this code, so we'd be thread-safe in our use-case. > > More generally, profiles and transforms are created on our main thread. But > these profiles and transforms may be used on any thread _when they are being > applied_ to do color correction. And there we need thread-saftey, and qcms is > indeed thread-safe in application (when being applied), in all cases. Okay, I buy that, but we should really look into adding debug thread checks, something like what is in src/base/threading/thread_checker* Decoding images and therefore decoding input profiles and computing transforms off the main thread is not a far fetched idea. I have nothing further. lgtm
On 2015/12/03 03:39:20, Justin Novosad wrote: > On 2015/12/03 03:14:51, noel gordon wrote: > > > > Yes it is, always. We currently create profiles on the Blink main thread > only. > > And that'd be where we call this code, so we'd be thread-safe in our use-case. > > > > More generally, profiles and transforms are created on our main thread. But > > these profiles and transforms may be used on any thread _when they are being > > applied_ to do color correction. And there we need thread-saftey, and qcms is > > indeed thread-safe in application (when being applied), in all cases. > > Okay, I buy that, but we should really look into adding debug thread checks, > something like what is in src/base/threading/thread_checker* Interesting: qcms calling into src/base. > Decoding images and therefore decoding input profiles and computing transforms > off the main thread is not a far fetched idea. Agree, and we do do that already ... in deferred image decoding and have TSAN. > I have nothing further. lgtm Thanks, I go submit. Nothing using this code yet, mind so no change in behavior. Robert@ bleep if you have more questions, please.
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/1494473003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494473003/20001
> On 2015/12/03 03:14:51, noel gordon wrote: > > > > Yes it is, always. We currently create profiles on the Blink main thread only. I was plain wrong here. Deferred Image Decoding creates profiles / transforms on the impl-side-thread, and has been doing that for a long time now.
Message was sent while issue was closed.
Description was changed from ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249]. BUG=564355 ========== to ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249]. BUG=564355 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249]. BUG=564355 ========== to ========== [qcms] Add a color profile white point transform api Use the chain tool to compute how a given color profile transforms white input to XYZ PCS (profile connection space). No change in behavior, no new tests. Tested with an iMac P3 profile: the returned XYZ result is exactly D50 white: [0.9642, 1.0000, 0.8249]. BUG=564355 Committed: https://crrev.com/cba2c8d41687786279d539071b3b957423f43cb1 Cr-Commit-Position: refs/heads/master@{#362927} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cba2c8d41687786279d539071b3b957423f43cb1 Cr-Commit-Position: refs/heads/master@{#362927}
Message was sent while issue was closed.
On 2015/12/03 07:24:14, noel gordon wrote: > > On 2015/12/03 03:14:51, noel gordon wrote: > > > > > > Yes it is, always. We currently create profiles on the Blink main thread > only. > > I was plain wrong here. Deferred Image Decoding creates profiles / transforms > on the impl-side-thread, and has been doing that for a long time now. but all is good now with the static var removed. Yay!
Message was sent while issue was closed.
On 2015/12/03 18:20:10, Justin Novosad wrote: > On 2015/12/03 07:24:14, noel gordon wrote: > > > > I was plain wrong here. Deferred Image Decoding creates profiles / transforms > > on the impl-side-thread, and has been doing that for a long time now. > > but all is good now with the static var removed. Yay! Yeap. The main-thread creates profiles / transforms too, so yes, being thread-safe everywhere in qcms is required.
Message was sent while issue was closed.
+ // A properly created color profile should produce Y=~1.0 in PCS XYZ with white + // input (the D50 test). If it does not, then the profile is likely bogus. We should fix this comment sometime, if it implies that only Y matters in a bogus test. X Y Z all matter in a bogus test: the XYZ should match D50 XYZ to some tolerance. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
