Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787053004:
Respect colorSpace in DecodingImageGenerator::onGetPixels() (Closed)
|
Created:
3 years, 8 months ago by msarett Modified:
3 years, 8 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, fmalita+watch_chromium.org, jbroman, Justin Novosad, kinuko+watch, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRespect colorSpace in DecodingImageGenerator::onGetPixels()
ImageDecoder.cpp
In case the of a rare image with an embedded color space that
is not parametric, we will do a color space xform at decode
time. Otherwise, leave it as is.
DecodingImageGenerator.cpp
Based on cc behavior, we should almost always be asking for
the SkImage "as is" (in the same color space that it is
already in). In the rare case that we miss the cc cache and
request it in a different color space, will need to do the
color space xform here.
We always use an unpremul decoder when we will be doing a
color space xform in a second pass. That way we don't
need to unpremultiply.
BUG=706613
Review-Url: https://codereview.chromium.org/2787053004
Cr-Commit-Position: refs/heads/master@{#464458}
Committed: https://chromium.googlesource.com/chromium/src/+/669daddab5f2f01c57add7ec00ab87fa8453b463
Patch Set 1 #Patch Set 2 : Add comment #
Total comments: 15
Patch Set 3 : Response to comments #Patch Set 4 : Basic approach #
Total comments: 8
Patch Set 5 : Response to comments #
Total comments: 7
Patch Set 6 : Response to comments #
Total comments: 37
Patch Set 7 : Moving xform back to image generator #
Total comments: 14
Patch Set 8 : Add hash support for std::tuple() #Patch Set 9 : Enourmous rebase #Patch Set 10 : Response to comments #
Total comments: 2
Patch Set 11 : AlphaOption #
Total comments: 10
Patch Set 12 : Write custom hash impl for DecoderCacheKey struct #
Total comments: 1
Patch Set 13 : Add trace #Patch Set 14 : Rebase #Messages
Total messages: 66 (33 generated)
msarett@chromium.org changed reviewers: + ccameron@chromium.org, fmalita@chromium.org, msarett@chromium.org, scroggo@chromium.org
https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:146: // |dst| color space. The Skia API for this conversion (non-linear I'll see if I can make this public. Regardless, this is a performance problem. Potential fixes: (1) Support kUnpremul decodes in ImageDecoder (2) Pass the dstColorSpace through to ImageDecoder and respect it. (3) Switch to SkCodec. (3) is the least throw-away work because that's the plan anyway. It will also probably take the longest. SkCodec also helps with the linear premultiply vs. non-linear premultiply problem because it supports both.
Thanks for looking at this. Doing the conversion at decode time would probably be the best in terms of performance (and re-use) ... I'm not sure if it's possible to plumb that all through or not (I'll have to re-page all of this into my brain to look). What's your feeling about the best way to do this in the short term? https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:146: // |dst| color space. The Skia API for this conversion (non-linear On 2017/03/31 19:05:28, msarett1 wrote: > I'll see if I can make this public. > > Regardless, this is a performance problem. Potential fixes: > (1) Support kUnpremul decodes in ImageDecoder > (2) Pass the dstColorSpace through to ImageDecoder and respect it. > (3) Switch to SkCodec. > > (3) is the least throw-away work because that's the plan anyway. It will also > probably take the longest. SkCodec also helps with the linear premultiply vs. > non-linear premultiply problem because it supports both. Doing (2) may be possible ... I'm looking to see about the order of creation of things. Also, would it be possible to just run an SkColorSpaceXform directly over the pixels here? https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:152: DCHECK(xformed); This DCHECK fires for the test image attached at https://bugs.chromium.org/p/chromium/issues/detail?id=706613#c6 I'll start digging in to see why.
I'm not familiar with color space subtleties, so I'll defer to the other reviewers for the actual change. Just some general comments. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:134: !SkColorSpace::Equals(decodeColorSpace.get(), dstInfo.colorSpace()))) { It reads a bit strange for this conditional to be different from the one setting up the temp buffer above - looks like we can land here with decodePixels == dstPixels, is that ok? I think it would be easier to follow if we made the decision once (bool requiresColorSpaceConversion = ...) and then stick to it. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:135: // FIXME: Nit: Chromium style is // TODO(msarett): ... https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:142: // FIXME: Nit: ditto https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:152: DCHECK(xformed); Nit: return xformed?
> Doing the conversion at decode time would probably be the best in terms of > performance (and re-use) ... I'm not sure if it's possible to plumb that all > through or not (I'll have to re-page all of this into my brain to look). > > What's your feeling about the best way to do this in the short term? I'll need to start re-learning this code too. I agree that decode time would be better for performance and re-use. I'm wondering if there are other code paths that are also depending on the decoder for these pixels (that might be expecting a different color space). The other problem with decode time is that we don't have color space xforms hooked into GIF, BMP, etc. Doing a second pass covers all of the codecs. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:146: // |dst| color space. The Skia API for this conversion (non-linear On 2017/03/31 19:46:08, ccameron wrote: > On 2017/03/31 19:05:28, msarett1 wrote: > > I'll see if I can make this public. > > > > Regardless, this is a performance problem. Potential fixes: > > (1) Support kUnpremul decodes in ImageDecoder > > (2) Pass the dstColorSpace through to ImageDecoder and respect it. > > (3) Switch to SkCodec. > > > > (3) is the least throw-away work because that's the plan anyway. It will also > > probably take the longest. SkCodec also helps with the linear premultiply vs. > > non-linear premultiply problem because it supports both. > > Doing (2) may be possible ... I'm looking to see about the order of creation of > things. > I looked at (2) first, then got scared and backed off. I'll probably talk with Leon (or someone who knows this code better). That may be a better approach. > Also, would it be possible to just run an SkColorSpaceXform directly over the > pixels here? Yeah that would be fine (except it doesn't support unpremul then re-premul). That's pretty much what SkPixmap::readPixels() will do anyway. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:152: DCHECK(xformed); On 2017/03/31 19:46:08, ccameron wrote: > This DCHECK fires for the test image attached at > https://bugs.chromium.org/p/chromium/issues/detail?id=706613#c6 > > I'll start digging in to see why. SkPixmap::readPixels() has some restrictions on transfer functions that I forgot about. I could make it more flexible, but this might not be the right API to use here.
Will put this on hold to discuss the pros and cons of various approaches. FWIW, I've fixed the issue with the image causing the DCHECK(). And this looks a lot cleaner now that I've taken F16 support out of it. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:134: !SkColorSpace::Equals(decodeColorSpace.get(), dstInfo.colorSpace()))) { On 2017/03/31 19:52:07, f(malita) wrote: > It reads a bit strange for this conditional to be different from the one setting > up the temp buffer above - looks like we can land here with decodePixels == > dstPixels, is that ok? > > I think it would be easier to follow if we made the decision once (bool > requiresColorSpaceConversion = ...) and then stick to it. Yeah I can see how this is confusing. I'm dropping F16 support from this CL altogether... I think I was getting a little greedy :). https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:135: // FIXME: On 2017/03/31 19:52:07, f(malita) wrote: > Nit: Chromium style is // TODO(msarett): ... Done. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:142: // FIXME: On 2017/03/31 19:52:07, f(malita) wrote: > Nit: ditto Done. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:152: DCHECK(xformed); On 2017/03/31 19:52:07, f(malita) wrote: > Nit: return xformed? I still like the DCHECK, but yeah I think so.
Patchset #4 (id:60001) has been deleted
> Respect dstInfo in DecodingImageGenerator::onGetPixels() Instead of "dstInfo", how about "color space"? We already "respect" some pieces of the dstInfo (at least, we fail for requests that we do not support, e.g. size and other color types), but this just adds support for color space conversion, right? https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:146: // |dst| color space. The Skia API for this conversion (non-linear On 2017/03/31 19:59:10, msarett1 wrote: > On 2017/03/31 19:46:08, ccameron wrote: > > On 2017/03/31 19:05:28, msarett1 wrote: > > > I'll see if I can make this public. > > > > > > Regardless, this is a performance problem. Potential fixes: > > > (1) Support kUnpremul decodes in ImageDecoder > > > (2) Pass the dstColorSpace through to ImageDecoder and respect it. > > > (3) Switch to SkCodec. > > > > > > (3) is the least throw-away work because that's the plan anyway. It will > also > > > probably take the longest. SkCodec also helps with the linear premultiply > vs. > > > non-linear premultiply problem because it supports both. > > > > Doing (2) may be possible ... I'm looking to see about the order of creation > of > > things. > > > > I looked at (2) first, then got scared and backed off. I'll probably talk with > Leon (or someone who knows this code better). That may be a better approach. FWIW, kUnpremul in ImageDecoder is already supported (see ImageDecoder::AlphaNotPremultiplied). That means you do not need to unpremultiply first (and then I'm guessing you need to premultiply after conversion since the client wants premultiplied?). I can believe (2) is scary, and it becomes throw-away with (3). This approach seems like the fastest way to implement the correct behavior, though it's probably not as fast as (2). > > > Also, would it be possible to just run an SkColorSpaceXform directly over the > > pixels here? > > Yeah that would be fine (except it doesn't support unpremul then re-premul). > > That's pretty much what SkPixmap::readPixels() will do anyway. https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:101: dstInfo.makeColorType(kN32_SkColorType).makeColorSpace(decodeColorSpace); You can drop the makeColorType piece, since it will always be kN32. (OTOH, I suppose it is needed once you add back in F16 support.) https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:134: bool success = This shadows a variable in the larger scope, and is only used for debugging. Maybe there is a better way to do this? https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:151: return success; I think this can be return true; If success was false, we would have already returned above (before the color xform conversion).
Description was changed from ========== Respect dstInfo in DecodingImageGenerator::onGetPixels() BUG=706613 ========== to ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() BUG=706613 ==========
Patchset #5 (id:100001) has been deleted
So here is the latest model I'm going for... ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. I've tried to optimize the conversion by decoding to kUnpremul if necessary. If there is a risk of using the same decoder over again with different premul behavior, I may need to back out this optimization. https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:146: // |dst| color space. The Skia API for this conversion (non-linear On 2017/04/04 20:52:45, scroggo_chromium wrote: > On 2017/03/31 19:59:10, msarett1 wrote: > > On 2017/03/31 19:46:08, ccameron wrote: > > > On 2017/03/31 19:05:28, msarett1 wrote: > > > > I'll see if I can make this public. > > > > > > > > Regardless, this is a performance problem. Potential fixes: > > > > (1) Support kUnpremul decodes in ImageDecoder > > > > (2) Pass the dstColorSpace through to ImageDecoder and respect it. > > > > (3) Switch to SkCodec. > > > > > > > > (3) is the least throw-away work because that's the plan anyway. It will > > also > > > > probably take the longest. SkCodec also helps with the linear premultiply > > vs. > > > > non-linear premultiply problem because it supports both. > > > > > > Doing (2) may be possible ... I'm looking to see about the order of creation > > of > > > things. > > > > > > > I looked at (2) first, then got scared and backed off. I'll probably talk > with > > Leon (or someone who knows this code better). That may be a better approach. > > FWIW, kUnpremul in ImageDecoder is already supported (see > ImageDecoder::AlphaNotPremultiplied). That means you do not need to > unpremultiply first (and then I'm guessing you need to premultiply after > conversion since the client wants premultiplied?). > > I can believe (2) is scary, and it becomes throw-away with (3). This approach > seems like the fastest way to implement the correct behavior, though it's > probably not as fast as (2). > > > > > > Also, would it be possible to just run an SkColorSpaceXform directly over > the > > > pixels here? > > > > Yeah that would be fine (except it doesn't support unpremul then re-premul). > > > > That's pretty much what SkPixmap::readPixels() will do anyway. > Thanks for the unpremul tip, not sure how I missed that. https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:101: dstInfo.makeColorType(kN32_SkColorType).makeColorSpace(decodeColorSpace); On 2017/04/04 20:52:45, scroggo_chromium wrote: > You can drop the makeColorType piece, since it will always be kN32. (OTOH, I > suppose it is needed once you add back in F16 support.) Thanks, removing. https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:134: bool success = On 2017/04/04 20:52:45, scroggo_chromium wrote: > This shadows a variable in the larger scope, and is only used for debugging. > Maybe there is a better way to do this? Done. https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:151: return success; On 2017/04/04 20:52:45, scroggo_chromium wrote: > I think this can be > > return true; > > If success was false, we would have already returned above (before the color > xform conversion). Done. "return success" gives us "false" on a failed color xform (when DCHECKS are off). I think this is fine though. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:310: DCHECK((*decoder)->frameBufferAtIndex(index)->premultiplyAlpha() == I'm expecting the DCHECK() to warn us if we've already created a decoder with the wrong alpha behavior.
Description was changed from ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() BUG=706613 ========== to ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. I've tried to optimize the conversion by decoding to kUnpremul if convenient. If there is a risk of using the same decoder over again with different premul behavior, I may need to back out this optimization. BUG=706613 ==========
https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:151: return success; On 2017/04/04 23:02:36, msarett1 wrote: > On 2017/04/04 20:52:45, scroggo_chromium wrote: > > I think this can be > > > > return true; > > > > If success was false, we would have already returned above (before the color > > xform conversion). > > Done. "return success" gives us "false" on a failed color xform (when DCHECKS > are off). I think this is fine though. I don't think so. The nested variable success gets set to false on a failed color xform, but the shadowed version will remain unchanged. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: bool needsColorXform = nit: can be const? https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:106: m_frameGenerator->setAlphaOption(ImageDecoder::AlphaNotPremultiplied); I think you can move this inside if (!decodeInfo.isOpaque()). Then you can even combine the two if statements. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:310: DCHECK((*decoder)->frameBufferAtIndex(index)->premultiplyAlpha() == On 2017/04/04 23:02:36, msarett1 wrote: > I'm expecting the DCHECK() to warn us if we've already created a decoder with > the wrong alpha behavior. frameBufferAtIndex may trigger a decode, so you don't want to call it inside DCHECK. (And you don't want to trigger the decode until later, either.) And frameBufferAtIndex might return null, for example in the case where we haven't set the data yet. Instead, perhaps calling setAlphaOption should tell the ImageFrameGenerator to use a different ImageDecoder? Maybe the AlphaOption should be added to the key for the cache of ImageDecoder in ImageDecodingStore::lockDecoder? (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...) Come to think of it, would it make sense to move code you're adding here instead of in DecodingImageGenerator? You already pass an SkImageInfo to the ImageFrameGenerator inside decodeAndScale. It uses the original SkImageInfo instead of the dstInfo, but maybe it could use the dstInfo and ImageFrameGenerator can decide how to handle the conversion?
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Description was changed from ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. I've tried to optimize the conversion by decoding to kUnpremul if convenient. If there is a risk of using the same decoder over again with different premul behavior, I may need to back out this optimization. BUG=706613 ========== to ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. We always use an unpremul decoder when we will be doing a color space xform in a second pass. That way we don't need to unpremultiply. BUG=706613 ==========
https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:151: return success; On 2017/04/05 19:59:01, scroggo_chromium wrote: > On 2017/04/04 23:02:36, msarett1 wrote: > > On 2017/04/04 20:52:45, scroggo_chromium wrote: > > > I think this can be > > > > > > return true; > > > > > > If success was false, we would have already returned above (before the color > > > xform conversion). > > > > Done. "return success" gives us "false" on a failed color xform (when DCHECKS > > are off). I think this is fine though. > > I don't think so. The nested variable success gets set to false on a failed > color xform, but the shadowed version will remain unchanged. Ack. I meant to not redeclare it... I think as it is now is better. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: bool needsColorXform = On 2017/04/05 19:59:01, scroggo_chromium wrote: > nit: can be const? Done. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:106: m_frameGenerator->setAlphaOption(ImageDecoder::AlphaNotPremultiplied); On 2017/04/05 19:59:01, scroggo_chromium wrote: > I think you can move this inside if (!decodeInfo.isOpaque()). > > Then you can even combine the two if statements. Thanks, of course. https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:310: DCHECK((*decoder)->frameBufferAtIndex(index)->premultiplyAlpha() == On 2017/04/05 19:59:01, scroggo_chromium wrote: > On 2017/04/04 23:02:36, msarett1 wrote: > > I'm expecting the DCHECK() to warn us if we've already created a decoder with > > the wrong alpha behavior. > > frameBufferAtIndex may trigger a decode, so you don't want to call it inside > DCHECK. (And you don't want to trigger the decode until later, either.) And > frameBufferAtIndex might return null, for example in the case where we haven't > set the data yet. > > Instead, perhaps calling setAlphaOption should tell the ImageFrameGenerator to > use a different ImageDecoder? Maybe the AlphaOption should be added to the key > for the cache of ImageDecoder in ImageDecodingStore::lockDecoder? > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...) > > Come to think of it, would it make sense to move code you're adding here instead > of in DecodingImageGenerator? You already pass an SkImageInfo to the > ImageFrameGenerator inside decodeAndScale. It uses the original SkImageInfo > instead of the dstInfo, but maybe it could use the dstInfo and > ImageFrameGenerator can decide how to handle the conversion? This all sgtm. Thanks for your thoughts.
After reviewing this patch set, I think I was wrong to suggest moving the transform into the ImageFrameGenerator (sorry!). Some of these changes are good though; I do still think it makes sense to make the AlphaOption part of the cache key. The transform probably belongs in either the ImageDecoder or in the DecodingImageGenerator. The downside of putting it in the DecodingImageGenerator is that for an incrementally decoding image, we'll have to transform the whole image each time we decode. (Moving the transform into the ImageDecoder would probably be a much larger change.) https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: if (dstInfo.width() != getInfo().width() || nit: I think you originally changed "info" to "dstInfo" since you had another SkImageInfo in this method. Now that you don't, I suggest you leave this code as it was. (Either way is fine, in my view, but leaving it unchanged makes git blame more useful, and the change isn't necessary.) https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:88: if (kN32_SkColorType != dstInfo.colorType()) { This change also appears to be unnecessary, I'm guessing. I think getInfo().colorType() will always be kN32? https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: return decoded; One difference between doing the conversion here versus inside the ImageFrameGenerator is that in the prior patch set the conversion did not count towards the time to decode in the trace, but it does in this patch set. Not sure whether it should count. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:274: const SkImageInfo info = SkImageInfo::Make( nit: You could use MakeN32Premul (which has a flavor that takes a color space) and drop the arguments for color and alpha type. That said, it seems odd to me that the ImageFrameGenerator should have a full SkImageInfo. IIUC, it can now create Frames that are premul or unpremul. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:101: typedef std::pair<const ImageFrameGenerator*, std::pair<SkISize, uint32_t>> Why not make this a tuple? (And why do you convert AlphaOption to a uint32_t?) https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:160: m_alphaOption(alphaOption) {} Can you move the call to decoder->alphaOption() here? I'm assuming that decoder should never be null. If it were, there wouldn't be a point to this Entry. Which makes me wonder a little more - why is this constructor public? It looks like it's only called from the public create(); it seems like that method could check for null (if necessary) and this constructor could be private. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:132: DCHECK(m_info.dimensions() == scaledSize); scaledSize looks to be never used again (or it doesn't have to be, see below), so maybe make this DCHECK(m_info.dimensions() == dstInfo.dimensions()) ? I think there's an equality flavor of DCHECK, too: DCHECK_EQ(m_info.dimensions(), dstInfo.dimensions()); https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:148: ASSERT(bitmap.width() == scaledSize.width()); If you drop scaledSize, this could be: DCHECK_EQ(bitmap.dimensions(), dstInfo.dimensions()); OTOH, I've already advised elsewhere that you don't change code you don't have to. Take your pick. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:152: return bitmap.readPixels(bitmap.info(), pixels, rowBytes, 0, 0); Might bitmap.info() be different from dstInfo? I'm surprised this is not dstInfo. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:216: if (needsXform && !dstInfo.isOpaque()) { It might be worth noting that you check dstInfo because it may be more up to date w.r.t. opacity than m_info. On that note, should m_info be updated? (e.g. in setHasAlpha) OTOH, why does the ImageFrameGenerator have an SkImageInfo (more specifically, an SkAlphaType)? The ImageFrameGenerator itself is not natively Premul or Unpremul, and opacity can vary by ImageFrame (as evidenced by setHasAlpha, which has an <index> parameter). https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:363: DCHECK(fullSizeBitmap.width() == m_info.width() && So long as you're changing this, it could be: DCHECK_EQ(fullSizeBitmap.dimensions(), m_info.dimensions()); https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:368: if (needsColorXform(m_info, dstInfo)) { I assume you do not need to do this if fullSizeBitmap.isNull()? (That said, I would expect that we won't get here if isNull() returns true. We return early if the frame is ImageFrame::FrameEmpty. If it's not FrameEmpty, and the bitmap isNull, one of them seems like a lie...) https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:374: SkBitmap tmp(fullSizeBitmap); IIRC, this will make tmp share fullSizeBitmap's SkPixelRef. Is that what you want? It seems like you might as well use fullSizeBitmap directly instead. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:377: for (int y = 0; y < dstInfo.height(); y++) { This method is called while m_decodeMutex is locked, so we're now going to keep it locked longer while we do the color correction. We certainly do not want to let another thread access the SkBitmap (which is owned by the ImageDecoder) while we're modifying it here, but this also means that if two threads attempt to decode e.g. into different color spaces or different frames they cannot do it simultaneously. (Not sure how often that will happen, though.) Since we're (potentially) modifying the SkBitmap owned by the decoder, we could run into an issue where we partially decode an image, convert it to one color space, and then continue decoding from another DecodingImageGenerator which needs a different color space. Another issue here: I see that you convert every line of the bitmap, regardless of what has already been decoded/converted: - is it safe to convert the transparent lines left over from a partial decode? - when resuming a decode, we'll re-transform the lines that were already transformed, which is probably not correct. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:384: dstInfo.isOpaque() ? kOpaque_SkAlphaType : kUnpremul_SkAlphaType; My first thought was that you could use dstInfo.alphaType() here. I guess you cannot because the client may have requested kPremul, and this is the same object the client provided? Instead, what if you at some point before here switched to dstInfo.makeAlphaType(kUnpremul)? Does that make the code simpler? Edit: I guess not, since down below you want to know the original alpha type. NVM. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:398: row = (uint32_t*)(((uint8_t*)row) + fullSizeBitmap.rowBytes()); I think tmp and fullSizeBitmap are essentially the same, but it would seem to be more correct to offset by tmp.rowBytes(). Also, switch to C++ style casts? https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:539: DCHECK(m_colorBehavior.isTag()); Alternatively, you could make this a switch statement, and then no need for the DCHECK. D'oh! Type is not public, so you cannot do that. NVM. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:230: return m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied; I wonder if we should store an AlphaOption instead of a boolean. Seems a little simpler (sometimes we end up converting the bool back to an AlphaOption; OTOH sometimes we do want the bool...).
Patchset #7 (id:200001) has been deleted
> After reviewing this patch set, I think I was wrong to suggest moving the > transform into the ImageFrameGenerator (sorry!). Some of these changes are good > though; I do still think it makes sense to make the AlphaOption part of the > cache key. > I came to the same conclusion after writing Patch Set 6. sgtm. > The transform probably belongs in either the ImageDecoder or in the > DecodingImageGenerator. The downside of putting it in the DecodingImageGenerator > is that for an incrementally decoding image, we'll have to transform the whole > image each time we decode. (Moving the transform into the ImageDecoder would > probably be a much larger change.) I think DecodingImageGenerator makes sense with ImageDecoder in flux somewhat, though you are right that it may be less efficient in some cases. ----------- Patch Set 7 moves the xform back to decoding image generator and keeps the caching changes. I still need to look into adding hash support for std::tuple(). https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: if (dstInfo.width() != getInfo().width() || On 2017/04/07 18:06:06, scroggo_chromium wrote: > nit: I think you originally changed "info" to "dstInfo" since you had another > SkImageInfo in this method. Now that you don't, I suggest you leave this code as > it was. (Either way is fine, in my view, but leaving it unchanged makes git > blame more useful, and the change isn't necessary.) Acknowledged. Moving the xform more logic to this function, so I'll leave as is. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:88: if (kN32_SkColorType != dstInfo.colorType()) { On 2017/04/07 18:06:05, scroggo_chromium wrote: > This change also appears to be unnecessary, I'm guessing. I think > getInfo().colorType() will always be kN32? Yes, but I think this is more clear. Regardless of whether or not all DecodingImageGenerators are created with kN32 as the colorType, this function only succeeds if kN32 is the dstColorType. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: return decoded; On 2017/04/07 18:06:06, scroggo_chromium wrote: > One difference between doing the conversion here versus inside the > ImageFrameGenerator is that in the prior patch set the conversion did not count > towards the time to decode in the trace, but it does in this patch set. Not sure > whether it should count. I think it should not (for now). We are typically seeing the conversion being triggered by code that is drawing SkImage. Right now it seems more logically to be part of the draw. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:274: const SkImageInfo info = SkImageInfo::Make( On 2017/04/07 18:06:06, scroggo_chromium wrote: > nit: You could use MakeN32Premul (which has a flavor that takes a color space) > and drop the arguments for color and alpha type. > > That said, it seems odd to me that the ImageFrameGenerator should have a full > SkImageInfo. IIUC, it can now create Frames that are premul or unpremul. Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:101: typedef std::pair<const ImageFrameGenerator*, std::pair<SkISize, uint32_t>> On 2017/04/07 18:06:06, scroggo_chromium wrote: > Why not make this a tuple? (And why do you convert AlphaOption to a uint32_t?) I'm trying to make use of the existing hash functions in "wtf/HashSet.h". There is no support for tuples - but there probably should be. I'll look at adding that. Also there is no support for AlphaOption. But it makes sense to hash this as if it is an int (rather than add an explicit specialization for AlphaOption). Actually, probably better to use uint8_t. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:160: m_alphaOption(alphaOption) {} On 2017/04/07 18:06:06, scroggo_chromium wrote: > Can you move the call to decoder->alphaOption() here? I'm assuming that decoder > should never be null. If it were, there wouldn't be a point to this Entry. > |decoder| is deleted by std::move(), so we would end up with code that depends on the order of evaluation of the arguments. > Which makes me wonder a little more - why is this constructor public? It looks > like it's only called from the public create(); it seems like that method could > check for null (if necessary) and this constructor could be private. I'll make it private. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:216: if (needsXform && !dstInfo.isOpaque()) { On 2017/04/07 18:06:06, scroggo_chromium wrote: > It might be worth noting that you check dstInfo because it may be more up to > date w.r.t. opacity than m_info. > > On that note, should m_info be updated? (e.g. in setHasAlpha) > > OTOH, why does the ImageFrameGenerator have an SkImageInfo (more specifically, > an SkAlphaType)? The ImageFrameGenerator itself is not natively Premul or > Unpremul, and opacity can vary by ImageFrame (as evidenced by setHasAlpha, which > has an <index> parameter). Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:363: DCHECK(fullSizeBitmap.width() == m_info.width() && On 2017/04/07 18:06:06, scroggo_chromium wrote: > So long as you're changing this, it could be: > > DCHECK_EQ(fullSizeBitmap.dimensions(), m_info.dimensions()); Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:368: if (needsColorXform(m_info, dstInfo)) { On 2017/04/07 18:06:06, scroggo_chromium wrote: > I assume you do not need to do this if fullSizeBitmap.isNull()? > > (That said, I would expect that we won't get here if isNull() returns true. We > return early if the frame is ImageFrame::FrameEmpty. If it's not FrameEmpty, and > the bitmap isNull, one of them seems like a lie...) Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:374: SkBitmap tmp(fullSizeBitmap); On 2017/04/07 18:06:06, scroggo_chromium wrote: > IIRC, this will make tmp share fullSizeBitmap's SkPixelRef. Is that what you > want? It seems like you might as well use fullSizeBitmap directly instead. Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:377: for (int y = 0; y < dstInfo.height(); y++) { On 2017/04/07 18:06:06, scroggo_chromium wrote: > This method is called while m_decodeMutex is locked, so we're now going to keep > it locked longer while we do the color correction. We certainly do not want to > let another thread access the SkBitmap (which is owned by the ImageDecoder) > while we're modifying it here, but this also means that if two threads attempt > to decode e.g. into different color spaces or different frames they cannot do it > simultaneously. (Not sure how often that will happen, though.) > > Since we're (potentially) modifying the SkBitmap owned by the decoder, we could > run into an issue where we partially decode an image, convert it to one color > space, and then continue decoding from another DecodingImageGenerator which > needs a different color space. > > Another issue here: I see that you convert every line of the bitmap, regardless > of what has already been decoded/converted: > - is it safe to convert the transparent lines left over from a partial > decode? > - when resuming a decode, we'll re-transform the lines that were > already transformed, which is probably not correct. Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:384: dstInfo.isOpaque() ? kOpaque_SkAlphaType : kUnpremul_SkAlphaType; On 2017/04/07 18:06:06, scroggo_chromium wrote: > My first thought was that you could use dstInfo.alphaType() here. I guess you > cannot because the client may have requested kPremul, and this is the same > object the client provided? Instead, what if you at some point before here > switched to dstInfo.makeAlphaType(kUnpremul)? Does that make the code simpler? > > Edit: I guess not, since down below you want to know the original alpha type. > NVM. Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:398: row = (uint32_t*)(((uint8_t*)row) + fullSizeBitmap.rowBytes()); On 2017/04/07 18:06:06, scroggo_chromium wrote: > I think tmp and fullSizeBitmap are essentially the same, but it would seem to be > more correct to offset by tmp.rowBytes(). > > Also, switch to C++ style casts? Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:539: DCHECK(m_colorBehavior.isTag()); On 2017/04/07 18:06:06, scroggo_chromium wrote: > Alternatively, you could make this a switch statement, and then no need for the > DCHECK. > > D'oh! Type is not public, so you cannot do that. NVM. Acknowledged. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:230: return m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied; On 2017/04/07 18:06:06, scroggo_chromium wrote: > I wonder if we should store an AlphaOption instead of a boolean. Seems a little > simpler (sometimes we end up converting the bool back to an AlphaOption; OTOH > sometimes we do want the bool...). I think it makes more sense to store the AlphaOption, but I don't want to make that part of this CL.
msarett@chromium.org changed reviewers: + esprehn@chromium.org
Adding hash support for std::tuple(). +esprehn@ for platform/wtf
nit: Please add the ImageDecodingStore and Hash changes to the description. LGTM, except on the hash functions, which I have not reviewed. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:88: if (kN32_SkColorType != dstInfo.colorType()) { On 2017/04/10 14:42:45, msarett1 wrote: > On 2017/04/07 18:06:05, scroggo_chromium wrote: > > This change also appears to be unnecessary, I'm guessing. I think > > getInfo().colorType() will always be kN32? > > Yes, but I think this is more clear. Regardless of whether or not all > DecodingImageGenerators are created with kN32 as the colorType, this function > only succeeds if kN32 is the dstColorType. Fair enough. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:101: typedef std::pair<const ImageFrameGenerator*, std::pair<SkISize, uint32_t>> On 2017/04/10 14:42:45, msarett1 wrote: > On 2017/04/07 18:06:06, scroggo_chromium wrote: > > Why not make this a tuple? (And why do you convert AlphaOption to a uint32_t?) > > I'm trying to make use of the existing hash functions in "wtf/HashSet.h". > > There is no support for tuples - but there probably should be. I'll look at > adding that. > > Also there is no support for AlphaOption. But it makes sense to hash this as if > it is an int (rather than add an explicit specialization for AlphaOption). > Actually, probably better to use uint8_t. sgtm https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:160: m_alphaOption(alphaOption) {} On 2017/04/10 14:42:45, msarett1 wrote: > On 2017/04/07 18:06:06, scroggo_chromium wrote: > > Can you move the call to decoder->alphaOption() here? I'm assuming that > decoder > > should never be null. If it were, there wouldn't be a point to this Entry. > > > > |decoder| is deleted by std::move(), so we would end up with code that depends > on the order of evaluation of the arguments. We already call m_cachedDecoder->decodedSize() - doesn't that also depend on the order of the evaluation of the arguments? I think this ordering is well defined though - it matches the order that the members are declared in. (In fact, you'll get a warning if the order here does not match the order of declaration.) So you can safely change this to m_alphaOption(m_cachedDecoder->alphaOption()) (You would be in trouble if you instead had something like the following: return new DecoderCacheEntry(gen, count, std::move(decoder), decoder->alphaOption()); That ordering is not defined.) > > > Which makes me wonder a little more - why is this constructor public? It looks > > like it's only called from the public create(); it seems like that method > could > > check for null (if necessary) and this constructor could be private. > > I'll make it private. https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2787053004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:230: return m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied; On 2017/04/10 14:42:46, msarett1 wrote: > On 2017/04/07 18:06:06, scroggo_chromium wrote: > > I wonder if we should store an AlphaOption instead of a boolean. Seems a > little > > simpler (sometimes we end up converting the bool back to an AlphaOption; OTOH > > sometimes we do want the bool...). > > I think it makes more sense to store the AlphaOption, but I don't want to make > that part of this CL. sgtm https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: if (dstInfo.width() != getInfo().width() || nit: you can call dimensions() for both, cutting this down to one line. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:99: sk_sp<SkColorSpace> decodeColorSpace = getInfo().refColorSpace(); nit: Why do you call refColorSpace (and store in an sk_sp) instead of colorSpace (and store in a raw pointer)? I think the SkImageGenerator will hang on to it for the lifetime of this method, so there is no need for you to take a ref? I see that makeColorSpace takes an sk_sp, rather than a raw pointer, so if you were to use a raw pointer, you would have to call dstInfo.makeColorSpace(sk_ref_sp(decodeColorSpace)); IIUC, that would save a call to ref/unref (although this probably won't have a big impact on performance?). https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:121: if (needsColorXform) { nit/suggestion: If you changed the above if to if (!decoded || !needsColorXform) { return decoded; } you can save a level of indentation, which might allow you to put more code on one line. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:125: uint32_t* row = (uint32_t*)pixels; C++ style cast. Same down below. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:100: // 3. Decoder alpha option nit: Maybe be explicit that this is AlphaOption? https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:160: std::make_pair(size, (uint8_t)alphaOption)); nit: C++ style cast https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:182: m_alphaOption = m_cachedDecoder->alphaOption(); nit: This could go in the initializer list. (See my new comment in your earlier patch set.)
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
PTAL, still needs OWNERS review for platform/graphics and platform/wtf. FYI, also went through a major rebase. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:83: if (dstInfo.width() != getInfo().width() || On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit: you can call dimensions() for both, cutting this down to one line. Done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:99: sk_sp<SkColorSpace> decodeColorSpace = getInfo().refColorSpace(); On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit: Why do you call refColorSpace (and store in an sk_sp) instead of colorSpace > (and store in a raw pointer)? I think the SkImageGenerator will hang on to it > for the lifetime of this method, so there is no need for you to take a ref? > > I see that makeColorSpace takes an sk_sp, rather than a raw pointer, so if you > were to use a raw pointer, you would have to call > > dstInfo.makeColorSpace(sk_ref_sp(decodeColorSpace)); > > IIUC, that would save a call to ref/unref (although this probably won't have a > big impact on performance?). You're right, done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:121: if (needsColorXform) { On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit/suggestion: If you changed the above if to > > if (!decoded || !needsColorXform) { > return decoded; > } > > you can save a level of indentation, which might allow you to put more code on > one line. Done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:125: uint32_t* row = (uint32_t*)pixels; On 2017/04/10 17:31:24, scroggo_chromium wrote: > C++ style cast. Same down below. Done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:100: // 3. Decoder alpha option On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit: Maybe be explicit that this is AlphaOption? Done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:160: std::make_pair(size, (uint8_t)alphaOption)); On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit: C++ style cast Done. https://codereview.chromium.org/2787053004/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:182: m_alphaOption = m_cachedDecoder->alphaOption(); On 2017/04/10 17:31:24, scroggo_chromium wrote: > nit: This could go in the initializer list. > > (See my new comment in your earlier patch set.) Yes of course, you're right. Thanks.
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msarett@chromium.org changed reviewers: + esprehn@chromium.org, thakis@chromium.org
Oops... actually adding platform/wtf OWNERS this time
lgtm https://codereview.chromium.org/2787053004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2787053004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:229: AlphaOption AlphaOpt() const { I suppose you abbreviated the name so it wouldn't conflict with the enum? The new style guide (https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyxgU/) is not explicit about this case, but there's a suggestion that this should be GetAlphaOption().
https://codereview.chromium.org/2787053004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/2787053004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:229: AlphaOption AlphaOpt() const { On 2017/04/11 12:37:38, scroggo_chromium wrote: > I suppose you abbreviated the name so it wouldn't conflict with the enum? > > The new style guide > (https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyxgU/) > is not explicit about this case, but there's a suggestion that this should be > GetAlphaOption(). sgtm, done.
This (honoring color spaces more) is a web-visible change from what I understand, did y'all go through http://www.chromium.org/blink#new-features for this somewhere (intent to implement email)?
On 2017/04/11 15:18:40, Nico wrote: > This (honoring color spaces more) is a web-visible change from what I > understand, did y'all go through http://www.chromium.org/blink#new-features for > this somewhere (intent to implement email)? This won't change any behavior, except when the --enable-color-correct-rendering flag is on. Right now, that flag is still experimental. ccameron@ may know more about how the "new feature" process might apply when that flag is eventually enabled.
On 2017/04/11 15:22:12, msarett1 wrote: > On 2017/04/11 15:18:40, Nico wrote: > > This (honoring color spaces more) is a web-visible change from what I > > understand, did y'all go through http://www.chromium.org/blink#new-features > for > > this somewhere (intent to implement email)? > > This won't change any behavior, except when the --enable-color-correct-rendering > flag is on. Right now, that flag is still experimental. ccameron@ may know > more about how the "new feature" process might apply when that flag is > eventually enabled. I think the process also applies for changes happening behind flags. You normally send an "intent to implement" very early on, then implement your thing behind a flag (usually behind a single global 'give me new web stuff' flag), and then you "intent to ship" when you're ready to move it out from behind a flag. This is designed so that not a lot of work is spent implementing something that isn't web-friendly, and also to give web devs a chance to chime in before a lot of implementation work has happened. I think in this case it's a pretty simple feature, but it should still go through the process as far as I understand.
On 2017/04/11 15:25:14, Nico wrote: > On 2017/04/11 15:22:12, msarett1 wrote: > > On 2017/04/11 15:18:40, Nico wrote: > > > This (honoring color spaces more) is a web-visible change from what I > > > understand, did y'all go through http://www.chromium.org/blink#new-features > > for > > > this somewhere (intent to implement email)? > > > > This won't change any behavior, except when the > --enable-color-correct-rendering > > flag is on. Right now, that flag is still experimental. ccameron@ may know > > more about how the "new feature" process might apply when that flag is > > eventually enabled. > > I think the process also applies for changes happening behind flags. You > normally send an "intent to implement" very early on, then implement your thing > behind a flag (usually behind a single global 'give me new web stuff' flag), and > then you "intent to ship" when you're ready to move it out from behind a flag. > This is designed so that not a lot of work is spent implementing something that > isn't web-friendly, and also to give web devs a chance to chime in before a lot > of implementation work has happened. I think in this case it's a pretty simple > feature, but it should still go through the process as far as I understand. Thanks. I believe that the process is already underway, but I'll wait for ccameron@ to chime in. FWIW, this is actually a bug fix so that image behavior with --enable-color-correct-rendering matches the current image behavior without the flag. Concerning this particular CL, there's definitely no behavior change.
thakis@chromium.org changed reviewers: + haraken@chromium.org
+haraken so he sees the image cache and the tuple hash things. Code looks generally fine assuming the process things mentioned above get sorted out. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if (kN32_SkColorType != dst_info.colorType()) { no yoda conditionals please https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:125: for (int y = 0; y < dst_info.height(); y++) { This function is getting very long. Consider extracting the loop into a well-named helper. (Guideline: Every chunk in a function should operate on the same level of abstraction, and a loop over pixels feels lower-level than the set-up for it) https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:102: DecoderCacheKey; Do you have an idea how much this will increase the size of this cache in practice? As far as I understand, image caches are a big part of blink's memory use. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/wtf/HashFunctions.h (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/wtf/HashFunctions.h:288: struct TupleHash { If we must have a TupleHash, consider making it a variadic template. But often, a struct with named members is much nicer than using tuples a lot.
esprehn@chromium.org changed reviewers: - esprehn@chromium.org
Added comment WRT this having impact on the web -- this is still behind a flag cause the dst color space is nullptr unless a flag is specified. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:103: decode_color_space && dst_info.colorSpace() && This will have no functional changes because dst_info.colorSpace() is nullptr unless --enable-color-correct-rendering is specified.
https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:103: decode_color_space && dst_info.colorSpace() && On 2017/04/11 19:03:10, ccameron wrote: > This will have no functional changes because dst_info.colorSpace() is nullptr > unless --enable-color-correct-rendering is specified. Pasting this bit from above: """I think the process also applies for changes happening behind flags. You normally send an "intent to implement" very early on, then implement your thing behind a flag (usually behind a single global 'give me new web stuff' flag), and then you "intent to ship" when you're ready to move it out from behind a flag. This is designed so that not a lot of work is spent implementing something that isn't web-friendly, and also to give web devs a chance to chime in before a lot of implementation work has happened. I think in this case it's a pretty simple feature, but it should still go through the process as far as I understand."""
Patchset #12 (id:320001) has been deleted
Patchset #12 (id:340001) has been deleted
Thanks for the review! The latest patch adds a hash implementation for a DecoderCacheKey struct. This removes the changes from wtf/platform. I think this makes more sense. Just to reiterate, this fixes "color correct rendering" of images to behave the same as Chrome currently behaves. No new features to disclose here. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if (kN32_SkColorType != dst_info.colorType()) { On 2017/04/11 15:41:05, Nico wrote: > no yoda conditionals please Done. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:125: for (int y = 0; y < dst_info.height(); y++) { On 2017/04/11 15:41:05, Nico wrote: > This function is getting very long. Consider extracting the loop into a > well-named helper. (Guideline: Every chunk in a function should operate on the > same level of abstraction, and a loop over pixels feels lower-level than the > set-up for it) Done. I think you're right, this works better as a helper. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:102: DecoderCacheKey; On 2017/04/11 15:41:05, Nico wrote: > Do you have an idea how much this will increase the size of this cache in > practice? As far as I understand, image caches are a big part of blink's memory > use. I imagine it's very small. I'm just adding a 8 bytes (max) to the key. https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/wtf/HashFunctions.h (right): https://codereview.chromium.org/2787053004/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/wtf/HashFunctions.h:288: struct TupleHash { On 2017/04/11 15:41:05, Nico wrote: > If we must have a TupleHash, consider making it a variadic template. But often, > a struct with named members is much nicer than using tuples a lot. Seems fine to write a custom hashing implementation for this Key. I've done this. Removed all changes to platform/wtf. https://codereview.chromium.org/2787053004/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h (right): https://codereview.chromium.org/2787053004/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h:70: // Base class for all cache entries. Starting here, code was moved from below.
haraken@chromium.org changed reviewers: + yutak@chromium.org
+yutak I haven't yet caught up the discussion history but would you help me understand why you need to introduce a complex cache data structure in this CL? i.e., why can't we simply use ListHashSet, LinkedHashSet etc?
On 2017/04/12 06:53:03, haraken wrote:
> +yutak
>
> I haven't yet caught up the discussion history but would you help me
understand
> why you need to introduce a complex cache data structure in this CL? i.e., why
> can't we simply use ListHashSet, LinkedHashSet etc?
I need to hash on three things { Ptr, Size, AlphaOption }. Before this change,
the hash implementation used was for std::pair(Ptr, Size).
In Patch Set 11, I extend wtf/HashFunctions.h to include std::tuple().
In Patch Set 12, I create a struct and write my own Hash and HashTraits in
ImageDecodingStore.h.
Either option is fine with me.
The complex cache data structure is not new and I have not modified it. Patch
Set 12 just requires moving that code to be defined before the new Hash structs.
Florin, would you mind taking a look?
On 2017/04/12 12:39:21, msarett1 wrote:
> On 2017/04/12 06:53:03, haraken wrote:
> > +yutak
> >
> > I haven't yet caught up the discussion history but would you help me
> understand
> > why you need to introduce a complex cache data structure in this CL? i.e.,
why
> > can't we simply use ListHashSet, LinkedHashSet etc?
>
> I need to hash on three things { Ptr, Size, AlphaOption }. Before this
change,
> the hash implementation used was for std::pair(Ptr, Size).
>
> In Patch Set 11, I extend wtf/HashFunctions.h to include std::tuple().
> In Patch Set 12, I create a struct and write my own Hash and HashTraits in
> ImageDecodingStore.h.
> Either option is fine with me.
>
> The complex cache data structure is not new and I have not modified it. Patch
> Set 12 just requires moving that code to be defined before the new Hash
structs.
Thanks for the clarification! PS12 LGTM.
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by msarett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2787053004/#ps400001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492104759218400,
"parent_rev": "154fc5ad392d229570cf7332ce8fb686da7d96c0", "commit_rev":
"669daddab5f2f01c57add7ec00ab87fa8453b463"}
Message was sent while issue was closed.
Description was changed from ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. We always use an unpremul decoder when we will be doing a color space xform in a second pass. That way we don't need to unpremultiply. BUG=706613 ========== to ========== Respect colorSpace in DecodingImageGenerator::onGetPixels() ImageDecoder.cpp In case the of a rare image with an embedded color space that is not parametric, we will do a color space xform at decode time. Otherwise, leave it as is. DecodingImageGenerator.cpp Based on cc behavior, we should almost always be asking for the SkImage "as is" (in the same color space that it is already in). In the rare case that we miss the cc cache and request it in a different color space, will need to do the color space xform here. We always use an unpremul decoder when we will be doing a color space xform in a second pass. That way we don't need to unpremultiply. BUG=706613 Review-Url: https://codereview.chromium.org/2787053004 Cr-Commit-Position: refs/heads/master@{#464458} Committed: https://chromium.googlesource.com/chromium/src/+/669daddab5f2f01c57add7ec00ab... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:400001) as https://chromium.googlesource.com/chromium/src/+/669daddab5f2f01c57add7ec00ab...
Message was sent while issue was closed.
Please address the intent-to-implement concern I raised above before further working on this feature.
Message was sent while issue was closed.
On 2017/04/13 20:53:26, Nico wrote: > Please address the intent-to-implement concern I raised above before further > working on this feature. I will certainly make sure it is addressed, thanks! |
