From 7d30da68a91d1cced11a0302cc7a8aa9a0ea9af1 Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Sat, 24 Jul 2021 10:56:42 -0400 Subject: [PATCH] Prevent divide-by-zero in ImageAnimator for images with 0 delay between frames (#56223) --- .../src/System/Drawing/ImageAnimator.cs | 8 +++- .../src/System/Drawing/ImageInfo.cs | 47 ++++++++++------------ .../System/Drawing/ImageAnimator.ManualTests.cs | 1 + 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs index 49ce7c0..fdc6d75 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/ImageAnimator.cs @@ -35,6 +35,12 @@ namespace System.Drawing /// public sealed partial class ImageAnimator { + // We use a timer to apply an animation tick speeds of something a bit shorter than 50ms + // such that if the requested frame rate is about 20 frames per second, we will rarely skip + // a frame entirely. Sometimes we'll show a few more frames if available, but we will never + // show more than 25 frames a second and that's OK. + internal const int AnimationDelayMS = 40; + /// /// A list of images to be animated. /// @@ -387,7 +393,7 @@ namespace System.Drawing while (true) { - Thread.Sleep(40); + Thread.Sleep(AnimationDelayMS); // Because Thread.Sleep is not accurate, capture how much time has actually elapsed during the animation long timeElapsed = stopwatch.ElapsedMilliseconds; diff --git a/src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs b/src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs index 24b0413..a5eed45 100644 --- a/src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs +++ b/src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs @@ -29,6 +29,7 @@ namespace System.Drawing private readonly bool _animated; private EventHandler? _onFrameChangedHandler; private readonly long[]? _frameEndTimes; + private long _totalAnimationTime; private long _frameTimer; public ImageInfo(Image image) @@ -66,7 +67,21 @@ namespace System.Drawing } // Frame delays are stored in 1/100ths of a second; convert to milliseconds while accumulating - _frameEndTimes[f] = (lastEndTime += (BitConverter.ToInt32(values, i) * 10)); + // Per spec, a frame delay can be 0 which is treated as a single animation tick + int delay = BitConverter.ToInt32(values, i) * 10; + lastEndTime += delay > 0 ? delay : ImageAnimator.AnimationDelayMS; + + // Guard against overflows + if (lastEndTime < _totalAnimationTime) + { + lastEndTime = _totalAnimationTime; + } + else + { + _totalAnimationTime = lastEndTime; + } + + _frameEndTimes[f] = lastEndTime; } } @@ -95,24 +110,12 @@ namespace System.Drawing /// /// Whether the image supports animation. /// - public bool Animated - { - get - { - return _animated; - } - } + public bool Animated => _animated; /// /// The current frame has changed but the image has not yet been updated. /// - public bool FrameDirty - { - get - { - return _frameDirty; - } - } + public bool FrameDirty => _frameDirty; public EventHandler? FrameChangedHandler { @@ -127,15 +130,15 @@ namespace System.Drawing } /// - /// The total animation time of the image, in milliseconds. + /// The total animation time of the image in milliseconds, or 0 if not animated. /// - private long TotalAnimationTime => Animated ? _frameEndTimes![_frameCount - 1] : 0; + private long TotalAnimationTime => Animated ? _totalAnimationTime : 0; /// /// Whether animation should progress, respecting the image's animation support /// and if there are animation frames or loops remaining. /// - private bool ShouldAnimate => Animated ? (_loopCount == 0 || _loop <= _loopCount) : false; + private bool ShouldAnimate => TotalAnimationTime > 0 ? (_loopCount == 0 || _loop <= _loopCount) : false; /// /// Advance the animation by the specified number of milliseconds. If the advancement @@ -195,13 +198,7 @@ namespace System.Drawing /// /// The image this object wraps. /// - internal Image Image - { - get - { - return _image; - } - } + internal Image Image => _image; /// /// Selects the current frame as the active frame in the image. diff --git a/src/libraries/System.Drawing.Common/tests/System/Drawing/ImageAnimator.ManualTests.cs b/src/libraries/System.Drawing.Common/tests/System/Drawing/ImageAnimator.ManualTests.cs index 63cb4e3..84d9da6 100644 --- a/src/libraries/System.Drawing.Common/tests/System/Drawing/ImageAnimator.ManualTests.cs +++ b/src/libraries/System.Drawing.Common/tests/System/Drawing/ImageAnimator.ManualTests.cs @@ -43,6 +43,7 @@ namespace System.Drawing.Tests "animated-timer-10fps-repeat-infinite.gif", "animated-timer-100fps-repeat-2.gif", "animated-timer-100fps-repeat-infinite.gif", + "animated-timer-0-delay-all-frames.gif", }; Dictionary handlers = new(); -- 2.7.4