Move per-element image animation state to ElementAnimations [chromium/src : main]
Seokho Song (Gerrit)
Seokho Song voted and added 4 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +1 |
4 comments
Seokho SongI'm not sure I understand all of indirection going on in this patch. I can see that when we have a value that is not normal, we need to track some information for that element - but the purpose of using ElementAnimations was to have element associated data - it seems like we're storing this data on the bitmap though with a possible risk of never erasing it.
the informations are moved to ElementAnimation.
if (RuntimeEnabledFeatures::CSSImageAnimationEnabled()) {Seokho SongThese calls to clear don't need to be flag guarded since we would not add any image animations if the flag wasn't enabled so the clear would be a no-op.
Done
image_animation_map_.find(image_node_animation_info->node_id);Seokho SongWhy is image_animation_map_ a map of NodeId to ImageAnimationData on BitmapImage rather than storing ImageAnimationData on ElementAnimations? It seems like if we stored the animation data on ElementAnimations then there would be no risk of the data persisting when the element is detached or memory leaks by not removing entries.
Ahh!! Yes. That's a good idea.
We don't need to store it in `bitmap_image`. We can do this by taking the paint id and sync sequence from the Painter.
image_animation_map_.erase(animation_data_it);Seokho SongIt seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?
Yes. Here is some context:
1. The first issue was raised on [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7596743/comment/fae52653_f5d32ce5/) where we decided to defer the erase logic.
2. And the discussions continued across WIP CLs. CLs ([First](https://chromium-review.googlesource.com/c/chromium/src/+/7692355) -> [Second](https://chromium-review.googlesource.com/c/chromium/src/+/7727509)).
So the original plan was to remove the entry data (similar with the first one) after landing this CL.
However, since this CL now changed the approach to store the data in `ElementAnimation` rather than `bitmap_image`, a further patch is no longer needed.
Related details
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 6 comments![]( "Open in Gerrit")
image_animation_data_;Much like we have wrapped up the css_animations_ into a separate CssAnimations class, would you mind creating a css_image_animations_ to encapsulate additional data members and functionality? I'm recommending css be in the name since it's driven by a css property and has the same lifetime expectations as css animations.
You can directly expose the inner class similar to how we do for CssAnimations():
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/element_animations.h;l=96 so that you don't need forwarding methods.
element_animations->ClearPreviousImageAnimation();We should clear everything about the CssImageAnimations - i.e. the entire map - not just the previous value.
image_animation);Rather than updating element animations here, i think it might be cleaner to provide The new CssImageAnimations class which could implement an interface in third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.
image_node_animation_info->previous_image_animation;I don't understand why we need to use the previous image animation value. We should be storing in the ImageAnimationData what the previous sequence was.
It seems like this should be sufficient to determine what we need to do here, e.g. something like this:
```c++
switch (image_animation) {
kNormal:
// Set stored sequence id to 0.
kRunning:
// If !has_previous_animation_data, then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
// Otherwise, it will continue to use the sequence that was set.
kPaused:
// If !has_previous_animation_data, or current sequence is 0,
// then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
kStopped:
// Remove from map
}
```
previous_image_animation.has_value();Rather than using the previous image animation value, it seems like we should use whether there was an ImageAnimationData in the map on ElementAnimations.
image_animation_map_.erase(animation_data_it);Seokho SongIt seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?
Yes. Here is some context:
1. The first issue was raised on [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7596743/comment/fae52653_f5d32ce5/) where we decided to defer the erase logic.
2. And the discussions continued across WIP CLs. CLs ([First](https://chromium-review.googlesource.com/c/chromium/src/+/7692355) -> [Second](https://chromium-review.googlesource.com/c/chromium/src/+/7727509)).So the original plan was to remove the entry data (similar with the first one) after landing this CL.
However, since this CL now changed the approach to store the data in `ElementAnimation` rather than `bitmap_image`, a further patch is no longer needed.
- Florin Malita
- Seokho Song
Seokho Song (Gerrit)
Seokho Song added 7 comments![]( "Open in Gerrit")
Much like we have wrapped up the css_animations_ into a separate CssAnimations class, would you mind creating a css_image_animations_ to encapsulate additional data members and functionality? I'm recommending css be in the name since it's driven by a css property and has the same lifetime expectations as css animations.
You can directly expose the inner class similar to how we do for CssAnimations():
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/element_animations.h;l=96 so that you don't need forwarding methods.
Done
We should clear everything about the CssImageAnimations - i.e. the entire map - not just the previous value.
Done
image_animation);Rather than updating element animations here, i think it might be cleaner to provide The new CssImageAnimations class which could implement an interface in third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.
... third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
Done. It's nice!
> This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.
`previous_image_animation` in ImageNodeAnimationInfo should be provided to use on the next paint (please check other [comment](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/619cbdd7_61c9cfaf/)).
image_node_animation_info->previous_image_animation;I don't understand why we need to use the previous image animation value. We should be storing in the ImageAnimationData what the previous sequence was.
It seems like this should be sufficient to determine what we need to do here, e.g. something like this:
```c++
switch (image_animation) {
kNormal:
// Set stored sequence id to 0.
kRunning:
// If !has_previous_animation_data, then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
// Otherwise, it will continue to use the sequence that was set.
kPaused:
// If !has_previous_animation_data, or current sequence is 0,
// then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
kStopped:
// Remove from map
}
```
Since the spec mentions that the timeline should start from the original one when it transitions to normal[1], there is no effect when it transitions from `normal` to `running`. The previous image animation is needed to determine whether we separate the timeline. (Currently, separation only happens when transitioning from 1) `paused` or 2) `stopped`.)
Additionally, for the `stopped` value, we need the `previous_image_animation` because the `kAnimationNone` does not update CC's entry for the image animation controller so we need to update the entry value on the next paint. Please check the details in [2].
[2] https://chromium-review.googlesource.com/c/chromium/src/+/7687054
previous_image_animation.has_value();Rather than using the previous image animation value, it seems like we should use whether there was an ImageAnimationData in the map on ElementAnimations.
Maybe related to [this comment](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/619cbdd7_61c9cfaf/). If we need to check the previous image animation, I suppose `has_previous_image_animation` feels more appropriate.
image_animation_map_.erase(animation_data_it);Seokho SongIt seems like this is the only call to erase the image animation data state, but what happens if the node is detached from the dom or becomes display none, we would never call PaintImageForCurrentFrameWithInfo right?
Robert FlackYes. Here is some context:
1. The first issue was raised on [this CL](https://chromium-review.googlesource.com/c/chromium/src/+/7596743/comment/fae52653_f5d32ce5/) where we decided to defer the erase logic.
2. And the discussions continued across WIP CLs. CLs ([First](https://chromium-review.googlesource.com/c/chromium/src/+/7692355) -> [Second](https://chromium-review.googlesource.com/c/chromium/src/+/7727509)).So the original plan was to remove the entry data (similar with the first one) after landing this CL.
However, since this CL now changed the approach to store the data in `ElementAnimation` rather than `bitmap_image`, a further patch is no longer needed.
Thanks!
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
image_node_animation_info->previous_image_animation;Seokho SongI don't understand why we need to use the previous image animation value. We should be storing in the ImageAnimationData what the previous sequence was.
It seems like this should be sufficient to determine what we need to do here, e.g. something like this:
```c++
switch (image_animation) {
kNormal:
// Set stored sequence id to 0.
kRunning:
// If !has_previous_animation_data, then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
// Otherwise, it will continue to use the sequence that was set.
kPaused:
// If !has_previous_animation_data, or current sequence is 0,
// then set sequence id to
// image_node_animation_info->non_normal_sequence_id + 1
kStopped:
// Remove from map
}
```
Since the spec mentions that the timeline should start from the original one when it transitions to normal[1], there is no effect when it transitions from `normal` to `running`. The previous image animation is needed to determine whether we separate the timeline. (Currently, separation only happens when transitioning from 1) `paused` or 2) `stopped`.)
Additionally, for the `stopped` value, we need the `previous_image_animation` because the `kAnimationNone` does not update CC's entry for the image animation controller so we need to update the entry value on the next paint. Please check the details in [2].
[2] https://chromium-review.googlesource.com/c/chromium/src/+/7687054
Since the spec mentions that the timeline should start from the original one when it transitions to normal[1], there is no effect when it transitions from normal to running.
This is handled by preserving the 0 sequence id, no? I.e. if it starts as normal, it will get a sequence id of 0, then when we switch to running it will keep the sequence id of 0. Any new images however, should get a new sequence.
As an example, consider the following:
```js
elem.style.imageAnimation = 'normal';
elem.style.backgroundImage = 'a.gif';
setTimeout(() => {
elem.style.imageAnimation = 'running';
elem.style.imageAnimation = 'a.gif b.gif';
}, 1000);
```
When `b.gif` is added, it should *not* get the normal sequence id because it was not running previously. It should generate a new sequence. `a.gif` should continue animating with the normal sequence which it will do because we have tracked that in the map.
The previous image animation is needed to determine whether we separate the timeline.
We in fact do need to separate the timeline if any new image urls show up, but this is unrelated to the previous image animation value, and entirely determined by whether this url was already in the map.
Additionally, for the stopped value, we need the previous_image_animation because the kAnimationNone does not update CC's entry for the image animation controller so we need to update the entry value on the next paint.
This would also be handled by having stopped remove the entry from the map. This way when it goes back to running, we can see that it was not previously running on the normal sequence.
Related details
- Florin Malita
- Seokho Song
Seokho Song (Gerrit)
Seokho Song added 3 comments![]( "Open in Gerrit")
image_animation);Seokho SongRather than updating element animations here, i think it might be cleaner to provide The new CssImageAnimations class which could implement an interface in third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.
... third_party/blink/renderer/platform/graphics/ that provides the necessary methods to check whether we have data for a given bitmap and allows updating that data.
Done. It's nice!
> This way we don't have to even construct ImageNodeAnimationInfo for the normal case, nor do we have to update it at a distance from where the logic to do the update is performed.`previous_image_animation` in ImageNodeAnimationInfo should be provided to use on the next paint (please check other [comment](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/619cbdd7_61c9cfaf/)).
Marked as resolved.
Ah.. I think now I got it! we can do with like state-machine. Uh, it feels quite complex. Added a comment and test suite suite too.
Thanks! PTAL.
previous_image_animation.has_value();Seokho SongRather than using the previous image animation value, it seems like we should use whether there was an ImageAnimationData in the map on ElementAnimations.
Maybe related to [this comment](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/619cbdd7_61c9cfaf/). If we need to check the previous image animation, I suppose `has_previous_image_animation` feels more appropriate.
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 11 comments![]( "Open in Gerrit")
// └─────────────── kStopped (erase entry) ──────────┘This diagram is great! It makes it very clear what's happening.
if (accessor && image_key &&When would accessor or image_key be null?
virtual void EraseImageAnimationData(ImageResourceContent*) = 0;Please document these functions.
ImageResourceContent*) const = 0;Given we never expect to be modifying this, only replacing it, maybe we could return an optional<ImageAnimationData> instead? This would prevent the risk of using a dangling pointer if the map is modified.
class PLATFORM_EXPORT CSSImageAnimationAccessor {Let's rename this to something that conceptually represents the interface, i.e. this is ElementImageAnimationData. Also add a class comment explaining what it represents.
<img src="../../images/red.png">Typically we use green as a success case. Can you modify the animation so that green is the expectation?
Also, does this need to be an image here? Can't we create a green div box instead?
if (count < 2)If both loads don't occur on the same frame the rest of the test will be broken, right?
setTimeout(() => {This seems extremely likely to be flaky. I think it would be better to have a raf loop and use the timestamp passed to raf to determine the time each time raf is called. It would also be good to set the image sources on raf #2, so that they aren't started during the initial load where because of everything else going on you are likely to be delayed.
img2.style.imageAnimation = 'paused';If img2 pauses at 100ms, wouldn't that put it in frame 2? If so, why is the expected result red?
img2.style.color = 'blue';What does the image style color have to do with the test result? The style color doesn't affect anything does it?
getComputedStyle(img1).color;What's being tested by calling getComputedStyle before setting color?
Related details
- Florin Malita
- Seokho Song
Seokho Song (Gerrit)
Seokho Song added 11 comments![]( "Open in Gerrit")
When would accessor or image_key be null?
MM, yes It was guarded on `BitmapImage::Draw`. Used `DCHECK` instead. I found that there was a bug path for root element (#document) and fixed it.
virtual void EraseImageAnimationData(ImageResourceContent*) = 0;Seokho SongPlease document these functions.
Done
Given we never expect to be modifying this, only replacing it, maybe we could return an optional<ImageAnimationData> instead? This would prevent the risk of using a dangling pointer if the map is modified.
Done
Let's rename this to something that conceptually represents the interface, i.e. this is ElementImageAnimationData. Also add a class comment explaining what it represents.
Done
<img src="../../images/red.png">Typically we use green as a success case. Can you modify the animation so that green is the expectation?
Also, does this need to be an image here? Can't we create a green div box instead?
Typically we use green as a success case. Can you modify the animation so that green is the expectation?
Yes, we can do that. If we change it to a match test, it would look like this:
```html
<img src="../../images/red.png">
<img src="../../images/green.png">
```
Also, does this need to be an image here? Can't we create a green div box instead?
That is possible but we need to define the dimension e.g.,
```html
<style>
div {
display: inline-block;
width: 100px;
height: 50px;
}
</style>
<div style="background: red;"></div>
<div style="background:lime"></div>
```
Would you prefer this approach? If so, I think we should separate this into a new CL, so we can update the existing tests at the same time.
if (count < 2)If both loads don't occur on the same frame the rest of the test will be broken, right?
the count < 2 gate only ensures both `onloads` have fired. If the load gap exceeds ~120ms, img1 has already left frame 2 by pause time and the test breaks.
if (count < 2)If both loads don't occur on the same frame the rest of the test will be broken, right?
the count < 2 gate only ensures both `onloads` have fired. If the load gap exceeds ~120ms, img1 has already left frame 2 by pause time and the test breaks.
setTimeout(() => {This seems extremely likely to be flaky. I think it would be better to have a raf loop and use the timestamp passed to raf to determine the time each time raf is called. It would also be good to set the image sources on raf #2, so that they aren't started during the initial load where because of everything else going on you are likely to be delayed.
So does that mean we should remove `setTimeout` and do something roughly like this:
```js
requestAnimationFrame(() => {
requestAnimationFrame((t0) => {
(function tick(now) {
if (now - t0 < 120) requestAnimationFrame(tick);
else { /* run test */ }
})(t0);
});
});
```
right?
Hmm, maybe it would be better to do this in a separated CL (since they are existing tests).
img2.style.imageAnimation = 'paused';If img2 pauses at 100ms, wouldn't that put it in frame 2? If so, why is the expected result red?
Because it is `mismatch` test. maybe better to use `match`?
img2.style.color = 'blue';What does the image style color have to do with the test result? The style color doesn't affect anything does it?
Now it doesn't have any effect. The previous patchset used a custom computed style and updated it during style recalc, which caused the failure. Would it be better to just remove them?
getComputedStyle(img1).color;What's being tested by calling getComputedStyle before setting color?
It was followed from [this](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/eb810678_18a0d7b0/).
I assume that would make style recalc.
Related details
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 8 comments![]( "Open in Gerrit")
} else if (auto* document = DynamicTo<Document>(node)) {What's an example of when we invoke this? The image-animation property only applies to elements as far as I can tell from https://drafts.csswg.org/css-image-animation-1/#image-animation . I'm not even sure if the document node can render images. The documentElement is a real Element and will pass the above test.
image_node_animation_info->previous_image_animation;Acknowledged
sync_animation_sequence_id = 1;If we only use the values 0 or 1 can we convert this to an enum of Normal and Custom or something like that? I'm not sure I see where we ever use different values.
<img src="../../images/red.png">Seokho SongTypically we use green as a success case. Can you modify the animation so that green is the expectation?
Also, does this need to be an image here? Can't we create a green div box instead?
Typically we use green as a success case. Can you modify the animation so that green is the expectation?
Yes, we can do that. If we change it to a match test, it would look like this:
```html
<img src="../../images/red.png">
<img src="../../images/green.png">
```Also, does this need to be an image here? Can't we create a green div box instead?
That is possible but we need to define the dimension e.g.,
```html
<style>
div {
display: inline-block;
width: 100px;
height: 50px;
}
</style>
<div style="background: red;"></div>
<div style="background:lime"></div>
```Would you prefer this approach? If so, I think we should separate this into a new CL, so we can update the existing tests at the same time.
Yeah I'm okay with separating this into a new cl.
setTimeout(() => {Seokho SongThis seems extremely likely to be flaky. I think it would be better to have a raf loop and use the timestamp passed to raf to determine the time each time raf is called. It would also be good to set the image sources on raf #2, so that they aren't started during the initial load where because of everything else going on you are likely to be delayed.
So does that mean we should remove `setTimeout` and do something roughly like this:
```js
requestAnimationFrame(() => {
requestAnimationFrame((t0) => {
(function tick(now) {
if (now - t0 < 120) requestAnimationFrame(tick);
else { /* run test */ }
})(t0);
});
});```
right?
Hmm, maybe it would be better to do this in a separated CL (since they are existing tests).
Yes exactly. raf has more consistent timing than setTimeout. I'm not sure how lenient the test expectations are to queuing delay - i.e. how slow can frame generation be before we will fail this test?
img2.style.imageAnimation = 'paused';Seokho SongIf img2 pauses at 100ms, wouldn't that put it in frame 2? If so, why is the expected result red?
Because it is `mismatch` test. maybe better to use `match`?
I see. It would be better to use a match test.
img2.style.color = 'blue';Seokho SongWhat does the image style color have to do with the test result? The style color doesn't affect anything does it?
Now it doesn't have any effect. The previous patchset used a custom computed style and updated it during style recalc, which caused the failure. Would it be better to just remove them?
Ah perhaps we should just remove these, it is a bit confusing to have this as part of the tests.
getComputedStyle(img1).color;Seokho SongWhat's being tested by calling getComputedStyle before setting color?
It was followed from [this](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/eb810678_18a0d7b0/).
I assume that would make style recalc.
I see, got it, so let's update the comment on line 47 to say "// Test that an intermediate style update does not interfere with updating the image animation."
Related details
- Florin Malita
- Seokho Song
Seokho Song (Gerrit)
Seokho Song voted and added 7 comments![]( "Open in Gerrit")
Votes added by Seokho Song
| Commit-Queue | +1 |
7 comments
} else if (auto* document = DynamicTo<Document>(node)) {What's an example of when we invoke this? The image-animation property only applies to elements as far as I can tell from https://drafts.csswg.org/css-image-animation-1/#image-animation . I'm not even sure if the document node can render images. The documentElement is a real Element and will pass the above test.
It caused the test failures for the root e.g. [this one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/css/css-image-animation/image-animation-root-background-body-paused-no-propagation.html;l=1;drc=121e378ec1b1e354f2c1f913c326a5ab35b0e848).
The render path is the following and the `Document` node is passing:
`ViewPainter::PaintBoxDecorationBackground()` -> `ViewPainter::PaintRootElementGroup()` -> `FillLayer::IterateFillLayersInReverseOrder()` -> `BoxPainterBase::PaintFillLayer()` -> `PaintFillLayerBackground()` -> `CSSImageAnimations::CreateImageNodeAnimationInfo()`.
sync_animation_sequence_id = 1;If we only use the values 0 or 1 can we convert this to an enum of Normal and Custom or something like that? I'm not sure I see where we ever use different values.
PTAL
<img src="../../images/red.png">Seokho SongTypically we use green as a success case. Can you modify the animation so that green is the expectation?
Also, does this need to be an image here? Can't we create a green div box instead?
Robert FlackTypically we use green as a success case. Can you modify the animation so that green is the expectation?
Yes, we can do that. If we change it to a match test, it would look like this:
```html
<img src="../../images/red.png">
<img src="../../images/green.png">
```Also, does this need to be an image here? Can't we create a green div box instead?
That is possible but we need to define the dimension e.g.,
```html
<style>
div {
display: inline-block;
width: 100px;
height: 50px;
}
</style>
<div style="background: red;"></div>
<div style="background:lime"></div>
```Would you prefer this approach? If so, I think we should separate this into a new CL, so we can update the existing tests at the same time.
Yeah I'm okay with separating this into a new cl.
Acknowledged
setTimeout(() => {Seokho SongThis seems extremely likely to be flaky. I think it would be better to have a raf loop and use the timestamp passed to raf to determine the time each time raf is called. It would also be good to set the image sources on raf #2, so that they aren't started during the initial load where because of everything else going on you are likely to be delayed.
Robert FlackSo does that mean we should remove `setTimeout` and do something roughly like this:
```js
requestAnimationFrame(() => {
requestAnimationFrame((t0) => {
(function tick(now) {
if (now - t0 < 120) requestAnimationFrame(tick);
else { /* run test */ }
})(t0);
});
});```
right?
Hmm, maybe it would be better to do this in a separated CL (since they are existing tests).
Yes exactly. raf has more consistent timing than setTimeout. I'm not sure how lenient the test expectations are to queuing delay - i.e. how slow can frame generation be before we will fail this test?
Actually the flakiness made from the `takeScreenshot()` function. [Here](https://chromium-review.googlesource.com/c/chromium/src/+/7458159/comment/ee182295_e5638f1b/) some context.
I'm not sure how lenient the test expectations are to queuing delay - i.e. how slow can frame generation be before we will fail this test?
Do you mean the test suite timeout?
It says `Regular timeout: 30000, slow test timeout: 150000`
img2.style.imageAnimation = 'paused';Seokho SongIf img2 pauses at 100ms, wouldn't that put it in frame 2? If so, why is the expected result red?
Robert FlackBecause it is `mismatch` test. maybe better to use `match`?
I see. It would be better to use a match test.
Done
img2.style.color = 'blue';Seokho SongWhat does the image style color have to do with the test result? The style color doesn't affect anything does it?
Robert FlackNow it doesn't have any effect. The previous patchset used a custom computed style and updated it during style recalc, which caused the failure. Would it be better to just remove them?
Ah perhaps we should just remove these, it is a bit confusing to have this as part of the tests.
Done
getComputedStyle(img1).color;Seokho SongWhat's being tested by calling getComputedStyle before setting color?
Robert FlackIt was followed from [this](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/eb810678_18a0d7b0/).
I assume that would make style recalc.
I see, got it, so let's update the comment on line 47 to say "// Test that an intermediate style update does not interfere with updating the image animation."
- Florin Malita
- Robert Flack
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. |
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
if (count < 2)If both loads don't occur on the same frame the rest of the test will be broken, right?
the count < 2 gate only ensures both `onloads` have fired. If the load gap exceeds ~120ms, img1 has already left frame 2 by pause time and the test breaks.
I see that, but if img1 loaded and started running before img2 loaded, you would start the test with it already having made some progress. Perhaps it would be more robust if the tests had the animations in a paused state until both were loaded?
setTimeout(() => {Seokho SongThis seems extremely likely to be flaky. I think it would be better to have a raf loop and use the timestamp passed to raf to determine the time each time raf is called. It would also be good to set the image sources on raf #2, so that they aren't started during the initial load where because of everything else going on you are likely to be delayed.
Robert FlackSo does that mean we should remove `setTimeout` and do something roughly like this:
```js
requestAnimationFrame(() => {
requestAnimationFrame((t0) => {
(function tick(now) {
if (now - t0 < 120) requestAnimationFrame(tick);
else { /* run test */ }
})(t0);
});
});```
right?
Hmm, maybe it would be better to do this in a separated CL (since they are existing tests).
Seokho SongYes exactly. raf has more consistent timing than setTimeout. I'm not sure how lenient the test expectations are to queuing delay - i.e. how slow can frame generation be before we will fail this test?
Actually the flakiness made from the `takeScreenshot()` function. [Here](https://chromium-review.googlesource.com/c/chromium/src/+/7458159/comment/ee182295_e5638f1b/) some context.
I'm not sure how lenient the test expectations are to queuing delay - i.e. how slow can frame generation be before we will fail this test?Do you mean the test suite timeout?
It says `Regular timeout: 30000, slow test timeout: 150000`
No what I mean is, if it takes more than 200ms before the script runs that pauses the image animation it would reach the blue frame right?
Also, how do you know that by the time the screenshot is taken that the animation would have reached the blue frame if it were not paused when only ~100ms are waited for?
Related details
- Florin Malita
- 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 added 2 comments![]( "Open in Gerrit")
if (count < 2)Seokho SongIf both loads don't occur on the same frame the rest of the test will be broken, right?
Robert Flackthe count < 2 gate only ensures both `onloads` have fired. If the load gap exceeds ~120ms, img1 has already left frame 2 by pause time and the test breaks.
I see that, but if img1 loaded and started running before img2 loaded, you would start the test with it already having made some progress. Perhaps it would be more robust if the tests had the animations in a paused state until both were loaded?
Yes. That would have a positive effect. not sure it creates critical effect or not.
Added 'paused' on test. Will change other tests in separated CL.
if it takes more than 200ms before the script runs that pauses the image animation it would reach the blue frame right?
In theory, yes. But it is now prevented change from [the comment](https://chromium-review.googlesource.com/c/chromium/src/+/7771511/comment/e36bcaa3_c6a4f1ab/).
Also, how do you know that by the time the screenshot is taken that the animation would have reached the blue frame if it were not paused when only ~100ms are waited for?
In this test suite, we paused the image animation before taking the screenshot (on L32 and L37) to prevent it from reaching the blue frame. Without this pause, it could cause the flakiness since the `takeScreenshot()` might not taken at the exact moment. I'm planning to change the other tests in a separated CL after the CLs are landed.
Related details
- Florin Malita
- Robert Flack
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. |