# media/libtheora/include/theora/theora.h * line 214 -- ogg_uint32_t fps_denominator; /**< frame rate denominator **/ # media/libtheora/include/theora/codec.h * line 241 -- ogg_uint32_t fps_denominator; 264 nsTheoraState::MaxKeyframeOffset() 265 { 271 PRInt64 frameDuration; 272 PRInt64 keyframeDiff; 273 274 PRInt64 shift = mInfo.keyframe_granule_shift; 275 276 // Max number of frames keyframe could possibly be offset. 277 keyframeDiff = (1 << shift) - 1; 278 279 // Length of frame in ms. 280 PRInt64 d = 0; // d will be 0 if multiplication overflows. as fps_denominator is 32bit, there's no need to use MulOverflow: 281 MulOverflow(1000, mInfo.fps_denominator, d); 282 frameDuration = d / mInfo.fps_numerator; 283 284 // Total time in ms keyframe can be offset from any given frame. 285 return frameDuration * keyframeDiff; 286 }

I'd like to take on this bug if no one else was eyeing it.

Thanks!

This bug has been fixed in the mozilla hg repository by a commit that was unknown to me. In my opinion it can be closed, however the patch that I had produced for it is available here: https://github.com/abhishekbh/mozilla-central/commit/5107348bd0fd66a96a9db4587bdc6c05d25d2926 I'm attaching it to the bug just in case it is needed in the future.

Bug 601535 converted a bunch of code, including this, from the hand-rolled *Overflow functions to CheckedInt. So the code is different, but it looks like it still uses CheckedInt in a place where it's unnecessary (per timeless's analysis in comment 0).

Ah indeed Matthew. I've updated a patch without CheckedInt.

Comment on attachment 613489 [details] [diff] [review] Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset Review of attachment 613489 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/ogg/nsOggCodecState.cpp @@ +355,5 @@ > PRInt64 keyframeDiff = (1 << mInfo.keyframe_granule_shift) - 1; > > // Length of frame in usecs. > + PRInt64 d = 0; > + d = PRInt64(mInfo.fps_denominator) * USECS_PER_S; Do you need the cast to silence a warning? You don't on Windows at least... We don't really need the temporary variable "d" anymore, can you instead eliminate "d" and make that: frameDuration = (mInfo.fps_denominator * USECS_PER_S) / mInfo.fps_numerator; That makes the code a little simpler. Thanks!

Comment on attachment 613493 [details] [diff] [review] 620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset; r=reviewers Review of attachment 613493 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Ah sure. I thought it was considered better to break up long lines of code, but I suppose extra variable don't help anyone.

Actually, before we set checkin-needed, you need to change the commit message to match the guidelines and re-uploadyour patch: Guidelines: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed i.e. "Bug 620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset. r=cpearce"

Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d3abd78272 Thanks for the patch! If you're ever looking for more bugs to work on, feel free to drop by #developers on irc.mozilla.org.