1473714 - FrameAnimator compares timestamps across processes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement, P3)

Description

[Tom Ritter [:tjr]]

While working on Fuzzyfox, I started hitting this assertion: https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/image/FrameAnimator.cpp#241

> NS_ASSERTION(aTime <= TimeStamp::Now(),
>               "Given time appears to be in the future");

This happens because aTime comes from the parent process and this code (and thus TimeStamp::Now()) executes in the content process. aTime comes from nsRefreshDriver.

Comparing timestamps across processes is problematic regardless of Fuzzyfox: there is no guarentee that such timestamps are consistent or in lockstep. Practically though it's unlikely to cause major issues unless one starts going to muck with time!  And the default behavior of NS_ASSERTION is just to print a warning in debug mode (and nothing in release).

Empirical testing (observing gifs while seeing the warnings print in the console) indicates this doesn't really cause problems. (At least with the 100/2000 us settings I have for Fuzzyfox right now.)

Comment 1

[Timothy Nikkel (:tnikkel)]

I don't think there is any reason we need that assertion.

There are two other uses of Now in FrameAnimator:
-GetCurrentImgFrameEndTime uses Now plus a day for "forever" frame timeouts of animated images, not realistically a problem
-InitAnimationFrameTimeIfNecessary sets mCurrentAnimationFrameTime to Now which might actually cause confusion in the code that advances frames if the refresh driver time we get after this is actually before the saved time stamp, BUT we should never call into that code because FrameAnimator::RequestRefresh only advances frames if the current frame end time is in the past relative to the refresh driver time.

All other uses appear to view time as relative to the passed in refresh driver time that I can see.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

I agree, it should just be removed.

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1473714 Remove unneeded commit in image/FrameAnimator.cpp:AdvanceFrame

Comment 4

[Phabricator Automation]

Comment on attachment 8996827 [details]
Bug 1473714 Remove unneeded commit in image/FrameAnimator.cpp:AdvanceFrame

Timothy Nikkel (:tnikkel) has approved the revision.

https://phabricator.services.mozilla.com/D2622

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/465005e948bf
Remove unneeded commit in image/FrameAnimator.cpp:AdvanceFrame

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/465005e948bf