All users were logged out of Bugzilla on October 13th, 2018

Don't release decoders in CompletedState if looping is true

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Depends on: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
When looping is true, playback will start again after reaching the end. We can reduce the latency by not releasing the decoders which will be slightly helpful for seamless audio playback.
(Assignee)

Updated

a year ago
See Also: → bug 654787
(Assignee)

Updated

a year ago
Assignee: nobody → jwwang
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8875624 - Flags: review?(jyavenard)
Attachment #8875625 - Flags: review?(jyavenard)
This relies on a bug I can't see here (using MediaDecoderInit).. Can you set the dependency in bugzilla please?
Flags: needinfo?(jwwang)

Comment 4

a year ago
mozreview-review
Comment on attachment 8875624 [details]
Bug 1371202. P1 - plumb the 'loop' attribute into MDSM.

https://reviewboard.mozilla.org/r/147048/#review151422
Attachment #8875624 - Flags: review?(jyavenard) → review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8875625 [details]
Bug 1371202. P2 - don't release decoders if we are looping since we will need them soon.

https://reviewboard.mozilla.org/r/147050/#review151432

::: dom/media/MediaDecoderStateMachine.cpp:1912
(Diff revision 1)
>    void Enter()
>    {
>      // TODO : use more approriate way to decide whether need to release
>      // resource in bug1367983.
>  #ifndef MOZ_WIDGET_ANDROID
> -    // We've decoded all samples. We don't need decoders anymore.
> +    if (!mMaster->mLooping) {

shouldn't we only do so if looping is true and we've reached eos ?
Attachment #8875625 - Flags: review?(jyavenard) → review+
(Assignee)

Updated

a year ago
Depends on: 1371200
Flags: needinfo?(jwwang)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8875625 [details]
Bug 1371202. P2 - don't release decoders if we are looping since we will need them soon.

https://reviewboard.mozilla.org/r/147050/#review151432

> shouldn't we only do so if looping is true and we've reached eos ?

CompletedState implies EOS. We transition to this state only after decoding all samples.

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8875625 [details]
Bug 1371202. P2 - don't release decoders if we are looping since we will need them soon.

https://reviewboard.mozilla.org/r/147050/#review151432

> CompletedState implies EOS. We transition to this state only after decoding all samples.

if so , why the check right after to see if we have a next frame?
(Assignee)

Comment 8

a year ago
Thanks for the review!

Comment 9

a year ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0e39a5c82024 -d 1ce78807187a: rebasing 401236:0e39a5c82024 "Bug 1371202. P1 - plumb the 'loop' attribute into MDSM. r=jya"
merging dom/html/HTMLMediaElement.cpp
merging dom/media/MediaDecoderStateMachine.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

a year ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9305945e748c
P1 - plumb the 'loop' attribute into MDSM. r=jya
https://hg.mozilla.org/integration/autoland/rev/52b2148d954c
P2 - don't release decoders if we are looping since we will need them soon. r=jya

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9305945e748c
https://hg.mozilla.org/mozilla-central/rev/52b2148d954c
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.