Fix potential overflow in GIF decoding

[fintelia]

Fixes: #877

That issue was caused by an overflow in the "frame delay" field between individual frames of the GIF frames when expressed in milliseconds. This translates to highly unrealistic frame rates of greater than 65 seconds per frame (i.e. over 1000x slower than a normal monitor refresh rate), but ideally should still not result in panics. This PR switches to using u32's to prevent the possibility of overflow while still allowing all delay values expressible by GIF images.

At the same time I noticed that there was some confusion about the actual units used to store frame delays in various places. I've renamed a bunch of places to "delay_ms" to be clear when milliseconds are in use.

Open questions: Is num_rational::Ratio really the right type to express the delay in milliseconds between frames of an animation?

[HeroicKatora]

Not the location I would suspected many bugs. Great!

[HeroicKatora]

Open questions: Is num_rational::Ratio really the right type to express the delay in milliseconds between frames of an animation?

Is it even used for any delay with numerator != 1?

[fintelia]

Is it even used for any delay with numerator != 1?

No, but it would be necessary to exactly encode something like 60 Hz. Though an integer number of nanoseconds for frame delay might also have low enough error to not matter?

[HeroicKatora]

Apparently it doesn't really matter for gif, the format itself doesn't allow for better timings, but there is no obvious disadvantage despite compile times either. Address one issue at a time I'd say, so let's simply keep that for now.

[fintelia]

Sounds good. Also, this is a breaking change since I've renamed some things/changed the type. Should I be pushing this to the next branch?

[HeroicKatora]

Right. That poses the question: Are the type changes necessary? We will have some arbitrary maximum supported frame delay in either case. While u16 is (obviously?) a bit too short in the long term other formats may/will have other limits and we shouldn't have to adjust the type every time. Maybe the pragmatic choice is to instead do:

Ratio::new(frame.delay.saturating_mul(10), 1)

and clearly document this saturation. We surely don't want to error on decoding this and it's the most accurate replacement value.

[HeroicKatora]

Add: I just noted that Frame is completely private so we are of course free to change the internals to Ration<u32> without breaking the external interface. If delay_u32 were an addition instead of a replacement we could have the best of both worlds.

[HeroicKatora]

@fintelia Do you think we can/should land this for 0.22.3?

[fintelia]

This changes a couple methods on animation::Frame so it is technically a breaking change. Merging onto the next branch might be better?

[HeroicKatora]

Ah right, yes, change the target to next in that case. OR we could maybe get away with a saturating multiplication and no type changes instead for an alternative 0.22 compatible change.

[HeroicKatora]

@fintelia Reminder that you wanted to rebase this on next and it's a partial blocker for comprehensive fuzzing.

[fintelia]

Rebased. If everything looks good, lets merge once CI passes

[HeroicKatora]

All concerns were over breaking the interface. The new names and types are more consistent so for next this is positive instead.