Last Comment Bug 620164 - nsTheoraState::MaxKeyframeOffset doesn't need to use MulOverflow
: nsTheoraState::MaxKeyframeOffset doesn't need to use MulOverflow
Status: RESOLVED FIXED
: coverity
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Abhishek Bhatnagar
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-18 14:47 PST by timeless
Modified: 2012-04-12 10:10 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset (1.21 KB, patch)
2012-04-09 21:33 PDT, Abhishek Bhatnagar
no flags Details | Diff | Review
620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset; r=reviewers (1.18 KB, patch)
2012-04-09 22:31 PDT, Abhishek Bhatnagar
cpearce: review+
Details | Diff | Review
620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset (1.18 KB, patch)
2012-04-09 22:33 PDT, Abhishek Bhatnagar
no flags Details | Diff | Review
Proposed patch for Bug 620164 (1.18 KB, patch)
2012-04-10 13:15 PDT, Abhishek Bhatnagar
cpearce: review+
Details | Diff | Review

Description timeless 2010-12-18 14:47:16 PST
# 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 }
Comment 1 Abhishek Bhatnagar 2012-01-18 20:20:42 PST
I'd like to take on this bug if no one else was eyeing it.
Comment 2 Chris Pearce (:cpearce) 2012-01-18 20:26:26 PST
Thanks!
Comment 3 Abhishek Bhatnagar 2012-04-09 20:50:46 PDT
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.
Comment 4 Matthew Gregan [:kinetik] 2012-04-09 20:57:07 PDT
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).
Comment 5 Abhishek Bhatnagar 2012-04-09 21:33:46 PDT
Created attachment 613489 [details] [diff] [review]
Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset
Comment 6 Abhishek Bhatnagar 2012-04-09 21:34:16 PDT
Ah indeed Matthew.

I've updated a patch without CheckedInt.
Comment 7 Chris Pearce (:cpearce) 2012-04-09 22:11:09 PDT
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 8 Abhishek Bhatnagar 2012-04-09 22:31:38 PDT
Created attachment 613493 [details] [diff] [review]
620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset; r=reviewers
Comment 9 Chris Pearce (:cpearce) 2012-04-09 22:33:03 PDT
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!
Comment 10 Abhishek Bhatnagar 2012-04-09 22:33:35 PDT
Created attachment 613494 [details] [diff] [review]
620164 - Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset

Ah sure.

I thought it was considered better to break up long lines of code, but I suppose extra variable don't help anyone.
Comment 11 Chris Pearce (:cpearce) 2012-04-09 22:38:45 PDT
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"
Comment 12 Abhishek Bhatnagar 2012-04-10 13:15:18 PDT
Created attachment 613731 [details] [diff] [review]
Proposed patch for Bug 620164

Removing CheckedInt and Muloverflow from nsTheoraState::MaxKeyframeOffset
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-04-11 05:01:36 PDT
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.
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-12 10:10:52 PDT
https://hg.mozilla.org/mozilla-central/rev/a5d3abd78272

Note You need to log in before you can comment on or make changes to this bug.