Implement the CSS image-animation property [chromium/src : main]
Stephen Chenney (Gerrit)
Stephen Chenney added 6 comments![]( "Open in Gerrit")
I didn't look at the tests. Otherwise, I like the way this is going.
raw_ptr<ImageNodeAnimationInfo> current_image_node_animation_info_ = nullptr;See comment below. I think you need to fix the FIXME.
const DOMNodeId PUASED_CACHED_FRAME_ID = -3;Typo: PAUSED_CACHED_FRAME_ID
PaintImage image = PaintImageForCurrentFrame();I think you need to add animation info as an optional arg to this method, even though it is called and overidden a lot. Maybe a separfate preparatory patch that adds the parameter, to keep the complexity of this CL down.
struct ImageNodeAnimationInfo {This should be it's own header file with the EImageAnimation conversion code too, if possible.
name: "CSSImageAnimation",Please add a comment with a link to the chromestatus entry.
Related details
- Seokho Song
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song voted and added 5 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +1 |
5 comments
raw_ptr<ImageNodeAnimationInfo> current_image_node_animation_info_ = nullptr;See comment below. I think you need to fix the FIXME.
Acknowledged
const DOMNodeId PUASED_CACHED_FRAME_ID = -3;Seokho SongTypo: PAUSED_CACHED_FRAME_ID
Done. Good catch!
I think you need to add animation info as an optional arg to this method, even though it is called and overidden a lot. Maybe a separfate preparatory patch that adds the parameter, to keep the complexity of this CL down.
Yeah. I think good idea. I'd prefer separated CL. So changed `FIXME` to `TODO`
This should be it's own header file with the EImageAnimation conversion code too, if possible.
Done
Please add a comment with a link to the chromestatus entry.
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Hi folks! It would be great to land this CL before CSSWG F2F Jan 27-Jan29 if possible! Thank you for review in advance :)
Related details
- Anders Hartvoll Ruud
- Daniel Bratell
- Dominik Röttsches
- Florin Malita
- Jeremy Roman
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 1 comment![]( "Open in Gerrit")
I am afraid I am not a good reviewer for this.
Related details
- Anders Hartvoll Ruud
- Daniel Bratell
- Florin Malita
- Jeremy Roman
- Seokho Song
- Stephen Chenney
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 4 comments![]( "Open in Gerrit")
Added -delayed.gif that hold the first frame 500ms to the test runningWPTs with setTimeout() are bad in the first place, but waiting up to 700ms seems extra painful.
I assume the flakiness (under 10ms) came from e.g. image-animation-img-running.tentative.html, where #img2 would incorrectly advance the animation before the screenshot was taken?
I wonder if it's an option (right after img2.src is set) to draw the image contents onto a canvas and look at the pixel data? (Basically make this a regular async testharness test, not a reftest.)
field_template: "keyword",Maybe add `field_group: "*",` here, to bury this value in raredata.
new_viewport_style_builder.SetImageAnimation(image_animation);Do we have a test for this propagation? It should either be tested in this CL, or we can make this change separately (along with a test).
Member<void*> own_ptrs[2];I think this is avoidable. (See other comment.)
Related details
- Daniel Bratell
- Florin Malita
- Jeremy Roman
- Seokho Song
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
I'm probably not a great reviewer for this, current rendering folks are better. There are a few already on this CL, but if you need another, I'd suggest pdr@.
Related details
- Daniel Bratell
- Florin Malita
- Seokho Song
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song voted and added 4 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +1 |
4 comments
Added -delayed.gif that hold the first frame 500ms to the test runningWPTs with setTimeout() are bad in the first place, but waiting up to 700ms seems extra painful.
I assume the flakiness (under 10ms) came from e.g. image-animation-img-running.tentative.html, where #img2 would incorrectly advance the animation before the screenshot was taken?
I wonder if it's an option (right after img2.src is set) to draw the image contents onto a canvas and look at the pixel data? (Basically make this a regular async testharness test, not a reftest.)
Yeah. I agree that the delay is bad. And your assumption is right. The flakyness frequently (almost only) occurred on `mac-rel`.
I tried using the canvas `drawImage()` to capture the image at the moment. However, according to the relevant test case[1], it only seems to draw the first frame of the GIF.
Another possible approach I considered was to capture the entire screen via `navigator.mediaDevices.getDisplayMedia` and render it to the canvas. Unfortunately, this approach is also unusable. Because it is mocked by [2]. It only generates greenish Pacman-likely shape. ðŸ˜
Is there another approach that could be suggested?
maybe `html-in-canvas`? But that is on the `test` status. So I'm not sure it is a good idea.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/canvas/element/drawing-images-to-the-canvas/2d.drawImage.animated.gif.html
[2] https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/fake_video_capture_device.cc
Maybe add `field_group: "*",` here, to bury this value in raredata.
Done
new_viewport_style_builder.SetImageAnimation(image_animation);Do we have a test for this propagation? It should either be tested in this CL, or we can make this change separately (along with a test).
Ah, yes, I think I should this one should be separated. This also need to spec-wise update too. Undo the changes, and leave todo.
Thanks!
I think this is avoidable. (See other comment.)
- Anders Hartvoll Ruud
- Florin Malita
- Stephen Chenney
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seokho Song (Gerrit)
Seokho Song voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Added -delayed.gif that hold the first frame 500ms to the test runningSeokho SongWPTs with setTimeout() are bad in the first place, but waiting up to 700ms seems extra painful.
I assume the flakiness (under 10ms) came from e.g. image-animation-img-running.tentative.html, where #img2 would incorrectly advance the animation before the screenshot was taken?
I wonder if it's an option (right after img2.src is set) to draw the image contents onto a canvas and look at the pixel data? (Basically make this a regular async testharness test, not a reftest.)
Yeah. I agree that the delay is bad. And your assumption is right. The flakyness frequently (almost only) occurred on `mac-rel`.
I tried using the canvas `drawImage()` to capture the image at the moment. However, according to the relevant test case[1], it only seems to draw the first frame of the GIF.
Another possible approach I considered was to capture the entire screen via `navigator.mediaDevices.getDisplayMedia` and render it to the canvas. Unfortunately, this approach is also unusable. Because it is mocked by [2]. It only generates greenish Pacman-likely shape. ðŸ˜Is there another approach that could be suggested?
maybe `html-in-canvas`? But that is on the `test` status. So I'm not sure it is a good idea.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/canvas/element/drawing-images-to-the-canvas/2d.drawImage.animated.gif.html
[2] https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/fake_video_capture_device.cc
Also, `drawElementImage()` a.k.a. `css-in-canvas`, renders the first frame of the gif image. Hence, I think we cannot use this approach too.
Seokho Song (Gerrit)
Seokho Song added 1 comment![]( "Open in Gerrit")
Added -delayed.gif that hold the first frame 500ms to the test runningSeokho SongWPTs with setTimeout() are bad in the first place, but waiting up to 700ms seems extra painful.
I assume the flakiness (under 10ms) came from e.g. image-animation-img-running.tentative.html, where #img2 would incorrectly advance the animation before the screenshot was taken?
I wonder if it's an option (right after img2.src is set) to draw the image contents onto a canvas and look at the pixel data? (Basically make this a regular async testharness test, not a reftest.)
Seokho SongYeah. I agree that the delay is bad. And your assumption is right. The flakyness frequently (almost only) occurred on `mac-rel`.
I tried using the canvas `drawImage()` to capture the image at the moment. However, according to the relevant test case[1], it only seems to draw the first frame of the GIF.
Another possible approach I considered was to capture the entire screen via `navigator.mediaDevices.getDisplayMedia` and render it to the canvas. Unfortunately, this approach is also unusable. Because it is mocked by [2]. It only generates greenish Pacman-likely shape. ðŸ˜Is there another approach that could be suggested?
maybe `html-in-canvas`? But that is on the `test` status. So I'm not sure it is a good idea.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/html/canvas/element/drawing-images-to-the-canvas/2d.drawImage.animated.gif.html
[2] https://source.chromium.org/chromium/chromium/src/+/main:media/capture/video/fake_video_capture_device.cc
Also, `drawElementImage()` a.k.a. `css-in-canvas`, renders the first frame of the gif image. Hence, I think we cannot use this approach too.
Opps. `html-in-canvas` I mean 😳