1350837 - Change the type of AMPLE_AUDIO_USECS to TimeUnit in MDSM

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Comment 1

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

Attachment: Bug 1350837. P1 - add AMPLE_AUDIO_THRESHOLD and use it to replace AMPLE_AUDIO_USECS.

Note we can't simply change the type of AMPLE_AUDIO_USECS to TimeUnit because it is
used in a static_assert and TimeUnit::ToMicroseconds() is not a const expression.
There is no easy way to change it because CheckedInt::value() calls MOZ_ASSERT which
can't be a const expression.

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

Comment 2

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

Attachment: Bug 1350837. P2 - change the type of mAmpleAudioThresholdUsecs to TimeUnit.

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

Comment 3

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

Attachment: Bug 1350837. P3 - rename the variable and fix comments.

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

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8852734 [details]
Bug 1350837. P1 - add AMPLE_AUDIO_THRESHOLD and use it to replace AMPLE_AUDIO_USECS.

https://reviewboard.mozilla.org/r/124894/#review127458

Comment 5

[Tzuhao Kuo [:kaku]]

Comment on attachment 8852735 [details]
Bug 1350837. P2 - change the type of mAmpleAudioThresholdUsecs to TimeUnit.

https://reviewboard.mozilla.org/r/124896/#review127456

::: dom/media/MediaDecoderStateMachine.h:605
(Diff revision 1)
>    // decoded. These two fields store how many video frames and audio
>    // samples we must consume before are considered to be finished prerolling.
>    uint32_t AudioPrerollUsecs() const
>    {
>      MOZ_ASSERT(OnTaskQueue());
> -    return mAmpleAudioThresholdUsecs / 2;
> +    return mAmpleAudioThresholdUsecs.ToMicroseconds() / 2;

Would it be better to do the opration first and then call .ToMicroseconds()?

::: dom/media/MediaDecoderStateMachine.cpp:2759
(Diff revision 1)
>  
>  bool
>  MediaDecoderStateMachine::HaveEnoughDecodedAudio()
>  {
>    MOZ_ASSERT(OnTaskQueue());
> -  auto ampleAudioUSecs = mAmpleAudioThresholdUsecs * mPlaybackRate;
> +  auto ampleAudioUSecs = mAmpleAudioThresholdUsecs.ToMicroseconds() * mPlaybackRate;

Would it be better to do the opration first and then call .ToMicroseconds()?

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8852736 [details]
Bug 1350837. P3 - rename the variable and fix comments.

https://reviewboard.mozilla.org/r/124898/#review127460

Comment 7

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

Comment on attachment 8852735 [details]
Bug 1350837. P2 - change the type of mAmpleAudioThresholdUsecs to TimeUnit.

https://reviewboard.mozilla.org/r/124896/#review127456

> Would it be better to do the opration first and then call .ToMicroseconds()?

It doesn't really matter. In addition, we will let AudioPrerollUsecs() return a TimeUnit in future bugs.

> Would it be better to do the opration first and then call .ToMicroseconds()?

No, we can't do that. See bug 1351160 for the detail.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cade02d0d47f
P1 - add AMPLE_AUDIO_THRESHOLD and use it to replace AMPLE_AUDIO_USECS. r=kaku
https://hg.mozilla.org/integration/autoland/rev/53f3febc7452
P2 - change the type of mAmpleAudioThresholdUsecs to TimeUnit. r=kaku
https://hg.mozilla.org/integration/autoland/rev/e9374f4264a5
P3 - rename the variable and fix comments. r=kaku

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cade02d0d47f
https://hg.mozilla.org/mozilla-central/rev/53f3febc7452
https://hg.mozilla.org/mozilla-central/rev/e9374f4264a5