Closed Bug 1111518 Opened 9 years ago Closed 9 years ago

Fix the logic in MediaDecoder::CanPlayThrough()

Categories

(Core :: Audio/Video, defect)

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jhao, Assigned: jhao)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Summary: Refactor of an if-statement in HTMLMediaElement::UpdateReadyStateForData() → Refactor MediaDecoder::CanPlayThrough()
Summary: Refactor MediaDecoder::CanPlayThrough() → Fix the logic in MediaDecoder::CanPlayThrough()
Attached patch Rewrite CanPlayThrough() (obsolete) — Splinter Review
Attachment #8537636 - Flags: review?(bechen)
Attachment #8537636 - Attachment is patch: true
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.
Attachment #8537636 - Flags: feedback+
Attached patch Rewrite CanPlayThrough() (obsolete) — Splinter Review
Attachment #8537636 - Attachment is obsolete: true
Attachment #8537636 - Flags: review?(bechen)
Attachment #8537641 - Flags: review?(jwwang)
Attachment #8537641 - Attachment is patch: true
I guess you upload the old file.
Update what JW mentioned, and add commit message.
Attachment #8537641 - Attachment is obsolete: true
Attachment #8537641 - Flags: review?(jwwang)
Attachment #8537646 - Flags: review?(jwwang)
Comment on attachment 8537646 [details] [diff] [review]
Rewrite CanPlayThrough()

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

looks good to me.
Attachment #8537646 - Flags: review?(jwwang) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0284c2f98602

All tests passed except two busted and two failed, but they all passed after retry.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/119ce28762a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8537646 - Flags: approval-mozilla-aurora?
Attachment #8537646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.