Closed Bug 1301639 Opened 3 years ago Closed 3 years ago

[CID 1368390] Unit issue in ADTSDemuxer

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: paul.bignier, Assigned: paul.bignier)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1368390)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160803113201
Keywords: coverity
Whiteboard: CID 1368390
* integer mult + div -> integer result. USECS_PER_S ensures that the result isn't decimal (at least negligible)
 * 'usPerFrame' is converted back to uint64_t by FromMicroseconds() anyway
 * CID 1368390
Attachment #8789743 - Flags: review?(jyavenard)
Attachment #8789743 - Attachment is obsolete: true
Attachment #8789743 - Flags: review?(jyavenard)
* integer mult + div -> integer result. USECS_PER_S ensures that the result isn't decimal (at least negligible)
 * 'usPerFrame' is converted back to uint64_t by FromMicroseconds() anyway
 * CID 1368390
Attachment #8789752 - Flags: review?(jyavenard)
Comment on attachment 8789752 [details] [diff] [review]
fixed - Fix a unit issue in ADTSDemuxer. r="jyavenard"

Review of attachment 8789752 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/ADTSDemuxer.cpp
@@ +613,5 @@
>    if (!mSamplesPerSecond) {
>      return media::TimeUnit::FromMicroseconds(-1);
>    }
>  
> +  const uint32_t usPerFrame = USECS_PER_S * mSamplesPerFrame / mSamplesPerSecond;

this is too inaccurate.
you're now rounding before the final result

keep it a double, just cast USECS_PER_S to double.
Attachment #8789752 - Flags: review?(jyavenard) → review-
if you do believe that using integer is fine.
Then use TimeUnit all the way and FramesToTimeUnit utility from VideoUtils.h
e.g.

TimeUnit frameDuration = FramesToTimeUnit(mSamplesPerFrame, mSamplesPerSecond);
return frameDuration * aNumFrames;

Actually,
return FramesToTimeUnit(aNumFrames * mSamplesPerFrame, mSamplesPerSeconds);
is probably the best approach.
* Simplified and factored computation by using FramesToTimeUnit()
 * CID 1368390
Attachment #8789757 - Flags: review?(jyavenard)
Attachment #8789752 - Attachment is obsolete: true
Attachment #8789757 - Flags: review?(jyavenard) → review+
Keywords: checkin-needed
Component: Untriaged → DOM
Product: Firefox → Core
Component: DOM → Audio/Video: Playback
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca5e5bbcf2d
Fix a unit issue in ADTSDemuxer. r=jya
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9ca5e5bbcf2d
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.