Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214343002:
[qcms] Keep the output of the TRC between 0 and 1 (Closed)
|
Created:
5 years, 5 months ago by Noel Gordon Modified:
5 years, 5 months ago Reviewers:
robert.bradford CC:
chromium-reviews, Ken Russell (switch to Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[qcms] Keep the output of the TRC between 0 and 1
Section 10.16: The domain and range of each function shall be [0,0 1,0].
Any function value outside the range shall be clipped to the range of
the function.
https://bugzilla.mozilla.org/show_bug.cgi?id=764181
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel,mac_blink_rel,win_blink_rel
BUG=498321
Committed: https://crrev.com/9491f8683c4f4f3e2211e77fba08d6ec524218fc
Cr-Commit-Position: refs/heads/master@{#337189}
Patch Set 1 : #
Total comments: 17
Patch Set 2 : Rebase to HEAD. #
Total comments: 3
Messages
Total messages: 53 (10 generated)
noel@chromium.org changed reviewers: + mgiuca@chromium.org
Another one I noticed we don't have when I was reviewing your other qcms changed.
.. or change even.
Patchset #1 (id:1) has been deleted
Hi Noel, It looks like there are some minor discrepancies between the upstream patch (https://hg.mozilla.org/mozilla-central/rev/a41fd66f1245) and your patch here. How did you apply the patch? I'm wondering if you applied it by hand and fixed some of these issues manually. I think the correct behaviour in a third-party library is to remain as close as possible to the upstream, so you should not be fixing these small issues here. Can you change the places I've noted to exactly match the upstream patch? https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:39: // the output range of this function is 0..1 The upstream patch says "range of this functions". https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:40: float lut_interp_linear(double input_value, uint16_t *table, size_t length) Why is length size_t instead of int? (The upstream is int.) Is this a local bug fix? Could it be upstreamed? https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:130: // 0..1^(0..255 + 255/256) will always be between 0 and 1 The upstream patch has 16 spaces here, not 2 tabs. :( https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:135: void compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, size_t length) The upstream patch doesn't change "int length" to "size_t length". https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.h (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.h:34: float lut_interp_linear(double input_value, uint16_t *table, size_t length); The upstream patch doesn't change these names in the header file.
On 2015/07/01 01:47:00, Matt Giuca wrote: > Hi Noel, > > It looks like there are some minor discrepancies between the upstream patch > (https://hg.mozilla.org/mozilla-central/rev/a41fd66f1245) and your patch here. Yeap, on purpose. > How did you apply the patch? > I'm wondering if you applied it by hand and fixed some of these issues manually. Yeap. > I think the correct behaviour in a third-party library is to remain as close as > possible to the upstream, so you should not be fixing these small issues here. > Can you change the places I've noted to exactly match the upstream patch? What is the canonical upstream? Answ: a pita as you noted when you changed qcms. qcms v4 git is not merged to its master, it is merged into moz hg, but subsequent changes submitted to moz hg are not been merged back to the qcms git master or its v4 branch. ¯\(ツ)/¯ moz hg is the closest I have to something ~canonical.
https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.h (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.h:34: float lut_interp_linear(double input_value, uint16_t *table, size_t length); On 2015/07/01 01:46:59, Matt Giuca wrote: > The upstream patch doesn't change these names in the header file. Well, "consistency" maybe.
https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:39: // the output range of this function is 0..1 On 2015/07/01 01:46:59, Matt Giuca wrote: > The upstream patch says "range of this functions". well "... the output range of this functions is 0..1", to be precise and we prefer English. https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:40: float lut_interp_linear(double input_value, uint16_t *table, size_t length) On 2015/07/01 01:46:59, Matt Giuca wrote: > Why is length size_t instead of int? (The upstream is int.) Is this a local bug > fix? Could it be upstreamed? Local bug fix, upstream was advised iirc and is still not fixed. We want size_t here. https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:130: // 0..1^(0..255 + 255/256) will always be between 0 and 1 On 2015/07/01 01:46:59, Matt Giuca wrote: > The upstream patch has 16 spaces here, not 2 tabs. > > :( Acknowledged. The qcms story re tabs and spaces is not good and it comes up in moz reviews often [1]. So for at least some consistency, tabs win sadly. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=678324#c23 https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:135: void compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, size_t length) On 2015/07/01 01:46:59, Matt Giuca wrote: > The upstream patch doesn't change "int length" to "size_t length". Refer back to the reply for line 40, we want size_t.
> What is the canonical upstream? Answ: a pita as you noted when you changed > qcms. qcms v4 git is not merged to its master, it is merged into moz hg, but > subsequent changes submitted to moz hg are not been merged back to the qcms git > master or its v4 branch. ¯\(ツ)/¯ moz hg is the closest I have to something > ~canonical. At this point, I'm considering moz hg to be "canonical" upstream (since github v4 doesn't even have this file at all!). Therefore, we should try to remain as close as possible to the upstream sources, so if some day someone wants to try and update the local copy to match upstream, they do not have to deal with hundreds of minor spelling corrections and so on. (Because this person would have to basically do a 3-way merge between github v4 as of 2012-03-13, the current upstream, and the current Chromium local copy, and any minor difference between upstream and local will result in an unnecessary merge conflict.) None of the changes you made are important (they are minor nits which we would want to fix if this was our code, but they are not worth applying to third-party code).
https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/README... File third_party/qcms/README.chromium (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/README... third_party/qcms/README.chromium:84: - Apply Keep the output of the TRC between 0 and 1 Sorry: you'll have to rebase against http://crrev.com/336970.
On 2015/07/01 04:14:48, Matt Giuca wrote: > > What is the canonical upstream? Answ: a pita as you noted when you changed > > qcms. qcms v4 git is not merged to its master, it is merged into moz hg, but > > subsequent changes submitted to moz hg are not been merged back to the qcms > git > > master or its v4 branch. ¯\(ツ)/¯ moz hg is the closest I have to something > > ~canonical. > > At this point, I'm considering moz hg to be "canonical" upstream That is also my conclusion. > (since github v4 doesn't even have this file at all!). /much sadness > Therefore, we should try to remain as close as possible to the upstream sources, That is the approach I take. Moreover, when we find issues, we advise by filing mozilla bug about them ... > so if some day someone wants to try and update the local copy to match upstream, > they do not have to deal with hundreds of minor spelling corrections and so on. > (Because this person would have to basically do a 3-way merge between github v4 > as of 2012-03-13, the current upstream, and the current Chromium local copy, and > any minor difference between upstream and local will result in an unnecessary > merge conflict.) and do a three-way merge / examine dance because of a the lack of a canonical qcms repo. > None of the changes you made are important (they are minor nits which we would > want to fix if this was our code, but they are not worth applying to third-party > code). Nope, the size_t part of the fix is important, so is limiting TRC values b/w 0 and 1. We definitely want this fix in Chrome.
https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/README... File third_party/qcms/README.chromium (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/README... third_party/qcms/README.chromium:84: - Apply Keep the output of the TRC between 0 and 1 On 2015/07/01 04:30:18, Matt Giuca wrote: > Sorry: you'll have to rebase against http://crrev.com/336970. Yeap and it's why I asked you to review in the first place - so that our changes did not bump.
Rebased, running dry run trys again to verify there is no changes in behavior in blink.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214343002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/61335)
Four mac reflection tests change, the changes look fine to me https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/61...
On 2015/07/01 04:35:51, noel gordon wrote: > > At this point, I'm considering moz hg to be "canonical" upstream > > That is also my conclusion. And confirmed here: https://github.com/jrmuizel/qcms/pull/4 > > Therefore, we should try to remain as close as possible to the upstream > sources, > > That is the approach I take. But correcting spelling mistakes and formatting is not remaining as close as possible to the upstream. > Moreover, when we find issues, we advise by filing mozilla bug about them ... Noted on the size_t issue. > and do a three-way merge / examine dance because of a the lack of a canonical > qcms repo. Well apparently the mozilla-central repo is canonical. I'm not sure what this has to do with the three-way merge. > Nope, the size_t part of the fix is important, so is limiting TRC values b/w 0 > and 1. We definitely want this fix in Chrome. OK, if the size_t is important, then we can land that. The other changes (renaming args in the header, grammar mistakes, formatting) do not matter, they're just nit-picks. On third-party code, remaining as close as possible to upstream trumps any other style rules (theirs or ours). https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:39: // the output range of this function is 0..1 If this were an ordinary code review, I'd tell the author to fix his grammar. But you aren't the author, you are just checking in code, and therefore you shouldn't change it unless you have a good reason. (This is not our code so it is not up to us to decide what grammar standards to uphold.) https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:40: float lut_interp_linear(double input_value, uint16_t *table, size_t length) On 2015/07/01 04:12:22, noel gordon wrote: > On 2015/07/01 01:46:59, Matt Giuca wrote: > > Why is length size_t instead of int? (The upstream is int.) Is this a local > bug > > fix? Could it be upstreamed? > > Local bug fix, upstream was advised iirc and is still not fixed. We want size_t > here. Acknowledged. https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:130: // 0..1^(0..255 + 255/256) will always be between 0 and 1 Absolutely ... if you were writing code for Mozilla you should be consistent with the surrounding code (and use tabs). But you aren't writing this code, you are just checking it in to Chromium, and therefore you should not change it unless you have a good reason to do so. https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:135: void compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, size_t length) On 2015/07/01 04:12:22, noel gordon wrote: > On 2015/07/01 01:46:59, Matt Giuca wrote: > > The upstream patch doesn't change "int length" to "size_t length". > > Refer back to the reply for line 40, we want size_t. Acknowledged. https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.h (right): https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.h:34: float lut_interp_linear(double input_value, uint16_t *table, size_t length); It would be consistent with the .c file to use the "input_value" name, but it is more important to remain compatible with the upstream code.
On 2015/07/01 06:41:05, noel gordon wrote: > Four mac reflection tests change, the changes look fine to me > https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/61... Submitted https://src.chromium.org/viewvc/blink?revision=198122&view=revision to skip them temporarily. Rerunning the trys again.
The CQ bit was checked by noel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214343002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> On 2015/07/01 04:35:51, noel gordon wrote: > > > At this point, I'm considering moz hg to be "canonical" upstream > > > > That is also my conclusion. > > And confirmed here: > > https://github.com/jrmuizel/qcms/pull/4 Not really open source is it. > > > Therefore, we should try to remain as close as possible to the upstream > > sources, > > > > That is the approach I take. > > But correcting spelling mistakes and formatting is not remaining as close as > possible to the upstream. "close" does not mean the same. Chrome has had to move on and has added to qcms as our needs changed (see the README). The chrome version is canonical to us. If there were changes to https://bugzilla.mozilla.org/buglist.cgi?quicksearch=color%20management&list_... that were valuable, we would consider them, though the list appears to be unowned. > > Moreover, when we find issues, we advise by filing mozilla bug about them ... > > Noted on the size_t issue. > > > and do a three-way merge / examine dance because of a the lack of a canonical > > qcms repo. > > Well apparently the mozilla-central repo is canonical. I'm not sure what this > has to do with the three-way merge. Canonical for _them_. Chrome has moved on: ours is canonical and modified for our uses inside / outside of Chrome. > > Nope, the size_t part of the fix is important, so is limiting TRC values b/w 0 > > and 1. We definitely want this fix in Chrome. > > OK, if the size_t is important, then we can land that. Having TRC b/w 0..1 as required by the ICC spec. If you're saying not to land that, then imho that'd be silly and ill-advised. > The other changes (renaming args in the header, grammar mistakes, formatting) do > not matter, they're just nit-picks. On third-party code, remaining as close as > possible to upstream trumps any other style rules (theirs or ours). Chrome move on (there was no canonical open source repository). And the various things you say don't not matter, do in fact matter and matter to the external users that have needed to expand qcms capabilities inside / outside of Chrome. > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > File third_party/qcms/src/transform_util.c (right): > > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.c:39: // the output range of this function > is 0..1 > If this were an ordinary code review, I'd tell the author to fix his grammar. > But you aren't the author, you are just checking in code, and therefore you > shouldn't change it unless you have a good reason. > (This is not our code so it is not up to us to decide what grammar standards to > uphold.) We are not "just checking in code". We are integrating a change so that it fits in our repo. > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.c:40: float lut_interp_linear(double > input_value, uint16_t *table, size_t length) > On 2015/07/01 04:12:22, noel gordon wrote: > > On 2015/07/01 01:46:59, Matt Giuca wrote: > > > Why is length size_t instead of int? (The upstream is int.) Is this a local > > bug > > > fix? Could it be upstreamed? > > > > Local bug fix, upstream was advised iirc and is still not fixed. We want > size_t > > here. > > Acknowledged. > > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.c:130: // 0..1^(0..255 + 255/256) will > always be between 0 and 1 > Absolutely ... if you were writing code for Mozilla you should be consistent > with the surrounding code (and use tabs). > > But you aren't writing this code, you are just checking it in to Chromium, and > therefore you should not change it unless you have a good reason to do so. We have ours reasons, we are not "just checking in code" here. > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.c:135: void > compute_curve_gamma_table_type2(float gamma_table[256], uint16_t *table, size_t > length) > On 2015/07/01 04:12:22, noel gordon wrote: > > On 2015/07/01 01:46:59, Matt Giuca wrote: > > > The upstream patch doesn't change "int length" to "size_t length". > > > > Refer back to the reply for line 40, we want size_t. > > Acknowledged. > > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > File third_party/qcms/src/transform_util.h (right): > > https://codereview.chromium.org/1214343002/diff/20001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.h:34: float lut_interp_linear(double > input_value, uint16_t *table, size_t length); > It would be consistent with the .c file to use the "input_value" name, but it is > more important to remain compatible with the upstream code. Again, we've moved on. Compatible with upstream (whatever that is) is less important. Indeed, we create patches from any where including other repositories like lcms for example, per crbug.com/504681 and https://codereview.chromium.org/1215583002
On 2015/07/01 09:38:25, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Ok good, adding Robert. Will sync up with you around your usual time London time. Run you eyes over this change please.
noel@chromium.org changed reviewers: + robert.bradford@intel.com - mgiuca@chromium.org
https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:131: gamma_table[i] = pow(i/255., gamma_float); I don't think we need to do the calculation in double precision? so what about: gamma_table[i] = powf(i/255.f, gamma_float);
On 2015/07/01 12:10:50, robert.bradford wrote: > https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... > File third_party/qcms/src/transform_util.c (right): > > https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... > third_party/qcms/src/transform_util.c:131: gamma_table[i] = pow(i/255., > gamma_float); > I don't think we need to do the calculation in double precision? > > so what about: > > gamma_table[i] = powf(i/255.f, gamma_float); Scratch that. I just did some runs of this code, 1M iterations of pow() vs powf() and powf() was slower (~13%). so lgtm.
Was a good question though and thanks for the data, Robert. Perf is currently good for V2 profiles. Need some perf work on V4 profiles, refer to https://bugzilla.mozilla.org/show_bug.cgi?id=538114#c28 for details. That bug became https://bugzilla.mozilla.org/show_bug.cgi?id=679527#c4 and 3 in-tree qcms patches landed that attempted to fix the problem by using floorf, ceilf, etc. Results were inconclusive but the bug was closed resolved. Not sure it's really fixed, or if we need those patches: there was no mention of how the changes affected colorimetric accuracy for example, and that'd be a concern. Having a faster floor / ceil in qcms is an interesting idea: they are used quite a lot (on every pixel RGB). Any speed-up there would improve perf V4 on the software path (non-SSE code path). Sent you some ideas on how to get a faster floor. You might have some others ideas to help us improve in that area?
On 2015/07/01 14:59:41, robert.bradford wrote: > Scratch that. I just did some runs of this code, 1M iterations of pow() vs > powf() and powf() was slower (~13%). so lgtm. Hmmm, maybe natural to expect that powf would be faster. I suppose we are automatically getting some SSE assist here under the hood. Is that correct?
On 2015/07/01 16:36:44, noel gordon wrote: > On 2015/07/01 14:59:41, robert.bradford wrote: > > Scratch that. I just did some runs of this code, 1M iterations of pow() vs > > powf() and powf() was slower (~13%). so lgtm. > > Hmmm, maybe natural to expect that powf would be faster. I suppose we are > automatically getting some SSE assist here under the hood. Is that correct? On all IA platforms (32 & 64-bit) chromium uses -msse2 -mfpath=sse which will use SSE instructions and registers for the floating point. The performance difference with pow() vs powf() is a glibc implementation detail: powf() has just a C implementation but pow() has SSE2 and FMA4 versions with runtime detection.
OK it sounds like your position on this is that third_party/qcms is no longer synced at all with upstream (it's essentially a fork, not a copy with patches which is how we normally treat third-party code). I don't understand why this is, but if that's the case, I think the README.chromium needs to be updated to reflect that. As it stands, the README suggests that it is following the normal third-party policy. Instead, it should state that we have diverged from the upstream and this is not expected to be synchronized with their code (though we can still pull in patches). On 2015/07/01 11:10:47, noel gordon wrote: > > https://github.com/jrmuizel/qcms/pull/4 > > Not really open source is it. Why do you say that? The source code is publicly available under a permissive license at: https://hg.mozilla.org/mozilla-central/file/tip/gfx/qcms > > > Nope, the size_t part of the fix is important, so is limiting TRC values b/w > 0 > > > and 1. We definitely want this fix in Chrome. > > > > OK, if the size_t is important, then we can land that. > > Having TRC b/w 0..1 as required by the ICC spec. If you're saying not to land > that, then imho that'd be silly and ill-advised. What? Maybe I misunderstood... isn't having TRC b/w 0..1 the change made by the upstream patch you are merging? Of course I'm not saying not to land that, that's the whole point of this patch. And, I'm saying that changing int to size_t is apparently important, so let's land that too. What I was pushing back on was the other changes which are by any measure unimportant: - Fixing grammar in comments, - Making .h and .c file argument names consistent, and - Fixing tab/space mixing. If we are trying to maintain a third-party clone in sync with an upstream repo, then those things should not be fixed locally. That was my understanding. But now you have explained that we have a divergent fork of QCMS, I think it's fine to make those changes, but we should also update the README to explain that we are not trying to stay in sync. > Chrome move on (there was no canonical open source repository). And the various > things you say don't not matter, do in fact matter and matter to the external > users that have needed to expand qcms capabilities inside / outside of Chrome. Again, when I said things that don't matter, I was referring to the cosmetic changes, not the things that affect the compiled code. > Again, we've moved on. Compatible with upstream (whatever that is) is less > important. Indeed, we create patches from any where including other > repositories like lcms for example, per crbug.com/504681 and > https://codereview.chromium.org/1215583002 That is a very different case, because it is inserting a small amount of code to fix a bug. The normal third-party policy is that local-only patches are OK, but they should not be superfluous.
On 2015/07/01 17:26:59, robert.bradford wrote: > On 2015/07/01 16:36:44, noel gordon wrote: > > On 2015/07/01 14:59:41, robert.bradford wrote: > > > Scratch that. I just did some runs of this code, 1M iterations of pow() vs > > > powf() and powf() was slower (~13%). so lgtm. > > > > Hmmm, maybe natural to expect that powf would be faster. I suppose we are > > automatically getting some SSE assist here under the hood. Is that correct? > > On all IA platforms (32 & 64-bit) chromium uses -msse2 -mfpath=sse which will > use SSE instructions and registers for the floating point. > > The performance difference with pow() vs powf() is a glibc implementation > detail: powf() has just a C implementation but pow() has SSE2 and FMA4 versions > with runtime detection. Right, and I see https://code.google.com/p/chromium/issues/detail?id=348761 has made SSE2 the minimum requirement for all chrome platforms. That makes the various sse1 code in qcms, and gyp / gn build file sse1 exceptions, ~redundant based on chrome platform minimum requirements.
On 2015/07/02 00:52:10, Matt Giuca wrote: > OK it sounds like your position on this is that third_party/qcms is no longer > synced at all with upstream There is no good definition of upstream. > I don't understand why this is, but if that's the case, Sad fact of life. > I think the README.chromium needs to be updated to reflect that. The README documents the source of the original code. > As it stands, the README suggests that it is following the normal third-party > policy. Instead, it should state that we have diverged from the upstream and > this is not expected to be synchronized with their code (though we can still > pull in patches). "Local Modifications:" The README states this by listing all our changes and from where they came.
On 2015/07/02 00:52:10, Matt Giuca wrote: > On 2015/07/01 11:10:47, noel gordon wrote: > > > https://github.com/jrmuizel/qcms/pull/4 > > > > Not really open source is it. > > Why do you say that? How about inactive open source whatever? Where we've had that problem in chrome before, well, see zlib.
On 2015/07/02 00:52:10, Matt Giuca wrote: > OK it sounds like your position on this is that third_party/qcms is no longer > synced at all with upstream (it's essentially a fork, not a copy with patches > which is how we normally treat third-party code). I don't understand why this > is, but if that's the case, I think the README.chromium needs to be updated to > reflect that. > > As it stands, the README suggests that it is following the normal third-party > policy. Instead, it should state that we have diverged from the upstream and > this is not expected to be synchronized with their code (though we can still > pull in patches). > > On 2015/07/01 11:10:47, noel gordon wrote: > > > https://github.com/jrmuizel/qcms/pull/4 > > > > Not really open source is it. > > Why do you say that? The source code is publicly available under a permissive > license at: > https://hg.mozilla.org/mozilla-central/file/tip/gfx/qcms > > > > > Nope, the size_t part of the fix is important, so is limiting TRC values > b/w > > 0 > > > > and 1. We definitely want this fix in Chrome. > > > > > > OK, if the size_t is important, then we can land that. > > > > Having TRC b/w 0..1 as required by the ICC spec. If you're saying not to land > > that, then imho that'd be silly and ill-advised. > > What? Maybe I misunderstood... isn't having TRC b/w 0..1 the change made by the > upstream patch you are merging? Of course I'm not saying not to land that, > that's the whole point of this patch. Good, but that is what you said. "OK, if the size_t is important, then we can land that" which missed the 0..1 stuff. Misunderstanding I suppose.
On 2015/07/02 00:52:10, Matt Giuca wrote: No questions in the remainder.
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/1214343002/40001
On 2015/07/01 14:59:41, robert.bradford wrote: > so lgtm. Thanks.
https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:131: gamma_table[i] = pow(i/255., gamma_float); On 2015/07/01 12:10:50, robert.bradford wrote: > I don't think we need to do the calculation in double precision? > > so what about: > > gamma_table[i] = powf(i/255.f, gamma_float); > Resolved by your subsequent measurements: use pow. If there's anything perf-wise we should be expending our time on, it'd be a faster floor.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2015/07/02 06:08:42, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) Filed https://code.google.com/p/chromium/issues/detail?id=506467
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/1214343002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9491f8683c4f4f3e2211e77fba08d6ec524218fc Cr-Commit-Position: refs/heads/master@{#337189}
Message was sent while issue was closed.
Sad fact of life: we should focus on the code ... https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... File third_party/qcms/src/transform_util.c (right): https://codereview.chromium.org/1214343002/diff/40001/third_party/qcms/src/tr... third_party/qcms/src/transform_util.c:194: if (X >= interval) { Oh and spot the bug: interval is a float on 0.0 .. 1.0 and X is a size_t on [0..255]. I mean, what could possibly go wrong? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
