Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728193004:
Add canvas.drawImage test for animated image types (Closed)
|
Created:
3 years, 9 months ago by Noel Gordon Modified:
3 years, 9 months ago Reviewers:
scroggo_chromium CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, Eric Willigers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd canvas.drawImage test for animated image types
canvas.drawImage() for animated images should draw the initial frame
(or poster frame) of the animated image, per spec. Add a layout test
that covers GIF, APNG, and WEBP animated images, ensure the test can
also be run manually in any browser [1].
[1] Manual testing: the test uses <canvas> to read the image pixels,
so serve the test page from a local web server (Chrome's same origin
policy prevents the pixel read otherwise). If you have python handy,
a script can be used:
#!/bin/bash -
# Simple python server: run in the local directory you want to serve
# then load http://localhost:8888 in your browser.
python -m SimpleHTTPServer 8888
BUG=698487
Review-Url: https://codereview.chromium.org/2728193004
Cr-Commit-Position: refs/heads/master@{#456273}
Committed: https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0e3dc6c5c500d
Patch Set 1 #Patch Set 2 : Add manual testing support. #
Total comments: 9
Messages
Total messages: 38 (25 generated)
The CQ bit was checked by noel@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 ========== git cl try# Enter a description of the change. Add canvas.drawImage test for animated images BUG= ========== to ========== Add canvas.drawImage test for animated images types APNG, WEBP, and GIF. BUG=698487 ==========
Description was changed from ========== Add canvas.drawImage test for animated images types APNG, WEBP, and GIF. BUG=698487 ========== to ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a new test that covers GIF, APNG, and WEBP animated. BUG=698487 ==========
Description was changed from ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a new test that covers GIF, APNG, and WEBP animated. BUG=698487 ========== to ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images. BUG=698487 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
noel@chromium.org changed reviewers: + scroggo@google.com
PTAL
The CQ bit was checked by noel@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 ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images. BUG=698487 ========== to ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser. BUG=698487 ==========
Patchset #2 (id:20001) has been deleted
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 noel@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...
On 2017/03/04 08:58:31, noel gordon wrote: > PTAL Patch Set #2: Added manual test support so that the test can run in manually in any browser.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scroggo@chromium.org changed reviewers: + scroggo@chromium.org
https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html (right): https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:37: } else { // Manual test. On 2017/03/07 02:11:18, noel gordon wrote: > On 2017/03/04 08:58:31, noel gordon wrote: > > PTAL > > Patch Set #2: Added manual test support so that the test can run in manually in > any browser. What does it mean to run it manually? Just open the page? I get the following messages (on my Linux Desktop's Chrome Version 56.0.2924.87 (64-bit), or my local builds, with/without your suggestion to change handling of frame 0 at https://codereview.chromium.org/2618633004/diff/220001/third_party/WebKit/Sou...): GIF image test: FAIL: SecurityError: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. APNG image test: FAIL: SecurityError: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. WEBP image test: FAIL: SecurityError: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. (Test passes when run as a LayoutTest using run-webkit-tests.) https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:71: // Frame 0 has color pixels: PASS if the center pixel is ~green. Should we also verify that frame 10 is not ~green on the center pixel? https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:85: startNextTestIfNeeded(0); When/why is this needed? It looks like this also gets called by runTest, which is called whether run manually or not. And this would only run the test for 0?
https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html (right): https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:37: } else { // Manual test. On 2017/03/08 20:53:22, scroggo_chromium wrote: > On 2017/03/07 02:11:18, noel gordon wrote: > > On 2017/03/04 08:58:31, noel gordon wrote: > > > PTAL > > > > Patch Set #2: Added manual test support so that the test can run in manually > in > > any browser. > > What does it mean to run it manually? Just open the page? Yes, just open the page. However, you do need to serve the page with a local web server. Say you have python handy, a script: #!/bin/bash - # simple python web server, run in any directory you want to serve python -m SimpleHTTPServer 9090 If you just open the page, the <canvas> will correctly remind you that ... > GIF image test: FAIL: SecurityError: Failed to execute 'getImageData' on > 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. > APNG image test: FAIL: SecurityError: Failed to execute 'getImageData' on > 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. > WEBP image test: FAIL: SecurityError: Failed to execute 'getImageData' on > 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. ... you are not allowed to read data cross-origin (same origin policy, SOP). For developer testing-purposes only, and without needing to serve the test page, you can run chrome with a special flag to work-around the SOP. > (Test passes when run as a LayoutTest using run-webkit-tests.) (Yeah, the page is served in that case). https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:46: startNextTestIfNeeded(i); (After testing an image, this call loads the next test image if needed). https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:71: // Frame 0 has color pixels: PASS if the center pixel is ~green. On 2017/03/08 20:53:22, scroggo_chromium wrote: > Should we also verify that frame 10 is not ~green on the center pixel? Ideally, but there's no way to do that with windows.internals currently :/ And there's no web API that I know of that can read frame 10 pixels either. <canvas> can only read frame 0 pixels of an animated image, per the spec. https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:85: startNextTestIfNeeded(0); On 2017/03/08 20:53:22, scroggo_chromium wrote: > When/why is this needed? All the <img> tags are defined like this <img onload='loaded(this, ...)'><br> None of them have a source, right? They'll display nothing until they do. So this call is needed to load the first image: setting img[0].src makes the <img> HTTP fetch a test image, and fire the JS onload() event when all the encoded image data is received from the HTTP request. > It looks like this also gets called by runTest, which > is called whether run manually or not. Yeap, correct, it's used to fetch the next image we need to test, after we have tested the current image. The images must be fetched in order so the test output is the same always, since I wanted the test output to tell you which image type broke if the test FAIL-ed. Note that if we instead had <img>.src set on each of them at page load time, then their JS onload events could fire in any order. Result: flaky test. We could work around that (order of onload event firing) if needed, e.g., count the onload's and when there are 3, then test each image then or some such. > And this would only run the test for 0? Not "only" for 0. It runs on page-load, so the test sequence is startNextTestIfNeeded(0) -> img[0].src = ... <img>[0].onload() -> runTest() -> startNextTestIfNeeded(1) -> <img>[1].src = ... <img>[1].onload() -> runTest() -> startNextTestIfNeeded(2) -> <img>[2].src = ... <img>[2].onload() -> runTest() -> startNextTestIfNeeded(3) -> notifyDone
lgtm
Description was changed from ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser. BUG=698487 ========== to ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 ==========
https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html (right): https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:37: } else { // Manual test. On 2017/03/09 03:41:00, noel gordon wrote: > On 2017/03/08 20:53:22, scroggo_chromium wrote: > > On 2017/03/07 02:11:18, noel gordon wrote: > > > On 2017/03/04 08:58:31, noel gordon wrote: > > > > PTAL > > > > > > Patch Set #2: Added manual test support so that the test can run in manually > > in > > > any browser. > > > > What does it mean to run it manually? Just open the page? > > Yes, just open the page. However, you do need to serve the page with a local > web server. Say you have python handy, a script: > > #!/bin/bash - > # simple python web server, run in any directory you want to serve > python -m SimpleHTTPServer 9090 ... then load http://localhost:9090 in your browser. Added some notes about manual testing to the change description.
On 2017/03/10 21:53:31, scroggo_chromium wrote: > lgtm Thanks.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add canvas.drawImage test for animated images types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 ========== to ========== Add canvas.drawImage test for animated image types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 ==========
https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html (right): https://codereview.chromium.org/2728193004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/canvas-drawImage-animated-images.html:73: pixel.data[0] < 5 && pixel.data[1] > 180 && pixel.data[2] < 5; Coulda also called this var |notGray|, I suppose. Black, white, and shades of gray have no color, their RGB values are equal pixel.data[0] == pixel.data[1] == pixel.data[2] So this line: pixel.data[0] < 5 && pixel.data[1] > 180 && pixel.data[2] < 5 checks the pixel values are no where near equal.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489196438006520,
"parent_rev": "362c621af1b1c9ad8da5bac607a082108b4aa213", "commit_rev":
"427dfb952160b6402103f5db70a0e3dc6c5c500d"}
Message was sent while issue was closed.
Description was changed from ========== Add canvas.drawImage test for animated image types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 ========== to ========== Add canvas.drawImage test for animated image types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 Review-Url: https://codereview.chromium.org/2728193004 Cr-Commit-Position: refs/heads/master@{#456273} Committed: https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0...
Message was sent while issue was closed.
On 2017/03/11 03:41:30, commit-bot: I haz the power wrote: > Committed patchset #2 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0... Also Leon, should this patch's bug be linked to the APNG bug?
Message was sent while issue was closed.
On 2017/03/13 13:53:59, noel gordon wrote: > On 2017/03/11 03:41:30, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:40001) as > > > https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0... > > Also Leon, should this patch's bug be linked to the APNG bug? Sure. Updated the bug.
Message was sent while issue was closed.
Description was changed from ========== Add canvas.drawImage test for animated image types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 Review-Url: https://codereview.chromium.org/2728193004 Cr-Commit-Position: refs/heads/master@{#456273} Committed: https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0... ========== to ========== Add canvas.drawImage test for animated image types canvas.drawImage() for animated images should draw the initial frame (or poster frame) of the animated image, per spec. Add a layout test that covers GIF, APNG, and WEBP animated images, ensure the test can also be run manually in any browser [1]. [1] Manual testing: the test uses <canvas> to read the image pixels, so serve the test page from a local web server (Chrome's same origin policy prevents the pixel read otherwise). If you have python handy, a script can be used: #!/bin/bash - # Simple python server: run in the local directory you want to serve # then load http://localhost:8888 in your browser. python -m SimpleHTTPServer 8888 BUG=698487 Review-Url: https://codereview.chromium.org/2728193004 Cr-Commit-Position: refs/heads/master@{#456273} Committed: https://chromium.googlesource.com/chromium/src/+/427dfb952160b6402103f5db70a0... ==========
Message was sent while issue was closed.
noel@chromium.org changed reviewers: - scroggo@google.com |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
