Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952063002:
Create SkColorSpaceXform to handle color conversions (Closed)
DescriptionCreate SkColorSpaceXform to handle color conversions
Also adds testing of qcms color correction, so we can compare
SkColorSpaceXform outputs to qcms outputs.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1952063002
Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c
Committed: https://skia.googlesource.com/skia/+/9876ac5b3016e5353c072378ac1545a0a2270757
Patch Set 1 : #Patch Set 2 : Comments #
Total comments: 9
Patch Set 3 : Fix gamma bug #Patch Set 4 : Response to comments #
Total comments: 5
Patch Set 5 : Multiply by the inverse of 255.0f #
Total comments: 7
Patch Set 6 : Rebase and compare with qcms #Patch Set 7 : Add private API to allow test code to access ICC data #
Total comments: 10
Patch Set 8 : Response to comments #Patch Set 9 : Add documentation about not premultiplying #Patch Set 10 : Rebase #Patch Set 11 : Disable qcms tests on Google3 #Patch Set 12 : Rebase #
Messages
Total messages: 55 (22 generated)
Description was changed from ========== Create SkColorSpaceXform to handle color conversions (attempt 2) BUG=skia: ========== to ========== Create SkColorSpaceXform to handle color conversions (attempt 2) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + brianosman@google.com, herb@google.com, mtklein@google.com, reed@google.com, scroggo@google.com
Here's a new design that treats the color conversion as a separate pass on a buffer. The test code should at least look somewhat familiar. https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:26: if (srcSpace->gammas().fRed.isValue() && srcSpace->gammas().fGreen.isValue() && It's technically possible for an image to have an fRed gamma value and some other representation of gamma for green and blue. Though I've never seen this in practice.
https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:1025: // prettier than normal) if there are no bugs in our conversions. Thanks for the better comment here. Like Leon, I was initially confused by the discrepancy between what the code was supposedly testing and what it did. I figured it out, but this makes it very clear for people that wander into this code. https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.h (right): https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.h:36: * Apply the color conversion to a src buffer, storing the output in the dst buffer. Should probably document that this assumes (requires) RGBA_8888 buffers.
lgtm https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:988: switch (r) { Should be an if statement. https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:12: SkColorSpaceXform* SkColorSpaceXform::New(sk_sp<SkColorSpace> srcSpace, Is there a reason you are forcing this to be built on the heap?
On 2016/05/05 14:44:51, herb_g wrote: > lgtm > > https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp > File dm/DMSrcSink.cpp (right): > > https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp#newcod... > dm/DMSrcSink.cpp:988: switch (r) { > Should be an if statement. > > https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... > File src/core/SkColorSpaceXform.cpp (right): > > https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... > src/core/SkColorSpaceXform.cpp:12: SkColorSpaceXform* > SkColorSpaceXform::New(sk_sp<SkColorSpace> srcSpace, > Is there a reason you are forcing this to be built on the heap? I clicked the wrong button. I didn't mean lgtm. I know. Lame.
https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:988: switch (r) { On 2016/05/05 14:44:50, herb_g wrote: > Should be an if statement. Yes! SGTM :) https://codereview.chromium.org/1952063002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:1025: // prettier than normal) if there are no bugs in our conversions. On 2016/05/05 13:29:44, Brian Osman wrote: > Thanks for the better comment here. Like Leon, I was initially confused by the > discrepancy between what the code was supposedly testing and what it did. I > figured it out, but this makes it very clear for people that wander into this > code. Acknowledged. https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:12: SkColorSpaceXform* SkColorSpaceXform::New(sk_sp<SkColorSpace> srcSpace, On 2016/05/05 14:44:50, herb_g wrote: > Is there a reason you are forcing this to be built on the heap? It's nice to be able to return nullptr on unimplemented/invalid transforms. Additionally, this allows us to return a generic pointer to an SkColorSpaceXform (hiding the implementation details of each specific transform). An alternative might be to return a function pointer here - I think Mike was also imagining something that is stateless. This would require passing dstSpace and srcSpace to the xform function. I think this a nice idea. My issue with it is a little bit of messiness. We would either need to: (1) Pass dstSpace and srcSpace both to New() and to the actual function ptr. or (2) Don't pass srcSpace or dstSpace to New(), but end up with a couple giant procs that must check the characteristics of the LUT, gamma, xyz before choosing which codepath to follow. https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.h (right): https://codereview.chromium.org/1952063002/diff/40001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.h:36: * Apply the color conversion to a src buffer, storing the output in the dst buffer. On 2016/05/05 13:29:44, Brian Osman wrote: > Should probably document that this assumes (requires) RGBA_8888 buffers. Yes you're right! I think it makes sense to start with RGBA_8888->F16, but we could always add additional functions here (ex: RGBA_8888->10_10_10). In that case, we might want a more descriptive name as well (maybe apply8888ToF16 or something).
https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:50: size_t dstRowBytes, size_t srcRowBytes) const { Do you plan to use this to put bytes in the cache or do you expect the blitters to use this function to transform pixels? https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:56: float r = ((rgba >> 0) & 0xFF) / 255.0f; instead of x / 255.0f it's better to use x * (1.0f/255.0f).
https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:50: size_t dstRowBytes, size_t srcRowBytes) const { On 2016/05/05 17:36:42, herb_g wrote: > Do you plan to use this to put bytes in the cache or do you expect the blitters > to use this function to transform pixels? My original plan was to apply this transform at decode time. I think moving the transform to a separate pass gives us the flexibility to do it at decode time (and cache F16) or to cache the raw bytes and apply it later. https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:56: float r = ((rgba >> 0) & 0xFF) / 255.0f; On 2016/05/05 17:36:42, herb_g wrote: > instead of x / 255.0f it's better to use x * (1.0f/255.0f). SGTM! Better in terms of performance? I expect this routine to be very slow... I'm hoping to get the design and correctness right first before working on optimized code.
https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... File src/core/SkColorSpaceXform.cpp (right): https://codereview.chromium.org/1952063002/diff/80001/src/core/SkColorSpaceXf... src/core/SkColorSpaceXform.cpp:56: float r = ((rgba >> 0) & 0xFF) / 255.0f; On 2016/05/05 17:47:10, msarett wrote: > On 2016/05/05 17:36:42, herb_g wrote: > > instead of x / 255.0f it's better to use x * (1.0f/255.0f). > > SGTM! > > Better in terms of performance? I expect this routine to be very slow... I'm > hoping to get the design and correctness right first before working on optimized > code. Yep, better for performance. In this case the more interesting reason to do this is to be consistent with all the other places we *(1/255.0f) instead of /255.0f. The multiply is much faster, but delivers slightly different results... so even in places where speed doesn't matter too much, it's nice to consistently multiply. One less variable to have to consider.
https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:997: sk_sp<SkColorSpace> srcSpace = sk_ref_sp(codec->getColorSpace()); Should we update the API to return a const sk_sp<SkColorSpace>& ? Then you won't need to call sk_ref_sp, IIUC. https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1005: return Error::Nonfatal("Xform is not implemented yet."); Alternatively, should we only upload an image for this test once this is implemented? https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1018: // to be in a non-linear dst color space when we write the pixels to the screen. It's strange that we are talking about writing to a screen when this test never does that. Instead, we'll write a new PNG (without color correction?) that you'll later look at on a screen in Gold (which may or may not do color correction?). Is that all okay?
https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:997: sk_sp<SkColorSpace> srcSpace = sk_ref_sp(codec->getColorSpace()); On 2016/05/05 20:57:26, scroggo wrote: > Should we update the API to return a const sk_sp<SkColorSpace>& ? Then you won't > need to call sk_ref_sp, IIUC. We *should* do that. We actually can't because SkColorSpace.h isn't in include, and we forward declare it in SkCodec.h. I don't think we can make this change until SkColorSpace.h moves to include. https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1005: return Error::Nonfatal("Xform is not implemented yet."); On 2016/05/05 20:57:26, scroggo wrote: > Alternatively, should we only upload an image for this test once this is > implemented? I believe that's how it works now. The only images in kSrcToLinear_Mode that will be uploaded are those where we have implemented the transform. https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1018: // to be in a non-linear dst color space when we write the pixels to the screen. On 2016/05/05 20:57:26, scroggo wrote: > It's strange that we are talking about writing to a screen when this test never > does that. > > Instead, we'll write a new PNG (without color correction?) that you'll later > look at on a screen in Gold (which may or may not do color correction?). Is that > all okay? Agreed I'll go back and explain this further... We are writing to unmarked pngs. We hope that the browser will not do any color correction on the unmarked pngs. It's likely that it will assume unmarked means sRGB and that the monitor is sRGB (so no correction). I think we'd still be ok if the monitor is non-sRGB, but the browser does some correction from sRGB to the monitor space - this would be weird though...
https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/100001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1005: return Error::Nonfatal("Xform is not implemented yet."); On 2016/05/05 21:34:26, msarett wrote: > On 2016/05/05 20:57:26, scroggo wrote: > > Alternatively, should we only upload an image for this test once this is > > implemented? > > I believe that's how it works now. > > The only images in kSrcToLinear_Mode that will be uploaded are those where we > have implemented the transform. In that case, shouldn't this be a fatal error?
Description was changed from ========== Create SkColorSpaceXform to handle color conversions (attempt 2) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create SkColorSpaceXform to handle color conversions BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #6 (id:120001) has been deleted
Please take a look. I've rebased this and taken it in a new direction. It looks like we will at least "some" of the time want to do the full color conversion at decode time. Which is what qcms does for Chrome right now. I'd like to build SkColorSpaceXform to be a replacement for qcms (hopefully faster and with more support). This could be a good first step toward getting this work into Chrome.
1. just 32bit sees a little restrictive, but maybe that's fine? 2. do we really need an object, instead of just a static method that does the job?
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
> 1. just 32bit sees a little restrictive, but maybe that's fine? Yeah it is restrictive, I'm envisioning different functions for different output formats. I've only added a 32-bit version because that's the only one we need right now. Even if we decided to handle multiple possibilities in a single function, I think this is an ok way to start. > 2. do we really need an object, instead of just a static method that does the > job? I've implemented this xform to be a single row at a time, which we need in order to replace qcms (and in general makes sense for Chrome, because they display images incrementally). IMO the set-up work (concatting the matrices, inverting some gammas) is significant enough that we only want to do it once - though I have no data. So that's why I'm using an object with some state. I bet that we'll eventually want buffer->buffer xforms, and I think those will work fine as static procs. --------------------------------------------------- I've added a protected API to SkCodec, so we can get at the original profile data (and test it with qcms). It's a little messy, but should be temporary.
Can you add a comment in the commit message regarding qcms? https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:886: sk_sp<SkColorSpace> dstSpace = SkColorSpace::NewICC(dstData->data(), dstData->size()); Should we fail gracefully if dstData is null? https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:920: qcms_profile_release(srcSpace); Can you put these in an auto deleter? https://codereview.chromium.org/1952063002/diff/200001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1952063002/diff/200001/include/codec/SkCodec.... include/codec/SkCodec.h:654: * Used for testing with qcms. I think you said this was temporary? Should there be a FIXME? https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:227: colorSpace = SkColorSpace::NewICC(iccData->data(), iccData->size()); Is it okay that iccData could be non-NULL but colorSpace could be NULL? https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:258: JpegDecoderMgr* decoderMgr, sk_sp<SkColorSpace> colorSpace, sk_sp<SkData> iccData, It's a little weird to me that this is in between two parameters that get passed to INHERITED. I think it'd be better to put it at the end.
Description was changed from ========== Create SkColorSpaceXform to handle color conversions BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
> Can you add a comment in the commit message regarding qcms? Yes! Done. https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:886: sk_sp<SkColorSpace> dstSpace = SkColorSpace::NewICC(dstData->data(), dstData->size()); On 2016/05/31 21:13:34, scroggo wrote: > Should we fail gracefully if dstData is null? Yes I think so. Let's fail, but not crash, if the resource path isn't set properly. https://codereview.chromium.org/1952063002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:920: qcms_profile_release(srcSpace); On 2016/05/31 21:13:34, scroggo wrote: > Can you put these in an auto deleter? Done. https://codereview.chromium.org/1952063002/diff/200001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1952063002/diff/200001/include/codec/SkCodec.... include/codec/SkCodec.h:654: * Used for testing with qcms. On 2016/05/31 21:13:34, scroggo wrote: > I think you said this was temporary? Should there be a FIXME? Yes I think so. https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:227: colorSpace = SkColorSpace::NewICC(iccData->data(), iccData->size()); On 2016/05/31 21:13:34, scroggo wrote: > Is it okay that iccData could be non-NULL but colorSpace could be NULL? Yes. That could mean that the image stored a garbage icc profile or one that we aren't smart enough interpret. Either way I think the right thing to do is guess sRGB. It'd be nice to catch profiles where this happens, but I'm not really sure how. I'll at least add a SkCodecPrintf. https://codereview.chromium.org/1952063002/diff/200001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:258: JpegDecoderMgr* decoderMgr, sk_sp<SkColorSpace> colorSpace, sk_sp<SkData> iccData, On 2016/05/31 21:13:34, scroggo wrote: > It's a little weird to me that this is in between two parameters that get passed > to INHERITED. I think it'd be better to put it at the end. Done.
1. Doh, I didn't notice it was just 1 row. Makes sense to create a long-lived object. 2. Does the output have to be premultiplied? Is that easier/harder for the transformation, given that the src might be unpremul (e.g. png input)? 3. its a private api, so I'm not going to stress-out too much :)
> 2. Does the output have to be premultiplied? Is that easier/harder for the > transformation, given that the src might be unpremul (e.g. png input)? Good question. I've been thinking about jpegs, so it hadn't even crossed my mind. Given that we are going all the way from src to dst, and that we should premultiply while we're linear, I think this definitely should at least be able to premultiply. But there should also be an option that skips the premultiply (ex: if the image is opaque, or if the gpu might want unpremul). For this CL, I'll just mark this transform as non-premultiplying.
lgtm
We may need the premul option, but at least we are well documented at the moment. lgtm
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952063002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952063002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from herb@google.com Link to the patchset: https://codereview.chromium.org/1952063002/#ps240001 (title: "Add documentation about not premultiplying")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952063002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952063002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for include/codec/SkCodec.h:
While running git apply --index -3 -p1;
error: patch failed: include/codec/SkCodec.h:23
Falling back to three-way merge...
Applied patch to 'include/codec/SkCodec.h' with conflicts.
U include/codec/SkCodec.h
Patch: include/codec/SkCodec.h
Index: include/codec/SkCodec.h
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index
50875e46c4a42cfbef1d204a0af3bfa01a74b38c..be1dc43432fe7eab0cd4bc72d29c3f55fcaccdcd
100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -23,6 +23,10 @@ class SkData;
class SkPngChunkReader;
class SkSampler;
+namespace DM {
+class ColorCodecSrc;
+}
+
/**
* Abstraction layer directly on top of an image codec.
*/
@@ -646,6 +650,11 @@ protected:
virtual int onOutputScanline(int inputScanline) const;
+ /**
+ * Used for testing with qcms.
+ * FIXME: Remove this when we are done comparing with qcms.
+ */
+ virtual sk_sp<SkData> getICCData() const { return nullptr; }
private:
const SkEncodedInfo fEncodedInfo;
const SkImageInfo fSrcInfo;
@@ -709,6 +718,10 @@ private:
*/
virtual SkSampler* getSampler(bool /*createIfNecessary*/) { return nullptr;
}
+ // For testing with qcms
+ // FIXME: Remove this when we are done comparing with qcms.
+ friend class DM::ColorCodecSrc;
+
friend class SkSampledCodec;
friend class SkIcoCodec;
};
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from herb@google.com, scroggo@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1952063002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952063002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952063002/260001
Message was sent while issue was closed.
Description was changed from ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:260001) has been created in https://codereview.chromium.org/2023093004/ by msarett@google.com. The reason for reverting is: Google3 can't find qcms.
Message was sent while issue was closed.
Description was changed from ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c ========== to ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c ==========
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952063002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952063002/280001
I've disabled the qcms tests on Google3. I don't really see any reason to try to get those working, given that we intend to delete qcms anyway. Leon, is there a risk of your png codec CL getting reverted? If so, I won't land until that is stable, because this is based on those changes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/01 16:53:54, msarett wrote: > I've disabled the qcms tests on Google3. I don't really see > any reason to try to get those working, given that we intend > to delete qcms anyway. > > Leon, is there a risk of your png codec CL getting reverted? > If so, I won't land until that is stable, because this is > based on those changes. Yep, I'm reverting it right now :( (FWIW, this is exactly the kind of problem I was trying to avoid by always using the same version of libpng. But CMake is the exception...) The revert has landed, so feel free to land this one and I'll rebase. lgtm
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from herb@google.com, reed@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1952063002/#ps300001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952063002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952063002/300001
Message was sent while issue was closed.
Description was changed from ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c ========== to ========== Create SkColorSpaceXform to handle color conversions Also adds testing of qcms color correction, so we can compare SkColorSpaceXform outputs to qcms outputs. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/740cc88ee3d63c75e52d31238f2a32600cc57a8c Committed: https://skia.googlesource.com/skia/+/9876ac5b3016e5353c072378ac1545a0a2270757 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as https://skia.googlesource.com/skia/+/9876ac5b3016e5353c072378ac1545a0a2270757 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
