Closed
Bug 1111518
Opened 9 years ago
Closed 9 years ago
Fix the logic in MediaDecoder::CanPlayThrough()
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jhao, Assigned: jhao)
References
Details
Attachments
(1 file, 2 obsolete files)
3.28 KB,
patch
|
jwwang
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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()
Assignee | ||
Updated•9 years ago
|
Summary: Refactor MediaDecoder::CanPlayThrough() → Fix the logic in MediaDecoder::CanPlayThrough()
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8537636 -
Flags: review?(bechen)
Updated•9 years ago
|
Attachment #8537636 -
Attachment is patch: true
Comment 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8537636 -
Attachment is obsolete: true
Attachment #8537636 -
Flags: review?(bechen)
Attachment #8537641 -
Flags: review?(jwwang)
Assignee | ||
Updated•9 years ago
|
Attachment #8537641 -
Attachment is patch: true
Comment 5•9 years ago
|
||
I guess you upload the old file.
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/119ce28762a9
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/119ce28762a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 11•9 years ago
|
||
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?
Updated•9 years ago
|
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
Updated•9 years ago
|
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.
Description
•