Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575693005:
Add unit test for color managed ImageBitamp(ImageBitmap) constructor (Closed)
|
Created:
4 years ago by zakerinasab Modified:
4 years ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit test for color managed ImageBitamp(ImageBitmap) constructor
This CL is the follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md)
BUG=665919
Committed: https://crrev.com/0165593b23888e943820fe8e96bb6001897d159b
Cr-Commit-Position: refs/heads/master@{#439706}
Patch Set 1 #Patch Set 2 : Moving ImageBitmap(HTMLCanvasElement) test to CanvasRenderingContext2DTest #
Total comments: 10
Patch Set 3 : Rebaseline #Patch Set 4 : Breaking the CL into multiple ones - ImageBitmap(ImageBitmap) unit test #
Total comments: 3
Patch Set 5 : Addressing comments #
Total comments: 1
Messages
Total messages: 25 (12 generated)
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 is the follow up for CL 2522693002. It covers the remaining ImageBitmap constructors along with the respective unit tests. This CL still partially addresses bug 665919. 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 is the follow up for CL 2522693002. It covers the remaining ImageBitmap constructors along with the respective unit tests. This CL still partially addresses bug 665919. BUG=665919 ==========
zakerinasab@google.com changed reviewers: + junov@chromium.org
zakerinasab@google.com changed reviewers: + dcheng@chromium.org
zakerinasab@google.com changed reviewers: + zakerinasab@google.com
Patch submitted. PTAL. Thanks.
This is a bit confusing, the tests that are added/changed do not all correspond to the code paths that are modified. Could you break this up into smaller CLs. I.e. : In each CL, fix one code path + test it Tests for code paths that have already been fixed can go in a standalone CL. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:734: // ImageData supports color spaces. crbug.com/670715. It seems this constructor is now in a half fixed state: the code paths that call scaleSkImage will have their colors converted. Is the right? Could you document this in the comment? https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:1014: // TODO(zakerinasab): Bitmap data must be returned in the current color space Note: This will require making StaticBitmapImage and its subclasses to not ignore the ColorBehavior argument of imageForCurrentFrame. You could use the caching behavior that ccameron put in BitmapImage::imageForCurrentFrame, perhaps move it up into Image so that other subclasses of Image can use it. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:232: RuntimeEnabledFeatures::setColorCorrectRenderingDefaultModeEnabled(!flag); Need to reset flags when done. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:288: ->imageForCurrentFrame(ColorBehavior::ignore()) Add a comment to explain why the "ignore" is important. Same for other similar tests. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:1200: RuntimeEnabledFeatures::setColorCorrectRenderingDefaultModeEnabled(!flag); RuntimeEnabledFeatures is a global state object. When the test ends, you need to reset the values of these flags to their original values otherwise this test could have side-effects on other tests.
CL reduced to a unit test for ImageBitmap(ImageBitmap) constructor with all the functions used by others. When this is landed, I will upload the other CLs gradually. PTAL. Thanks. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:734: // ImageData supports color spaces. crbug.com/670715. On 2016/12/14 20:53:46, Justin Novosad wrote: > It seems this constructor is now in a half fixed state: the code paths that call > scaleSkImage will have their colors converted. Is the right? Could you document > this in the comment? No, but it can be if we pass the color space parameter. I left it null to not leave the constructor half fixed. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmap.cpp:1014: // TODO(zakerinasab): Bitmap data must be returned in the current color space On 2016/12/14 20:53:46, Justin Novosad wrote: > Note: This will require making StaticBitmapImage and its subclasses to not > ignore the ColorBehavior argument of imageForCurrentFrame. You could use the > caching behavior that ccameron put in BitmapImage::imageForCurrentFrame, perhaps > move it up into Image so that other subclasses of Image can use it. I added your comments to the TODO to be considered when implementing. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:232: RuntimeEnabledFeatures::setColorCorrectRenderingDefaultModeEnabled(!flag); On 2016/12/14 20:53:46, Justin Novosad wrote: > Need to reset flags when done. Fixed to store and reset the state of the flags at the beginning and end of each test. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:288: ->imageForCurrentFrame(ColorBehavior::ignore()) On 2016/12/14 20:53:46, Justin Novosad wrote: > Add a comment to explain why the "ignore" is important. Same for other similar > tests. Done. https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp (right): https://codereview.chromium.org/2575693005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:1200: RuntimeEnabledFeatures::setColorCorrectRenderingDefaultModeEnabled(!flag); On 2016/12/14 20:53:46, Justin Novosad wrote: > RuntimeEnabledFeatures is a global state object. When the test ends, you need to > reset the values of these flags to their original values otherwise this test > could have side-effects on other tests. Done.
lgtm. Please edit the title and description before committing
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 is the follow up for CL 2522693002. It covers the remaining ImageBitmap constructors along with the respective unit tests. This CL still partially addresses bug 665919. BUG=665919 ========== to ========== Add unit test for color managed ImageBitamp(ImageBitmap) constructor This CL is the follow up for CL 2522693002, part of color management implementation for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) BUG=665919 ==========
zakerinasab@google.com changed reviewers: - zakerinasab@google.com
junov@ already reviewed, so I skipped reviewing most of the actual logic, but I do one have one question about the runtime flags management. https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:267: ReadRuntimeFlags(flagsStatus); Shouldn't the helpers in SetUp/TearDown already manage this?
zakerinasab@google.com changed reviewers: + zakerinasab@google.com
https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:267: ReadRuntimeFlags(flagsStatus); On 2016/12/19 18:36:43, dcheng wrote: > Shouldn't the helpers in SetUp/TearDown already manage this? As I know it is possible for multiple tests to end up in the same process. Now that you asked this I'm wondering if it does mean that we have a separate setup and tear down for each test or we might have setup and tear down run once for multiple tests inside a test suite. If it is the former case, and they run once per test, then this extra check is not needed here.
https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:267: ReadRuntimeFlags(flagsStatus); On 2016/12/19 19:41:59, zakerinasab1 wrote: > On 2016/12/19 18:36:43, dcheng wrote: > > Shouldn't the helpers in SetUp/TearDown already manage this? > > As I know it is possible for multiple tests to end up in the same process. Now > that you asked this I'm wondering if it does mean that we have a separate setup > and tear down for each test or we might have setup and tear down run once for > multiple tests inside a test suite. If it is the former case, and they run once > per test, then this extra check is not needed here. SetUp() and TearDown() are called for each test: https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#te... So we can just rely on that to save/restore the flags.
On 2016/12/19 19:47:17, dcheng wrote: > https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/2575693005/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:267: > ReadRuntimeFlags(flagsStatus); > On 2016/12/19 19:41:59, zakerinasab1 wrote: > > On 2016/12/19 18:36:43, dcheng wrote: > > > Shouldn't the helpers in SetUp/TearDown already manage this? > > > > As I know it is possible for multiple tests to end up in the same process. Now > > that you asked this I'm wondering if it does mean that we have a separate > setup > > and tear down for each test or we might have setup and tear down run once for > > multiple tests inside a test suite. If it is the former case, and they run > once > > per test, then this extra check is not needed here. > > SetUp() and TearDown() are called for each test: > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#te... > > So we can just rely on that to save/restore the flags. Fixed.
New patch submitted addressing dcheng@ comments. PTAL.
lgtm https://codereview.chromium.org/2575693005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/2575693005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp:72: // this test suite. Nit: probably revert the latter half of the comment, since individual tests don't do this now.
The CQ bit was checked by zakerinasab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2575693005/#ps80001 (title: "Addressing comments")
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": 1482203590132250,
"parent_rev": "9605d463e66ca170b8337075304490dfbecce513", "commit_rev":
"69c10b332547d827ad1280111e143c3cab5749df"}
Message was sent while issue was closed.
Description was changed from ========== Add unit test for color managed ImageBitamp(ImageBitmap) constructor This CL is the follow up for CL 2522693002, part of color management implementation for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) BUG=665919 ========== to ========== Add unit test for color managed ImageBitamp(ImageBitmap) constructor This CL is the follow up for CL 2522693002, part of color management implementation for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) BUG=665919 Review-Url: https://codereview.chromium.org/2575693005 Committed: https://chromium.googlesource.com/chromium/src/+/69c10b332547d827ad1280111e14... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/69c10b332547d827ad1280111e14...
Message was sent while issue was closed.
Description was changed from ========== Add unit test for color managed ImageBitamp(ImageBitmap) constructor This CL is the follow up for CL 2522693002, part of color management implementation for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) BUG=665919 Review-Url: https://codereview.chromium.org/2575693005 Committed: https://chromium.googlesource.com/chromium/src/+/69c10b332547d827ad1280111e14... ========== to ========== Add unit test for color managed ImageBitamp(ImageBitmap) constructor This CL is the follow up for CL 2522693002, part of color management implementation for ImageBitmap as described in proposal: (github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md) BUG=665919 Committed: https://crrev.com/0165593b23888e943820fe8e96bb6001897d159b Cr-Commit-Position: refs/heads/master@{#439706} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0165593b23888e943820fe8e96bb6001897d159b Cr-Commit-Position: refs/heads/master@{#439706} |
