Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801413002:
color: Enable color conversion in image decoder caches (Closed)
|
Created:
3 years, 8 months ago by ccameron Modified:
3 years, 7 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, fmalita+watch_chromium.org, Rik, Stephen Chennney, Justin Novosad, blink-reviews, cc-bugs_chromium.org, ajuma+watch_chromium.org, kinuko+watch, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncolor: Enable color conversion in image decoder caches
In SoftwareImageDecodeCache, specify the color space for readPixels().
In the case of scalePixels, don't specify a color space, but interpret the
colors as being in this space at the end.
Make DecodingImageGenerator::onGetPixels() respect this color space
by performing conversion if needed.
In GpuImageDecodeCache, call makeColorSpace on the decoder-backed
image before uploading pixels. Add TODOs to make this call after upload.
BUG=706613
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2801413002
Cr-Commit-Position: refs/heads/master@{#464581}
Committed: https://chromium.googlesource.com/chromium/src/+/11694bf1f0daf864723ed81304720ba949db4517
Patch Set 1 #
Total comments: 9
Patch Set 2 : Review feedback #Patch Set 3 : Rebase #
Total comments: 5
Patch Set 4 : Dont use readPixels #Patch Set 5 : Rebase #
Total comments: 2
Messages
Total messages: 24 (11 generated)
Description was changed from ========== color: Enable color conversion in image decoder caches BUG= ========== to ========== color: Enable color conversion in image decoder caches BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by ccameron@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...
Description was changed from ========== color: Enable color conversion in image decoder caches BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== color: Enable color conversion in image decoder caches In SoftwareImageDecodeCache, specify the color space for readPixels(). In the case of scalePixels, don't specify a color space, but interpret the colors as being in this space at the end. Make DecodingImageGenerator::onGetPixels() respect this color space by performing conversion if needed. In GpuImageDecodeCache, call makeColorSpace on the decoder-backed image before uploading pixels. Add TODOs to make this call after upload. BUG=706613 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ccameron@chromium.org changed reviewers: + msarett@chromium.org
The software solution here is what I'd like to keep it at (it doesn't do any extra copies of the data). The gpu solution is a bit hacky, but will work for now, as we figure out other details.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
General idea looks good to me. Would prefer to consider this after https://codereview.chromium.org/2787053004/ lands. https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: // Apply color space transformation if requested. Do not fail the decode if Why not? When is it ok to fail? https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:115: case kRGBA_F16_SkColorType: This case feels a little strange here. Supporting 8888->F16 might make more sense because the decoder can currently output 8888. But it may be best to leave this out. https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:118: case kRGB_565_SkColorType: Let's not support 565 in Chrome. This ColorFormat exists as a concession to Android, not because it's a good idea to use 565 for color correct rendering (or any rendering). https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:132: row, info.width(), info.alphaType()); This won't handle alpha correctly. kOpaque will be fine. Passing kPremul here says: "Do a premultiply". This doesn't make sense because: (1) The image is already premultiplied. (2) This will premultiply in a linear space.
Good point -- there's no point in playing diff-pasta -- I've rebased this to be built on https://codereview.chromium.org/2787053004/ and https://skia-review.googlesource.com/c/12103/ instead. https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: // Apply color space transformation if requested. Do not fail the decode if On 2017/04/10 15:27:21, msarett1 wrote: > Why not? When is it ok to fail? This is to match existing behavior -- see for instance this one https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... Or any of the other places we call ColorTransform() in the decoders -- we treat "couldn't parse the color space" the same as "didn't have a color space" and we also treat "couldn't apply the xform" as "nothing bad happened". (that's what's happening in your https://codereview.chromium.org/2787053004/). https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:115: case kRGBA_F16_SkColorType: On 2017/04/10 15:27:21, msarett1 wrote: > This case feels a little strange here. Supporting 8888->F16 might make more > sense because the decoder can currently output 8888. > > But it may be best to leave this out. (rebased on your patch instead) https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:118: case kRGB_565_SkColorType: On 2017/04/10 15:27:21, msarett1 wrote: > Let's not support 565 in Chrome. This ColorFormat exists as a concession to > Android, not because it's a good idea to use 565 for color correct rendering (or > any rendering). (rebased on your patch instead) https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:132: row, info.width(), info.alphaType()); On 2017/04/10 15:27:21, msarett1 wrote: > This won't handle alpha correctly. kOpaque will be fine. > > Passing kPremul here says: "Do a premultiply". This doesn't make sense because: > (1) The image is already premultiplied. > (2) This will premultiply in a linear space. (rebased on your patch instead)
nit: Update commit message. Some general questions about the software implementation... It looks like you never call SkImage::makeColorSpace(). Where is the color conversion happening? Is it in readPixels()? If so, you'll find yourself annoyed by readPixels(). It only supports srgb and linear transfer functions. Parametric transfer functions are possibly a TODO, but there is a lot of complexity because readPixels() supports arbitrary format conversions and premul/unpremuls. Might be better to call makeColorSpace() first and then readPixels() with nullptr as the color space. You still might run into issues with SkImageInfoIsValid()... We'll need to find a way to make it work. https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp (right): https://codereview.chromium.org/2801413002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:102: // Apply color space transformation if requested. Do not fail the decode if On 2017/04/10 20:04:06, ccameron wrote: > On 2017/04/10 15:27:21, msarett1 wrote: > > Why not? When is it ok to fail? > > This is to match existing behavior -- see for instance this one > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image... > > Or any of the other places we call ColorTransform() in the decoders -- we treat > "couldn't parse the color space" the same as "didn't have a color space" and we > also treat "couldn't apply the xform" as "nothing bad happened". > > (that's what's happening in your https://codereview.chromium.org/2787053004/). Acknowledged. https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:165: kPremul_SkAlphaType, color_space); Ignorable nit/question not related to this CL: Is there always alpha? Or can we sometimes mark the image as opaque?
Updated to use readPixels. I also added a TRACE_EVENT for the conversion -- feel free to roll that into your conversion. https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:165: kPremul_SkAlphaType, color_space); On 2017/04/10 21:02:53, msarett1 wrote: > Ignorable nit/question not related to this CL: > > Is there always alpha? Or can we sometimes mark the image as opaque? I'm not sure ... vmpstr may know. I believe that we do have some image metadata and perhaps could mark some images as opaque here. https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:599: bool result = image->readPixels(decoded_info, decoded_pixels->data(), Yes, I was hoping to just call readPixels here... calling image->makeColorSpace->readPixels means making an extra copy of the data long the way, which may be an unacceptable penalty. But ... I'll put the readPixels in for now, and we'll see what happens after that.
I'm still not sure I understand where the conversion is happening for scaled/subrect decodes. Is it going directly to DecodingImageGenerator? If so, looks good to me. Sorry the dependency CL is taking so long. https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:165: kPremul_SkAlphaType, color_space); On 2017/04/10 23:18:30, ccameron wrote: > On 2017/04/10 21:02:53, msarett1 wrote: > > Ignorable nit/question not related to this CL: > > > > Is there always alpha? Or can we sometimes mark the image as opaque? > > I'm not sure ... vmpstr may know. I believe that we do have some image metadata > and perhaps could mark some images as opaque here. Marking as opaque would definitely improve the performance of the color space conversion. https://codereview.chromium.org/2801413002/diff/40001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:599: bool result = image->readPixels(decoded_info, decoded_pixels->data(), On 2017/04/10 23:18:29, ccameron wrote: > Yes, I was hoping to just call readPixels here... calling > image->makeColorSpace->readPixels means making an extra copy of the data long > the way, which may be an unacceptable penalty. > > But ... I'll put the readPixels in for now, and we'll see what happens after > that. Acknowledged.
> I also added a TRACE_EVENT for the conversion -- feel free to roll that into > your conversion. Done.
Rebased off of your patch. On 2017/04/12 12:59:17, msarett1 wrote: > I'm still not sure I understand where the conversion is happening for > scaled/subrect decodes. Is it going directly to DecodingImageGenerator? If so, > looks good to me. We read the full-size image, we color convert that using makeColorSpace, and then we call scalePixels (or readPixels) on the result -- does that sg?
On 2017/04/13 21:25:13, ccameron wrote: > Rebased off of your patch. > > On 2017/04/12 12:59:17, msarett1 wrote: > > I'm still not sure I understand where the conversion is happening for > > scaled/subrect decodes. Is it going directly to DecodingImageGenerator? If > so, > > looks good to me. > > We read the full-size image, we color convert that using makeColorSpace, and > then we call scalePixels (or readPixels) on the result -- does that sg? lgtm
The CQ bit was checked by ccameron@chromium.org
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": 80001, "attempt_start_ts": 1492119004853580,
"parent_rev": "847809ed052d0f4c72098045039fae43be6dee1a", "commit_rev":
"11694bf1f0daf864723ed81304720ba949db4517"}
Message was sent while issue was closed.
Description was changed from ========== color: Enable color conversion in image decoder caches In SoftwareImageDecodeCache, specify the color space for readPixels(). In the case of scalePixels, don't specify a color space, but interpret the colors as being in this space at the end. Make DecodingImageGenerator::onGetPixels() respect this color space by performing conversion if needed. In GpuImageDecodeCache, call makeColorSpace on the decoder-backed image before uploading pixels. Add TODOs to make this call after upload. BUG=706613 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== color: Enable color conversion in image decoder caches In SoftwareImageDecodeCache, specify the color space for readPixels(). In the case of scalePixels, don't specify a color space, but interpret the colors as being in this space at the end. Make DecodingImageGenerator::onGetPixels() respect this color space by performing conversion if needed. In GpuImageDecodeCache, call makeColorSpace on the decoder-backed image before uploading pixels. Add TODOs to make this call after upload. BUG=706613 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2801413002 Cr-Commit-Position: refs/heads/master@{#464581} Committed: https://chromium.googlesource.com/chromium/src/+/11694bf1f0daf864723ed8130472... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/11694bf1f0daf864723ed8130472...
Message was sent while issue was closed.
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2801413002/diff/80001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2801413002/diff/80001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:584: if (target_color_space) { Is this the right spot to do this? From my understanding doing this conversion will essentially decode the image into malloc'ed memory and return a new image for that. In the block below, we'll then call read pixels which will just copy the data into discardable. Is there a way to first read the pixels and then convert them? Or is there a way to specify our own memory in the makeColorSpace function? I'm trying to avoid a full decoded copy here if possible.
Message was sent while issue was closed.
https://codereview.chromium.org/2801413002/diff/80001/cc/tiles/software_image... File cc/tiles/software_image_decode_cache.cc (right): https://codereview.chromium.org/2801413002/diff/80001/cc/tiles/software_image... cc/tiles/software_image_decode_cache.cc:584: if (target_color_space) { On 2017/04/27 22:49:45, vmpstr wrote: > Is this the right spot to do this? > > From my understanding doing this conversion will essentially decode the image > into malloc'ed memory and return a new image for that. In the block below, we'll > then call read pixels which will just copy the data into discardable. > > Is there a way to first read the pixels and then convert them? Or is there a way > to specify our own memory in the makeColorSpace function? I'm trying to avoid a > full decoded copy here if possible. This understanding is correct. So, if (target_color_space) then - the call to makeColorSpace will trigger the decode and color conversion - readPixels will copy that to the buffer If (!target_color_space) then - the call to readPixels will trigger the decode into the buffer It's true, we're doing an extra copy. A more direct scheme would be to have something like "SkImage::readPixelsIntoColorSpace". I'll need to do a power analysis of all of this to see what the problematic areas are. IIRC, the color conversion cost is 2-3x the decode (even when done at the same time as the decode), so this may be more marginal. Of note is that in the case of GPU raster, doing the color conversion this way (uploading the original pixels to the GPU and then doing a conversion into a new buffer on the GPU) proves cheaper. Also of note is that we should be saying "if target_color_space and target_color_space isn't approximately equal to the image color's space". |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
