Skip to content
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

Prevent divide-by-zero in ImageAnimator for images with 0 delay between frames #56223

Merged
merged 2 commits into from
Jul 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ namespace System.Drawing
/// </summary>
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;
jeffhandley marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// A list of images to be animated.
/// </summary>
Expand Down Expand Up @@ -387,7 +393,7 @@ private static void AnimateImages()

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;
Expand Down
47 changes: 22 additions & 25 deletions src/libraries/System.Drawing.Common/src/System/Drawing/ImageInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ private sealed class ImageInfo
private readonly bool _animated;
private EventHandler? _onFrameChangedHandler;
private readonly long[]? _frameEndTimes;
private long _totalAnimationTime;
private long _frameTimer;

public ImageInfo(Image image)
Expand Down Expand Up @@ -66,7 +67,21 @@ public ImageInfo(Image image)
}

// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for cases where delay is negative? does that ever happen/is it legal or are you just being careful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if just being careful, could you instead simply force delay to be no less than zero? then I think you wouldn't need _totalAnimationTime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being careful mostly. Here's how it comes together though.

  1. If the delay is <= 0, then apply the default animation delay
  2. If the accumulated last end time overflows, then collapse all of the remaining frames into that instant (essentially skipping any frames beyond the overflow of animation time)
  3. Ensure that TotalAnimationTime is a non-zero value if any frames are to be animated
  4. Eliminate the array access from TotalAnimationTime, replacing that with _totalAnimationTime

{
lastEndTime = _totalAnimationTime;
}
else
{
_totalAnimationTime = lastEndTime;
}

_frameEndTimes[f] = lastEndTime;
}
}

Expand Down Expand Up @@ -95,24 +110,12 @@ public ImageInfo(Image image)
/// <summary>
/// Whether the image supports animation.
/// </summary>
public bool Animated
{
get
{
return _animated;
}
}
public bool Animated => _animated;

/// <summary>
/// The current frame has changed but the image has not yet been updated.
/// </summary>
public bool FrameDirty
{
get
{
return _frameDirty;
}
}
public bool FrameDirty => _frameDirty;

public EventHandler? FrameChangedHandler
{
Expand All @@ -127,15 +130,15 @@ public EventHandler? FrameChangedHandler
}

/// <summary>
/// The total animation time of the image, in milliseconds.
/// The total animation time of the image in milliseconds, or <value>0</value> if not animated.
/// </summary>
private long TotalAnimationTime => Animated ? _frameEndTimes![_frameCount - 1] : 0;
private long TotalAnimationTime => Animated ? _totalAnimationTime : 0;

/// <summary>
/// Whether animation should progress, respecting the image's animation support
/// and if there are animation frames or loops remaining.
/// </summary>
private bool ShouldAnimate => Animated ? (_loopCount == 0 || _loop <= _loopCount) : false;
private bool ShouldAnimate => TotalAnimationTime > 0 ? (_loopCount == 0 || _loop <= _loopCount) : false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behavior at line 160, preventing animation from occurring if the total animation time is <= 0.


/// <summary>
/// Advance the animation by the specified number of milliseconds. If the advancement
Expand Down Expand Up @@ -195,13 +198,7 @@ public void AdvanceAnimationBy(long milliseconds)
/// <summary>
/// The image this object wraps.
/// </summary>
internal Image Image
{
get
{
return _image;
}
}
internal Image Image => _image;

/// <summary>
/// Selects the current frame as the active frame in the image.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public void AnimateAndCaptureFrames()
"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<string, EventHandler> handlers = new();
Expand Down