Bug 1251405. Part 1. Fix a significant signed/unsigned mismatch in handling the return value of FrameAnimator::GetSingleLoopTime. r=edwin
authorTimothy Nikkel <tnikkel@gmail.com>
Fri, 04 Mar 2016 21:54:00 -0600
changeset 323195 5be4e5b20d33bf626f4c83bf7253f64f666d7e46
parent 323194 605c2c5d1ab07663c86ff9aba39be2759520dd86
child 323196 52ad95a283842c3a022a90340b4b4750bddec035
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1251405, 890743
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1251405. Part 1. Fix a significant signed/unsigned mismatch in handling the return value of FrameAnimator::GetSingleLoopTime. r=edwin GetSingleLoopTime returns -1 on exceptional cases but we used an unsigned int to hold the return value in AdvanceFrame. So the |loopTime > 0| check would succeed. Fortunately the |delay.ToMilliseconds() > loopTime| check would fail because loopTime was MAX_UNIT32, so we didn't do anything incorrect. http://hg.mozilla.org/mozilla-central/rev/263980931d1b (bug 890743) changed GetSingleLoopTime from returning 0 (and uint32_t) to -1 (and int32_t) on exceptional cases. But the caller of GetSingleLoopTime wasn't updated.
image/FrameAnimator.cpp
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -28,17 +28,17 @@ FrameAnimator::GetSingleLoopTime() const
     return -1;
   }
 
   // If we're not looping, a single loop time has no meaning
   if (mAnimationMode != imgIContainer::kNormalAnimMode) {
     return -1;
   }
 
-  uint32_t looptime = 0;
+  int32_t looptime = 0;
   for (uint32_t i = 0; i < mImage->GetNumFrames(); ++i) {
     int32_t timeout = GetTimeoutForFrame(i);
     if (timeout >= 0) {
       looptime += static_cast<uint32_t>(timeout);
     } else {
       // If we have a frame that never times out, we're probably in an error
       // case, but let's handle it more gracefully.
       NS_WARNING("Negative frame timeout - how did this happen?");
@@ -175,19 +175,24 @@ FrameAnimator::AdvanceFrame(TimeStamp aT
     }
 
     nextFrame->SetCompositingFailed(false);
   }
 
   mCurrentAnimationFrameTime = GetCurrentImgFrameEndTime();
 
   // If we can get closer to the current time by a multiple of the image's loop
-  // time, we should.
-  uint32_t loopTime = GetSingleLoopTime();
+  // time, we should. We need to be done decoding in order to know the full loop
+  // time though!
+  int32_t loopTime = GetSingleLoopTime();
   if (loopTime > 0) {
+    // We shouldn't be advancing by a whole loop unless we are decoded and know
+    // what a full loop actually is. GetSingleLoopTime should return -1 so this
+    // never happens.
+    MOZ_ASSERT(mDoneDecoding);
     TimeDuration delay = aTime - mCurrentAnimationFrameTime;
     if (delay.ToMilliseconds() > loopTime) {
       // Explicitly use integer division to get the floor of the number of
       // loops.
       uint32_t loops = static_cast<uint32_t>(delay.ToMilliseconds()) / loopTime;
       mCurrentAnimationFrameTime +=
         TimeDuration::FromMilliseconds(loops * loopTime);
     }