Closed Bug 1383653 Opened 7 years ago Closed 7 years ago

[Shutdown Decoder] video can only be suspended once.

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Should be a regression of bug 1274919.

STR.
1. go to about:config, 
   make sure "media.suspend-bkgnd-video.enabled" is TRUE.
   set "media.suspend-bkgnd-video.delay-ms" to ZERO.
   - it can reduce waiting time because it forces suspending video immediately once the tab goes background.
2. install about:media.
3. open a new tab, going to Youtube and playing arbitrary video.
4. put the Youtube tab on background, switch to "about:media" tab,
   make sure that "Video Decoder:" field is "blank media data decoder".
5. hover on the Youtube tab and then click it to bring it back to foreground.
6. switch to "about:media" again.

Expect.
7. The "Video Decoder:" field is "blank media data decoder".

Actual.
7. The "Video Decoder:" field is NOT "blank media data decoder", which means that we don't suspend the video decoder.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Comment on attachment 8889757 [details]
Bug 1383653 P1 - add debug messages for understanding the status of suspending video decoder;

https://reviewboard.mozilla.org/r/160832/#review166080

::: dom/media/MediaDecoder.cpp:1270
(Diff revision 1)
>  void
>  MediaDecoder::UpdateVideoDecodeMode()
>  {
>    // The MDSM may yet be set.
>    if (!mDecoderStateMachine) {
> +    LOG("UpdateVideoDecodeMode(), early return because we don't have MDSM.\n");

Remove '\n' from the message.
Attachment #8889757 - Flags: review?(jwwang) → review+
Comment on attachment 8889758 [details]
Bug 1383653 P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected;

https://reviewboard.mozilla.org/r/160834/#review166746

::: browser/base/content/tabbrowser.xml:7489
(Diff revision 1)
>            if (val) {
> +
> +            // Set unselectedTabHover to false since this tab has been selected.
> +            if (this.linkedPanel && this.linkedBrowser) {
> +              this.linkedBrowser.unselectedTabHover(false);
> +            }

I don't think this belongs in the _visuallySelected setter. updateCurrentBrowser is probably the right place to do this?
Attachment #8889758 - Flags: review?(dao+bmo) → review-
Comment on attachment 8889758 [details]
Bug 1383653 P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected;

https://reviewboard.mozilla.org/r/160834/#review166746

> I don't think this belongs in the _visuallySelected setter. updateCurrentBrowser is probably the right place to do this?

I'm not familiar with the front-end code, but I had a discussion with :ralin and he think updateCurrentBrowser should be the right place.

After going through the updateCurrentBrowser code, I think I should put the `this.linkedBrowser.unselectedTabHover(false);` logic here:
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/browser/base/content/tabbrowser.xml#1239-1283

right?
Comment on attachment 8889758 [details]
Bug 1383653 P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected;

https://reviewboard.mozilla.org/r/160834/#review167366

::: browser/base/content/tabbrowser.xml:1212
(Diff revision 2)
>                this.mCurrentTab.removeAttribute("titlechanged");
>                this.mCurrentTab.removeAttribute("attention");
> +
> +              // Call the current browser's unselectedTabHover() with false
> +              // because the tab has been selected.
> +              this.mCurrentBrowser.unselectedTabHover(false);

Can you move the finishUnselectedTabHoverTimer call as well?
Comment on attachment 8891166 [details]
Bug 1383653 P2 - call tabbrowser-tab::finishUnselectedTabHoverTimer() at tabbrowser::updateCurrentBrowser();

https://reviewboard.mozilla.org/r/162360/#review167842
Attachment #8891166 - Flags: review?(dao+bmo) → review+
Comment on attachment 8889758 [details]
Bug 1383653 P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected;

https://reviewboard.mozilla.org/r/160834/#review167844
Attachment #8889758 - Flags: review?(dao+bmo) → review+
Thanks for the review!
Pushed by tkuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fbab9f755a8
P1 - add debug messages for understanding the status of suspending video decoder; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/de7f272d1f1e
P2 - call tabbrowser-tab::finishUnselectedTabHoverTimer() at tabbrowser::updateCurrentBrowser(); r=dao
https://hg.mozilla.org/integration/autoland/rev/8c470a91f0ff
P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected; r=dao
You need to log in before you can comment on or make changes to this bug.