-
Notifications
You must be signed in to change notification settings - Fork 29.5k
Image GIF pausing #176492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Image GIF pausing #176492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces functionality to pause animated GIFs, respecting TickerMode and MediaQueryData.disableAnimations. The implementation ensures that the first frame of a GIF is always displayed even if animations are disabled on load. The changes include updates to the Image widget's state management and documentation, along with comprehensive tests to verify the new behavior. My review focuses on improving documentation clarity and test correctness. I've pointed out a typo in a doc comment, a misleading comment in the new tests, and a typo in a test name.
| /// when [TickerMode] is disabled or [MediaQuerData.disableAnimations] is | ||
| /// true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in MediaQuerData. It should be MediaQueryData. Correct documentation is important for maintainability.1
/// when [TickerMode] is disabled or [MediaQueryData.disableAnimations] isStyle Guide References
Footnotes
-
Code should be optimized for readability. Clear and correct documentation improves readability. ↩
| // Another frame arriving also doesn't do anything, because | ||
| // disableAnimations is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is slightly misleading. The second frame is processed and displayed, and only after that does the image stop listening for more frames. The test assertions correctly reflect this behavior (expect(lastFrame, 1)), but the comment suggests no update happens. A more accurate comment would improve clarity.1
// The second frame is displayed, and then the image stops listening for
// new frames since disableAnimations is true.Style Guide References
Footnotes
-
Code should be optimized for readability. Clear and accurate comments are crucial for readability and maintainability. ↩
| // Another frame arriving also doesn't do anything, because | ||
| // disableAnimations is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is slightly misleading, similar to the one in the disableAnimations test. The second frame is processed and displayed before the image stops listening for new frames. The test assertions correctly reflect this. A more accurate comment would improve clarity.1
// The second frame is displayed, and then the image stops listening for
// new frames since TickerMode is disabled.Style Guide References
Footnotes
-
Code should be optimized for readability. Clear and accurate comments are crucial for readability and maintainability. ↩
| expect(find.byType(Image), findsOneWidget); | ||
| }); | ||
|
|
||
| testWidgets('Keeps stream alive when anmiations are disabled', (WidgetTester tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the test name: "anmiations" should be "animations". Correct naming improves readability and maintainability.1
testWidgets('Keeps stream alive when animations are disabled', (WidgetTester tester) async {Style Guide References
Footnotes
-
Code should be optimized for readability. Correct naming in tests improves readability and makes test failures easier to understand. ↩
|
@evantea Let me know if this won't solve your problem! |
| bool _isPaused = false; | ||
|
|
||
| /// False when the class first is instantiated and true forever after the | ||
| /// first frame of the image is received. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if image source changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, if the image source changes we want to load the first frame of the new image. I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works now but it was a journey. I put everything related to source swapping into this into one commit: 3665a79. Summarizing:
- When I first started looking at this, the behavior was that when I paused the image and then changed the image source, the Image would disappear (the first frame was never loaded).
- I fixed that and it was pretty straightforward, but I noticed another problem: swapping the original source back in would advance it by one frame, not show the originally paused frame. If I switched back and forth between sources, they would each advance by one frame every time. Depending on the gif, this bug could be pretty noticeable.
- To solve this, I implemented a cache of the most recent frame from each unique source.
Let me know if my cache solution is overkill and we should just accept the frame advancing bug. I'm happy with how the caching solution turned out but in retrospect I probably should have saved it for a separate PR if I had known it would take a day plus of hacking.
| } | ||
|
|
||
| void _handleImageFrame(ImageInfo imageInfo, bool synchronousCall) { | ||
| if (_hasReceivedFirstFrame && _isPaused) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to worry about first frame throws error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten this part, let me know if it's still a concern.
| _resolveImage(); | ||
|
|
||
| if (TickerMode.of(context)) { | ||
| _isPaused = !TickerMode.of(context) || (MediaQuery.maybeDisableAnimationsOf(context) ?? false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not something needs to be addressed in this PR, but I am surprised maybeDisableAnimationsOf is not hooked up to the TickerMode directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about them as two different things: MediaQueryData.disableAnimations is related to the system accessibility setting while TickerMode is about definitively disabling all animations. @evantea was unable to use TickerMode because it also disabled the fade animation that displayed the image, but that is not a problem for accessibility (see #175516 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediaQueryData.disableAnimations is related to the system accessibility setting
Yes, but what if a user toggle this setting and see all the animation is still animating. That feels bad. Right now we have to rely on developer or per widget to handle them seems not too scalable.
In the same time I wonder for the fade case, whether turning on disable animation should only turn off certain animation. Or maybe ticker may be used outside the just visual animation. Then in that case I agree hook up directly to ticker mode may not be a good idea. Maybe what we want is to hardcode all animation duration to zero if disableAnimations is on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to think hard about exactly which animations should be disabled and which (if any) shouldn't when disableAnimations is true. It is reasonable that maybe some Fade animations should be disabled.
The docs for disableAnimations do give me the impression that some animations will not be fully disabled:
Whether the platform is requesting that animations be disabled or reduced as much as possible.
See also:
dart:ui.PlatformDispatcher.accessibilityFeatures, where the setting originates.
|
@justinmc copied this commit into my local workspace, using only the "Remove Animations" system setting images, GIFs paused automatically (though only after rebuilding the view). Thanks for putting this together! |
This is a combination of 5 commits, the older 4 being: WIP Image swapping WIP exploring swapping image sources and caching frames WIP swapping works without frame advancement, but some tests are failing WIP Works, but seems to be leaking
|
@chunhtai This is ready for re-review, but it seems to be failing a skwasm test in a way that I can't reproduce locally. See my comment at #176492 (comment). Since this is a P1, maybe I should split out the image source swapping fix into a follow-up PR. Interested in what you think. |
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment on the cache management, the rest LGTM
| if (oldImageInfo != null) { | ||
| SchedulerBinding.instance.addPostFrameCallback((Duration duration) { | ||
| // If oldImageInfo is still in the cache, then it will be disposed when | ||
| // the widget is disposed. Otherwise it has been evicted, so dispose it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we don't dispose them now? also the current structure seems to make the _imageInfosCache to grow infinitely when Image source keep changing during the life time of this widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this to no longer need a cache.
As to why not dispose the image info immediately, I don't know! It seems to still work if I change it to dispose immediately, and all the tests even continue to pass.
I'll open a separate PR to try that out.
|
@chunhtai I guess this only helps with Image, but that issue seems more broad. I will edit the description. |
This reverts commit a28b26a. This should no longer be necessary if I can land a pending fix in Google's internal tests.
|
I have a local commit to revert a28b26a that I will push as soon as my fix for the Google tests is merged. |
|
Google tests have been fixed in Google internal cl/829551684. Now triggering a re-run to confirm that they pass with this PR applied. |
| } else { | ||
| _isPaused = !TickerMode.of(context) || (MediaQuery.maybeDisableAnimationsOf(context) ?? false); | ||
|
|
||
| if (_isPaused && _frameNumber != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you still need to keep track of number of frames? or can this be a boolean field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to change it but I realized it is used to give the frame number to frameBuilder: https://main-api.flutter.dev/flutter/widgets/ImageFrameBuilder.html
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Flutter needs a strong story around pausing animated GIFs because it affects accessibility via MediaQueryData.disableAnimations. See #175516.
Today
Currently, it's possible to pause a gif using TickerProvider. However, if paused on initial load, the first frame will never display, and I believe that's a bug.
Currently
disableAnimationshas no effect:With this PR
Fixes #175516
Partial fix for #130976
Fixes Google b/419605327