Closed Bug 1422657 Opened 2 years ago Closed 2 years ago

ChannelMediaDecoder::GetStatistics() should pass mPlaybackPosition to mResource->GetCachedDataEnd()

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

https://searchfox.org/mozilla-central/rev/477ac066b565ae0eb3519875581a62dfb1430e98/dom/media/ChannelMediaDecoder.cpp#440

GetStatistics() is used to evaluate 'canplaythrough' which is defined as the media resource could be rendered at the current playback rate all the way to its end without having to stop for further buffering. Therefore it makes more sense to pass the playback position instead of decoder position since 'canplaythrough' is about playback instead of decoding.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8934036 - Flags: review?(bechen)
Attachment #8934037 - Flags: review?(bechen)
Comment on attachment 8934036 [details]
Bug 1422657. P1 - GetStatistics() should pass mPlaybackPosition to mResource->GetCachedDataEnd().

https://reviewboard.mozilla.org/r/204968/#review210422
Attachment #8934036 - Flags: review?(bechen) → review+
Comment on attachment 8934037 [details]
Bug 1422657. P2 - remove unused mDecoderPosition and related code.

https://reviewboard.mozilla.org/r/204970/#review210424
Attachment #8934037 - Flags: review?(bechen) → review+
Attachment #8934036 - Flags: review?(gsquelart)
Attachment #8934037 - Flags: review?(gsquelart)
Comment on attachment 8934036 [details]
Bug 1422657. P1 - GetStatistics() should pass mPlaybackPosition to mResource->GetCachedDataEnd().

https://reviewboard.mozilla.org/r/204968/#review210428
Attachment #8934036 - Flags: review?(gsquelart) → review+
Comment on attachment 8934037 [details]
Bug 1422657. P2 - remove unused mDecoderPosition and related code.

https://reviewboard.mozilla.org/r/204970/#review210430
Attachment #8934037 - Flags: review?(gsquelart) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c77dddb2fac
P1 - GetStatistics() should pass mPlaybackPosition to mResource->GetCachedDataEnd(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/7267883c2b72
P2 - remove unused mDecoderPosition and related code. r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/9c77dddb2fac
https://hg.mozilla.org/mozilla-central/rev/7267883c2b72
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Backed out for browser chrome failures on browser_cache.js

https://hg.mozilla.org/integration/autoland/rev/d1f1e12af235415ede6221af98cdaeb5d1b2f1d6
https://treeherder.mozilla.org/logviewer.html#?job_id=149960949&repo=autoland&lineNumber=8127
Status: RESOLVED → REOPENED
Flags: needinfo?(jwwang)
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---
I can repro the timeout locally.

https://searchfox.org/mozilla-central/rev/2e08acdf8862e68b13166970e17809a3b5d6a555/browser/components/originattributes/test/browser/browser_cache.js#198-213

It looks like 'canplaythrough' is not evaluated correctly and the 'canplaythrough' event is never fired.
In fact, I think the test is wrong. There is no guarantee 'canplaythrough' will always fire when download is slow. It should listen to 'suspend' which is fired when download is stopped.
Attachment #8934848 - Flags: review?(arthuredelstein)
Attachment #8934848 - Flags: review?(honzab.moz)
Comment on attachment 8934848 [details]
Bug 1422657. P3 - listen to 'suspend' instead of 'canplaythrough'.

https://reviewboard.mozilla.org/r/205776/#review212292

I don't think I'm the right reviewer for this.
Attachment #8934848 - Flags: review?(honzab.moz)
Attachment #8934848 - Flags: review?(tihuang)
Comment on attachment 8934848 [details]
Bug 1422657. P3 - listen to 'suspend' instead of 'canplaythrough'.

https://reviewboard.mozilla.org/r/205776/#review213000

LGTM
Attachment #8934848 - Flags: review?(tihuang) → review+
Thanks!
Flags: needinfo?(jwwang)
Attachment #8934848 - Flags: review?(arthuredelstein)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98c7a448aadd
P1 - GetStatistics() should pass mPlaybackPosition to mResource->GetCachedDataEnd(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/247347cf175a
P2 - remove unused mDecoderPosition and related code. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/b8e88741a76c
P3 - listen to 'suspend' instead of 'canplaythrough'. r=timhuang
https://hg.mozilla.org/mozilla-central/rev/98c7a448aadd
https://hg.mozilla.org/mozilla-central/rev/247347cf175a
https://hg.mozilla.org/mozilla-central/rev/b8e88741a76c
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
See Also: → 1423386
You need to log in before you can comment on or make changes to this bug.