1277776 - Use SaferMultDiv in AudioClock::GetPositionInFrames()

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

Per suggestion by jya to avoid integer overflow.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1277776 - Use SaferMultDiv in AudioClock::GetPositionInFrames(). .

Review commit: https://reviewboard.mozilla.org/r/57534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57534/

Comment 2

[Jean-Yves Avenard [:jya]]

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?

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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 4

[JW Wang [:jwwang] [:jw_wang]]

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/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bdeb9f1eca
Use SaferMultDiv in AudioClock::GetPositionInFrames(). r=jya.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57bdeb9f1eca