Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522693002:
Color correct ImageBitmap(HTMLImageElement*) constructor (Closed)
|
Created:
4 years, 1 month ago by zakerinasab Modified:
3 years, 11 months ago CC:
chromium-reviews, blink-reviews, msarett1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement color management for ImageBitamp in Canvas
Implement color management for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md)
ImageBitmap objects are augmented to have an internal color space
attribute of type CanvasColorSpace. The colorSpaceConversion creation
attribute also accepts enum values that correspond to CanvasColorSpace
values. Specifying a CanvasColorSpace value results in a conversion of
the image to the specified color space.
This CL only covers the constructor that uses an HTMLImageElement object.
BUG=665919
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/0f76e01dbfc50d33d6bffb930487ff494babd695
Cr-Commit-Position: refs/heads/master@{#438272}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Resolving issues #
Total comments: 21
Patch Set 3 : Addressing comments and unit tests issues #
Total comments: 2
Patch Set 4 : Removing ImageDecoder::globalTargetColorSpace() calls for now #
Total comments: 9
Patch Set 5 : Addressing comments. #
Total comments: 1
Patch Set 6 : Addressing comments. #
Total comments: 4
Patch Set 7 : Color correction using SkColorSpaceXform #Patch Set 8 : Minor correctinos #
Total comments: 8
Patch Set 9 : Addressing comments. #Patch Set 10 : Putting the new "default" behavior behind a flag #Patch Set 11 : Minor corrections #Patch Set 12 : Swicth moved from cc to content + rebaseline. #Patch Set 13 : Minor corrections. #Patch Set 14 : Rebaseline #
Total comments: 15
Patch Set 15 : Addressing comments. #Patch Set 16 : Addressing crash on memory sanitizer trybot #Messages
Total messages: 101 (53 generated)
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. BUG=665919 ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. BUG=665919 ==========
ccameron@chromium.org changed reviewers: + ccameron@chromium.org
https://codereview.chromium.org/2522693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:270: !colorCorrectRendering) { Ah -- as soon as https://codereview.chromium.org/2512683003/ lands You'll be able to query this directly using ImageDecoder::globalTargetColorSpace()
zakerinasab@chromium.org changed reviewers: + dcheng@chromium.org, junov@chromium.org, xidachen@chromium.org
https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:247: return image; This part should be moved to the function parseOptions(). https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:259: return image; This part should be moved to parseOptions() as well. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:386: croppedSkImage, parsedOptions.premultiplyAlpha, imageBitmapOptions); Pass |parsedOptions| instead of the last two arguments. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:440: skiaImage, parsedOptions.premultiplyAlpha, imageBitmapOptions); Pass |parsedOptions| instead of the last two arguments. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:466: ImageDecoder::ColorSpaceApplied, &options); This is not the best way, and it is caused by my original implementation, sigh. Please change the implementation to add a new item in the struct ParsedOptions, maybe called "dstColorspace". It could be an enum, to represent the destination color space. Then in the function parseOptions(), you parse the input options, and assign the right value to dstColorspace. By doing the above, you don't need this additional |options| argument, you can also get rid of the second last argument here which is the |ImageDecoder::ColorSpaceApplied| because that should be determined by the dstColorspace. Does that make sense? https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:251: ColorSpaceConversion csConversion = (ColorSpaceConversion)(i); Please avoid using abbreviation, use colorSpaceConversion instead of csConversion.
Drive-by https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:196: static const std::vector<std::string> conversions = { Please use Vector and String in Blink (which will also simplify the conversion code that is present below) https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:199: WTF::String(conversions[(uint8_t)(colorSpaceConversion)].c_str())); Here and elsewhere, casts should be using C++-style casts (static_cast, etc) rather than C-style casts. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:281: uint8_t* convertedPixel = new uint8_t[imageInfo.bytesPerPixel()]; It's not clear if this leaks. I would recommend writing this as: auto convertedPixels = makeUnique<uint8_t[]>(imageInfo.bytesPerPixel());
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
New patch uploaded. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:259: return image; On 2016/11/22 20:34:37, xidachen wrote: > This part should be moved to parseOptions() as well. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:386: croppedSkImage, parsedOptions.premultiplyAlpha, imageBitmapOptions); On 2016/11/22 20:34:37, xidachen wrote: > Pass |parsedOptions| instead of the last two arguments. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:440: skiaImage, parsedOptions.premultiplyAlpha, imageBitmapOptions); On 2016/11/22 20:34:38, xidachen wrote: > Pass |parsedOptions| instead of the last two arguments. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:466: ImageDecoder::ColorSpaceApplied, &options); On 2016/11/22 20:34:37, xidachen wrote: > This is not the best way, and it is caused by my original implementation, sigh. > > Please change the implementation to add a new item in the struct ParsedOptions, > maybe called "dstColorspace". It could be an enum, to represent the destination > color space. > > Then in the function parseOptions(), you parse the input options, and assign the > right value to dstColorspace. > > By doing the above, you don't need this additional |options| argument, you can > also get rid of the second last argument here which is the > |ImageDecoder::ColorSpaceApplied| because that should be determined by the > dstColorspace. Does that make sense? For now we need to keep ImageDecoder::ColorSpaceOption parameter as other parts of the code expect the default value to be true. We will remove this after we added the color conversion support to all the ImageBitmap constructors. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:196: static const std::vector<std::string> conversions = { On 2016/11/23 02:23:54, dcheng wrote: > Please use Vector and String in Blink (which will also simplify the conversion > code that is present below) Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:199: WTF::String(conversions[(uint8_t)(colorSpaceConversion)].c_str())); On 2016/11/23 02:23:54, dcheng wrote: > Here and elsewhere, casts should be using C++-style casts (static_cast, etc) > rather than C-style casts. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:251: ColorSpaceConversion csConversion = (ColorSpaceConversion)(i); On 2016/11/22 20:34:38, xidachen wrote: > Please avoid using abbreviation, use colorSpaceConversion instead of > csConversion. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:281: uint8_t* convertedPixel = new uint8_t[imageInfo.bytesPerPixel()]; On 2016/11/23 02:23:54, dcheng wrote: > It's not clear if this leaks. I would recommend writing this as: > > auto convertedPixels = makeUnique<uint8_t[]>(imageInfo.bytesPerPixel()); Done.
Great work! Almost there. https://codereview.chromium.org/2522693002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:437: : ImageDecoder::ColorSpaceIgnored); Why not move the last argument to cropImage() function? So right before ImageDecoder::create() is called, you do: colorspaceOp = parsedOptions.dstColorSpace ? ImageDecoder::ColorSpaceApplied : ImageDecoder::ColorSpaceIgnored; Then you do ImageDecoder::create() with colorspaceOp. In this case, you can delete the last argument.
New patch submitted. https://codereview.chromium.org/2522693002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:437: : ImageDecoder::ColorSpaceIgnored); On 2016/11/24 18:41:01, xidachen wrote: > Why not move the last argument to cropImage() function? > > So right before ImageDecoder::create() is called, you do: > colorspaceOp = parsedOptions.dstColorSpace ? ImageDecoder::ColorSpaceApplied : > ImageDecoder::ColorSpaceIgnored; > > Then you do ImageDecoder::create() with colorspaceOp. In this case, you can > delete the last argument. We can't do it now as other constructors rely on the default value of ImageDecoder::ColorSpaceApplied. I will remove it after fixing the remained constructors. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:69: //parsedOptions.dstColorSpace = ImageDecoder::globalTargetColorSpace(); @ccameron Can you take a look at this? When I do this call the browser crashes on Mac on all image bitmap unit tests. Please see the Mac try-bot results for patch set 3.
https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:69: //parsedOptions.dstColorSpace = ImageDecoder::globalTargetColorSpace(); On 2016/11/24 20:13:42, zakerinasab wrote: > @ccameron Can you take a look at this? When I do this call the browser crashes > on Mac on all image bitmap unit tests. Please see the Mac try-bot results for > patch set 3. I am fine with setting this to SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named); + a comment or TODO saying that it should be changed to use ImageDecoder::globalTargetColorSpace() once the crash is fixed. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:196: "srgb", "linear-rgb"}; Why are we having two "default"? https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:225: uint8_t* srcPixel = new uint8_t[rasterImageInfo.bytesPerPixel()]; Please use makeUnique. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:292: uint8_t* transformedPixel = new uint8_t[imageInfo.bytesPerPixel()]; Please use makeUnique.
https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:199: WTF::String(conversions[(uint8_t)(colorSpaceConversion)].c_str())); On 2016/11/24 18:18:33, zakerinasab wrote: > On 2016/11/23 02:23:54, dcheng wrote: > > Here and elsewhere, casts should be using C++-style casts (static_cast, etc) > > rather than C-style casts. > > Done. Looks like there's still casts like (void*), etc. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:281: uint8_t* convertedPixel = new uint8_t[imageInfo.bytesPerPixel()]; On 2016/11/24 18:18:33, zakerinasab wrote: > On 2016/11/23 02:23:54, dcheng wrote: > > It's not clear if this leaks. I would recommend writing this as: > > > > auto convertedPixels = makeUnique<uint8_t[]>(imageInfo.bytesPerPixel()); > > Done. There's a few more instances of this throughout that need to be fixed as well.
New patch submitted (addressing comments). https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:199: WTF::String(conversions[(uint8_t)(colorSpaceConversion)].c_str())); On 2016/11/24 20:28:15, dcheng wrote: > On 2016/11/24 18:18:33, zakerinasab wrote: > > On 2016/11/23 02:23:54, dcheng wrote: > > > Here and elsewhere, casts should be using C++-style casts (static_cast, etc) > > > rather than C-style casts. > > > > Done. > > Looks like there's still casts like (void*), etc. Done. https://codereview.chromium.org/2522693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:281: uint8_t* convertedPixel = new uint8_t[imageInfo.bytesPerPixel()]; On 2016/11/24 20:28:15, dcheng wrote: > On 2016/11/24 18:18:33, zakerinasab wrote: > > On 2016/11/23 02:23:54, dcheng wrote: > > > It's not clear if this leaks. I would recommend writing this as: > > > > > > auto convertedPixels = makeUnique<uint8_t[]>(imageInfo.bytesPerPixel()); > > > > Done. > > There's a few more instances of this throughout that need to be fixed as well. Done. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:69: //parsedOptions.dstColorSpace = ImageDecoder::globalTargetColorSpace(); On 2016/11/24 20:27:31, xidachen wrote: > On 2016/11/24 20:13:42, zakerinasab wrote: > > @ccameron Can you take a look at this? When I do this call the browser crashes > > on Mac on all image bitmap unit tests. Please see the Mac try-bot results for > > patch set 3. > > I am fine with setting this to > SkColorSpace::MakeNamed(SkColorSpace::kSRGB_Named); + a comment or TODO saying > that it should be changed to use ImageDecoder::globalTargetColorSpace() once the > crash is fixed. Done. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:196: "srgb", "linear-rgb"}; On 2016/11/24 20:27:31, xidachen wrote: > Why are we having two "default"? We have two "default"s because there are two default modes with different behavior (depending on the --enable-experimental-canvas-features and --color-correct-rendering flags). https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:225: uint8_t* srcPixel = new uint8_t[rasterImageInfo.bytesPerPixel()]; On 2016/11/24 20:27:31, xidachen wrote: > Please use makeUnique. Done. https://codereview.chromium.org/2522693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:292: uint8_t* transformedPixel = new uint8_t[imageInfo.bytesPerPixel()]; On 2016/11/24 20:27:31, xidachen wrote: > Please use makeUnique. Done.
lgtm with nits. https://codereview.chromium.org/2522693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:71: // is fixed. Please file a bug indicating that there is a crash in ImageDecoder::globalTargetColorSpace(), and pointing out the crbug number here.
New patch submitted (addressing comments).
Patchset #6 (id:140001) has been deleted
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. BUG=665919 ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 ==========
https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:82: parsedOptions.bytesPerPixel = 8; avoid hard-coded values. IMHO, we should remove 'bytesPerPixel' completely since this information is redundant with dstColorSpace. Just call SkColorTypeBytesPerPixel() to get the byte per pixel for a given color type. https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:273: surface->getCanvas()->drawImage(image, 0, 0); This works, but it would be even better to have a path that can do color conversions in-place, especially for cases that require no color type conversion. It is possible that Skia has no API for this right now though (not sure). Please create a new crbug for this and add a TODO comment right here in the code, associated with the bug.
New patch submitted. https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:82: parsedOptions.bytesPerPixel = 8; On 2016/11/28 16:30:43, Justin Novosad wrote: > avoid hard-coded values. IMHO, we should remove 'bytesPerPixel' completely > since this information is redundant with dstColorSpace. Just call > SkColorTypeBytesPerPixel() to get the byte per pixel for a given color type. Done. https://codereview.chromium.org/2522693002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:273: surface->getCanvas()->drawImage(image, 0, 0); On 2016/11/28 16:30:42, Justin Novosad wrote: > This works, but it would be even better to have a path that can do color > conversions in-place, especially for cases that require no color type > conversion. It is possible that Skia has no API for this right now though (not > sure). Please create a new crbug for this and add a TODO comment right here in > the code, associated with the bug. Done.
lgtm
On 2016/11/28 21:16:15, Justin Novosad wrote: > lgtm lgtm too -- go ahead and land this, and I'll resolve my patch https://codereview.chromium.org/2523943002/ against it. Hopefully when that lands, we'll be able to use the global target color space.
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps180001 (title: "Addressing comments")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
New patch submitted with in-place color conversion. @msarett: PTAL at the SkColorSpaceXform usage and let me know if anything can be improved.
Patchset #9 (id:220001) has been deleted
msarett@chromium.org changed reviewers: + msarett@chromium.org
https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:37: SkColorType dstColorType = SkColorType::kN32_SkColorType; FWIW, I don't think the SkColorType:: is necessary. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:293: case kIndex_8_SkColorType: I'm guessing that this case isn't necessary for now - don't think Chrome uses kIndex8? https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310: static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image, Unless I'm missing something, I think that this function could just be a couple lines if not for a Skia bug... readPixels(SkImageInfo ...) should respect whatever info is passed in. Ex: if the info has a color space, we should transform to that color space. This currently doesn't work. FWIW, if the the info has a nullptr color space or the src image has a nullptr color space, we should not do a color space transformation (I at least think this probably works correctly now). So I think there should be no need to transform after calling readPixels()... and I'm not sure I completely understand the case where we need to draw the image to a canvas. I'm fine with this as a workaround. But I know there is a bit of ongoing work towards fixing readPixels()... and I'll go ahead and help get this working properly. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:362: // Skia does not supports color conversion for unpremultiplied images. Not sure what this comment means... I would say that: "Skia does not support drawing to unpremul surfaces/canvases." But I do think the we do (or should) support drawing unpremultiplied images.
Patchset #10 (id:260001) has been deleted
New patch submitted. Comments are applied. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:37: SkColorType dstColorType = SkColorType::kN32_SkColorType; On 2016/11/30 21:32:39, msarett1 wrote: > FWIW, I don't think the SkColorType:: is necessary. Removed. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:293: case kIndex_8_SkColorType: On 2016/11/30 21:32:39, msarett1 wrote: > I'm guessing that this case isn't necessary for now - don't think Chrome uses > kIndex8? Removed. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310: static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image, On 2016/11/30 21:32:39, msarett1 wrote: > Unless I'm missing something, I think that this function could just be a couple > lines if not for a Skia bug... > > readPixels(SkImageInfo ...) should respect whatever info is passed in. Ex: if > the info has a color space, we should transform to that color space. This > currently doesn't work. > > FWIW, if the the info has a nullptr color space or the src image has a nullptr > color space, we should not do a color space transformation (I at least think > this probably works correctly now). > > So I think there should be no need to transform after calling readPixels()... > and I'm not sure I completely understand the case where we need to draw the > image to a canvas. > > I'm fine with this as a workaround. But I know there is a bit of ongoing work > towards fixing readPixels()... and I'll go ahead and help get this working > properly. Proper TODO added. https://codereview.chromium.org/2522693002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:362: // Skia does not supports color conversion for unpremultiplied images. On 2016/11/30 21:32:39, msarett1 wrote: > Not sure what this comment means... > > I would say that: "Skia does not support drawing to unpremul surfaces/canvases." > > But I do think the we do (or should) support drawing unpremultiplied images. Done.
new changes lgtm
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, ccameron@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps280001 (title: "Addressing comments.")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
zakerinasab@chromium.org changed reviewers: + piman@chromium.org
New patch submitted. The new behavior of "default" color space conversion option is behind a flag now to avoid changing the current behavior.
Patchset #11 (id:300001) has been deleted
Patchset #11 (id:320001) has been deleted
Patchset #12 (id:360001) has been deleted
zakerinasab@chromium.org changed reviewers: + enne@chromium.org - piman@chromium.org
zakerinasab@chromium.org changed reviewers: + danakj@chromium.org - enne@chromium.org
On 2016/12/05 16:43:08, zakerinasab wrote: > New patch submitted. The new behavior of "default" color space conversion option > is behind a flag now to avoid changing the current behavior. new changes lgtm
zakerinasab@chromium.org changed reviewers: + vmpstr@chromium.org
Why is this a cc switch? It's not changing compositor behaviour afaict?
Patchset #7 (id:180001) has been deleted
On 2016/12/08 17:38:03, danakj_OOO_and_slow wrote: > Why is this a cc switch? It's not changing compositor behaviour afaict? Right. Moved to content.
That seems better but I'm no longer an owner then :)
zakerinasab@chromium.org changed reviewers: + piman@chromium.org - danakj@chromium.org, vmpstr@chromium.org
On 2016/12/08 21:18:29, danakj_OOO_and_slow wrote: > That seems better but I'm no longer an owner then :) @piman, PTAL at the flag. Thanks.
zakerinasab@chromium.org changed reviewers: - xidachen@chromium.org
lgtm
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xidachen@chromium.org, ccameron@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps420001 (title: "Minor corrections.")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zakerinasab@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zakerinasab@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org, junov@chromium.org, piman@chromium.org, xidachen@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps440001 (title: "Rebaseline")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
zakerinasab@chromium.org changed reviewers: - piman@chromium.org
https://codereview.chromium.org/2522693002/diff/440001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2522693002/diff/440001/content/public/common/... content/public/common/web_preferences.h:174: bool color_correct_rendering_default_mode_enabled = false; I think the IPC serialization [1] for this need to be updated. Otherwise, the renderer is not going to get any value other than false for this. [1] https://cs.chromium.org/chromium/src/content/public/common/common_param_trait... https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:145: bool dstBufferSizeHasOverflow(ParsedOptions options) { Nit: not introduced by this CL, but this should be const ParsedOptions& https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:227: bool enforceAlphaPremultiply = false, Ditto about preferring enums over bools here as well: at the callsites, it's not obvious what behavior the bool controls. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:296: SkASSERT(false); I think we should be writing NOTREACHED in Blink code. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310: static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image, This refs the SkImage unnecessarily, as this method does not take ownership of |image|. Instead, pass the image by SkImage* (similar to unPremulSkImageToPremul, etc) (The general guideline in Chromium and Blink is that if ownership is transferred or taken, pass by smart pointer. Otherwise, pass by raw pointer.) https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:398: decodedColorSpace = sk_sp<SkColorSpace>(frame->bitmap().colorSpace()); This parameter is passed by value, so the caller will never see this. Since this is an optional out param, we can combine the bool and the output param and just have the param be a sk_sp<SkColorpace>* decodedColorSpace. Then we can DCHECK((decodedColorType && decodedColorSpace) || (!decodedCOlorType && decodedColorSpace)); to make sure the arguments always agree. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:80: bool = false); Please don't write unnamed bool parameters, it's not obvious what the bool parameter controls. In general, Blink style prefers to use enums rather than bools, so I would recommend writing an enum here.
Patchset #15 (id:460001) has been deleted
zakerinasab@google.com changed reviewers: + zakerinasab@google.com
New patch submitted to address the new comments. https://codereview.chromium.org/2522693002/diff/440001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2522693002/diff/440001/content/public/common/... content/public/common/web_preferences.h:174: bool color_correct_rendering_default_mode_enabled = false; On 2016/12/11 20:11:58, dcheng wrote: > I think the IPC serialization [1] for this need to be updated. Otherwise, the > renderer is not going to get any value other than false for this. > > [1] > https://cs.chromium.org/chromium/src/content/public/common/common_param_trait... Done. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:145: bool dstBufferSizeHasOverflow(ParsedOptions options) { On 2016/12/11 20:11:58, dcheng wrote: > Nit: not introduced by this CL, but this should be const ParsedOptions& Done. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:227: bool enforceAlphaPremultiply = false, On 2016/12/11 20:11:58, dcheng wrote: > Ditto about preferring enums over bools here as well: at the callsites, it's not > obvious what behavior the bool controls. Done. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:296: SkASSERT(false); On 2016/12/11 20:11:58, dcheng wrote: > I think we should be writing NOTREACHED in Blink code. Done. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310: static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image, On 2016/12/11 20:11:58, dcheng wrote: > This refs the SkImage unnecessarily, as this method does not take ownership of > |image|. Instead, pass the image by SkImage* (similar to > unPremulSkImageToPremul, etc) > > (The general guideline in Chromium and Blink is that if ownership is transferred > or taken, pass by smart pointer. Otherwise, pass by raw pointer.) I changed the function signature. PTAL. Passing SkImage* and returning sk_sp<SkImage> results in SkAssert(SkRefCnt > 0) errors. I suspect this might have something to do with in-place color conversion that we do, i.e., we use the same memory buffer of the input SkImage to write the new pixel values for the color corrected SkImage. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:398: decodedColorSpace = sk_sp<SkColorSpace>(frame->bitmap().colorSpace()); On 2016/12/11 20:11:58, dcheng wrote: > This parameter is passed by value, so the caller will never see this. > > Since this is an optional out param, we can combine the bool and the output > param and just have the param be a sk_sp<SkColorpace>* decodedColorSpace. > > Then we can DCHECK((decodedColorType && decodedColorSpace) || (!decodedCOlorType > && decodedColorSpace)); to make sure the arguments always agree. The parameter is fixed. The flag is needed as the input SkColorSpace might be null and we still need to fill it with ptoper color space information. https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.h (right): https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.h:80: bool = false); On 2016/12/11 20:11:58, dcheng wrote: > Please don't write unnamed bool parameters, it's not obvious what the bool > parameter controls. In general, Blink style prefers to use enums rather than > bools, so I would recommend writing an enum here. Done
lgtm https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2522693002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:310: static sk_sp<SkImage> applyColorSpaceConversion(const sk_sp<SkImage> image, On 2016/12/12 18:35:21, zakerinasab1 wrote: > On 2016/12/11 20:11:58, dcheng wrote: > > This refs the SkImage unnecessarily, as this method does not take ownership of > > |image|. Instead, pass the image by SkImage* (similar to > > unPremulSkImageToPremul, etc) > > > > (The general guideline in Chromium and Blink is that if ownership is > transferred > > or taken, pass by smart pointer. Otherwise, pass by raw pointer.) > > I changed the function signature. PTAL. > Passing SkImage* and returning sk_sp<SkImage> results in SkAssert(SkRefCnt > 0) > errors. I suspect this might have something to do with in-place color conversion > that we do, i.e., we use the same memory buffer of the input SkImage to write > the new pixel values for the color corrected SkImage. Ah... another option is to pass by value (instead of const value); then you can std::move() and be slightly more efficient. This is fine though.
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org, junov@chromium.org, piman@chromium.org, xidachen@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps480001 (title: "Addressing comments.")
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zakerinasab@chromium.org
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
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_...)
Patchset #16 (id:500001) has been deleted
The CQ bit was checked by zakerinasab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ccameron@chromium.org, junov@chromium.org, piman@chromium.org, dcheng@chromium.org, xidachen@chromium.org Link to the patchset: https://codereview.chromium.org/2522693002/#ps520001 (title: "Addressing crash on memory sanitizer trybot")
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": 520001, "attempt_start_ts": 1481653007708390,
"parent_rev": "b09db807d0bd9dbc075bae430a1c5a825536dda2", "commit_rev":
"b9ba84ea6b91852214404113d1d2036bb2fef263"}
Message was sent while issue was closed.
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2522693002 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2522693002 ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/0f76e01dbfc50d33d6bffb930487ff494babd695 Cr-Commit-Position: refs/heads/master@{#438272} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/0f76e01dbfc50d33d6bffb930487ff494babd695 Cr-Commit-Position: refs/heads/master@{#438272}
Message was sent while issue was closed.
Description was changed from ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/0f76e01dbfc50d33d6bffb930487ff494babd695 Cr-Commit-Position: refs/heads/master@{#438272} ========== to ========== Implement color management for ImageBitamp in Canvas Implement color management for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) ImageBitmap objects are augmented to have an internal color space attribute of type CanvasColorSpace. The colorSpaceConversion creation attribute also accepts enum values that correspond to CanvasColorSpace values. Specifying a CanvasColorSpace value results in a conversion of the image to the specified color space. This CL only covers the constructor that uses an HTMLImageElement object. BUG=665919 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/0f76e01dbfc50d33d6bffb930487ff494babd695 Cr-Commit-Position: refs/heads/master@{#438272} ========== |
