Closed Bug 1277776 Opened 4 years ago Closed 4 years ago

Use SaferMultDiv in AudioClock::GetPositionInFrames()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Per suggestion by jya to avoid integer overflow.
Assignee: nobody → jwwang
Blocks: 1274160
Depends on: 1277188
Attachment #8759541 - Flags: review?(jyavenard) → review+
Comment on attachment 8759541 [details]
Bug 1277776 - Use SaferMultDiv in AudioClock::GetPositionInFrames(). .

https://reviewboard.mozilla.org/r/57534/#review54576

::: dom/media/AudioStream.cpp:668
(Diff revision 1)
>    mFrameHistory->Append(aServiced, aUnderrun, mOutRate);
>  }
>  
>  int64_t AudioClock::GetPositionInFrames(int64_t frames) const
>  {
> -  return (GetPosition(frames) * mInRate) / USECS_PER_S;
> +  CheckedInt64 v = UsecsToFrames(GetPosition(frames), mInRate);

Shouldnt the argument name be changed? You pass frames to know the position in frames?

and how overflow be handled in the caller?
https://reviewboard.mozilla.org/r/57534/#review54576

> Shouldnt the argument name be changed? You pass frames to know the position in frames?
> 
> and how overflow be handled in the caller?

1. Do you mean s/int64_t frames/int64_t aFrames/?
2. https://hg.mozilla.org/mozilla-central/file/3e8ee3599a67edd971770af4982ad4b0fe77f073/dom/media/AudioStream.h#l49
    frames: the playback position in frames of the audio engine, not adjusted by playback rate changes or underrun frames.
    return: the playback position in frames of the stream (i.e. the audio track in our case) adjusted by playback rate changes and underrun frames.
3. AudioStream::GetPositionInFrames() returns -1 when somethings goes wrong, which is though not handled by DecodedAudioDataSink::HasUnplayedFrames().
Comment on attachment 8759541 [details]
Bug 1277776 - Use SaferMultDiv in AudioClock::GetPositionInFrames(). .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57534/diff/1-2/
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bdeb9f1eca
Use SaferMultDiv in AudioClock::GetPositionInFrames(). r=jya.
https://hg.mozilla.org/mozilla-central/rev/57bdeb9f1eca
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.