-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix ImageAnimator to render the correct frames #52236
Fix ImageAnimator to render the correct frames #52236
Conversation
Tagging subscribers to this area: @safern, @tarekgh Issue DetailsFixes #52144 with an update to how the animation timing is handled. Also fixes #39405 by using the same implementation across both Windows and Unix. This PR will remain a draft until #51981 is merged, as the animation assets used by the tests in this PR should flow through Here's what I found from the current implementation:
The new implementation converts frame delay times into frame "end times" so that we know at what time each frame's time is up. Then, as animation progresses, we can accurately determine which frame should be rendered at that time, preventing drift. This PR also adds a manual test for image animator that captures the frames of the animation for visual verification. From those tests, I was able to discover that animation on Linux (Ubuntu 20.04) is corrupting the rendered images in a couple of ways. Those seem to be issues down at the libgdiplus level though, so I didn't want to block this PR on those.
|
src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Drawing.Common/tests/System/Drawing/ImageAnimator.ManualTests.cs
Outdated
Show resolved
Hide resolved
I couldn't resist the urge to respect the animation loop limit too; it's straightforward enough. Another commit will be soon pushed with that added in. |
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! Thanks 🎉
Stopwatch stopwatch = new Stopwatch(); | ||
stopwatch.Start(); |
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.
Nit:
Stopwatch stopwatch = Stopwatch.StartNew();
Fixes #52144 with an update to how the animation timing is handled. Also fixes #39405 by using the same implementation across both Windows and Unix.
This PR will remain a draft until #51981 is merged, as the animation assets used by the tests in this PR should flow through
runtime-assets
instead of being merged intoruntime
.Here's what I found from the current implementation:
Thread.Sleep(50)
to be very accurate; it's not0
The new implementation converts frame delay times into frame "end times" so that we know at what time each frame's time is up. Then, as animation progresses, we can accurately determine which frame should be rendered at that time, preventing drift.
This PR also adds a manual test for image animator that captures the frames of the animation for visual verification. From those tests, I was able to discover that animation on Linux (Ubuntu 20.04) is corrupting the rendered images in a couple of ways. Those seem to be issues down at the libgdiplus level though, so I didn't want to block this PR on those.
ImageAnimator
wasn't respecting a GIF's animation repeat count either, but I decided to also consider that out of scope for this PR. I'll file an issue for that too, but I imagine we will close it until we receive customer reports of it.