Closed Bug 1427931 Opened 2 years ago Closed 2 years ago

Remove MediaDecoder::PinForSeek/UnpinForSeek

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/dom/media/MediaDecoder.cpp#545-548

PinForSeek() is called only when playback reaches the end. In other words, it is not called for most cases of seeking. I think it should be OK not to call it at all during seeking.

This will allow us to remove the call to MediaCacheStream::Pin/Unpin on the main thread.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8939709 - Flags: review?(bechen)
Attachment #8939710 - Flags: review?(bechen)
Comment on attachment 8939709 [details]
Bug 1427931. P1 - remove MediaDecoder::PinForSeek/UnpinForSeek.

https://reviewboard.mozilla.org/r/210018/#review215686
Attachment #8939709 - Flags: review?(bechen) → review+
Comment on attachment 8939710 [details]
Bug 1427931. P2 - assert MediaCacheStream::Pin/Unpin is called off the main thread.

https://reviewboard.mozilla.org/r/210020/#review215688
Attachment #8939710 - Flags: review?(bechen) → review+
Attachment #8939709 - Flags: review?(gsquelart)
Attachment #8939710 - Flags: review?(gsquelart)
Comment on attachment 8939709 [details]
Bug 1427931. P1 - remove MediaDecoder::PinForSeek/UnpinForSeek.

https://reviewboard.mozilla.org/r/210018/#review215698

r+ assuming you're confident enough about this, and/or you'll monitor fallout.

::: commit-message-7d81f:4
(Diff revision 1)
> +Bug 1427931. P1 - remove MediaDecoder::PinForSeek/UnpinForSeek.
> +
> +PinForSeek() is called only when playback reaches the end. In other words,
> +it is not called for most cases of seeking. It should be OK not to call it at

"should"? It sounds a bit scary!
Anything you can do to test that theory, or will you just keep an eye on possible ill effects in the wild?
Attachment #8939709 - Flags: review?(gsquelart) → review+
Comment on attachment 8939710 [details]
Bug 1427931. P2 - assert MediaCacheStream::Pin/Unpin is called off the main thread.

https://reviewboard.mozilla.org/r/210020/#review215704
Attachment #8939710 - Flags: review?(gsquelart) → review+
Comment on attachment 8939709 [details]
Bug 1427931. P1 - remove MediaDecoder::PinForSeek/UnpinForSeek.

https://reviewboard.mozilla.org/r/210018/#review215698

> "should"? It sounds a bit scary!
> Anything you can do to test that theory, or will you just keep an eye on possible ill effects in the wild?

I am quite confident our media cache will open new channels to download the missing data when necessary. Pinning is to disable data eviction temporarily to reduce the chance to open new channels and make seek a bit faster. However, I don't think the optimization doesn't worth the complexity not to mention pinning is employed only when playback reaches the end.
"I don't think the optimization is worth the complexity"
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c362d0ac18f4
P1 - remove MediaDecoder::PinForSeek/UnpinForSeek. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/0c86905e8a4a
P2 - assert MediaCacheStream::Pin/Unpin is called off the main thread. r=bechen,gerald
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/34d4a9907d4f
P1 - remove MediaDecoder::PinForSeek/UnpinForSeek. r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/b8d69a73212b
P2 - assert MediaCacheStream::Pin/Unpin is called off the main thread. r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/34d4a9907d4f
https://hg.mozilla.org/mozilla-central/rev/b8d69a73212b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.