1111518 - Fix the logic in MediaDecoder::CanPlayThrough()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jonathan Hao (inactive) [:jhao]]

In nearly the end of HTMLMediaElement::UpdateReadyStateForData(), 

> MediaDecoder::Statistics stats = mDecoder->GetStatistics();
> if (stats.mTotalBytes < 0 ? stats.mDownloadRateReliable
>                           : stats.mTotalBytes == stats.mDownloadPosition ||
>                             mDecoder->CanPlayThrough())
> {
>   ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA);
>   return;
> }

Benjamin suggests to put the code related to statistics into CanPlayThrough(), since there's similar comparison to statistics in CanPlayThrough(). Then HTMLMediaElement wouldn't have to deal with such details in MediaDecoder.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Benjamin and I also found some logical error in MediaDecoder::CanPlayThrough(). Currently in CanPlayThrough(), the stats.mDownloadRateReliable and stats.mPlaybackRateReliable are necessary conditions. However, if stats.mTotalBytes == stats.mDownloadPosition, i.e. the whole file has been downloaded, CanPlayThrough() should return true regardless of the mDownloadRateReliable and mPlaybackRateReliable.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rewrite CanPlayThrough()

Comment 3

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

Comment on attachment 8537636 [details] [diff] [review]
Rewrite CanPlayThrough()

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

::: dom/html/HTMLMediaElement.cpp
@@ +3227,5 @@
>    // reliable. We have to move to HAVE_ENOUGH_DATA at some point or
>    // autoplay elements for live streams will never play. Otherwise we
>    // move to HAVE_ENOUGH_DATA if we can play through the entire media
>    // without stopping to buffer.
>    MediaDecoder::Statistics stats = mDecoder->GetStatistics();

You can get rid of this line.

::: dom/media/MediaDecoder.cpp
@@ +1566,5 @@
>  bool MediaDecoder::CanPlayThrough()
>  {
>    Statistics stats = GetStatistics();
> +  if ((stats.mTotalBytes < 0 && stats.mDownloadRateReliable) ||
> +      (stats.mTotalBytes > 0 && stats.mTotalBytes == stats.mDownloadPosition)) {

Check |stats.mTotalBytes >= 0| since the file could be zero-length.

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rewrite CanPlayThrough()

Comment 5

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

I guess you upload the old file.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rewrite CanPlayThrough()

Update what JW mentioned, and add commit message.

Comment 7

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

Comment on attachment 8537646 [details] [diff] [review]
Rewrite CanPlayThrough()

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

looks good to me.

Comment 8

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0284c2f98602

All tests passed except two busted and two failed, but they all passed after retry.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/119ce28762a9

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/119ce28762a9

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8537646 [details] [diff] [review]
Rewrite CanPlayThrough()

Landed on Aurora with a=mse to reduce code variance between firefox 36 and 37.

https://hg.mozilla.org/releases/mozilla-aurora/rev/38492efb4a02

Risk is low. This affects all video playback but is a straightforward change.