1383653 - [Shutdown Decoder] video can only be suspended once.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

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.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1383653 P1 - add debug messages for understanding the status of suspending video decoder;

Review commit: https://reviewboard.mozilla.org/r/160832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160832/

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1383653 P3 - notify the MediaDecoder::BackgroundVideoDecodingPermissionObserver when a tab is selected;

In bug 1274919, we only distribute the hovered/un-hovered status to BackgroundVideoDecodingPermissionObserver if a tab is in the background.

Consider the following steps:
Step 1: move mouse to a background tab -> send the hovered event.
Step 2: click on the background tab -> turn it into the foreground.
Step 3: move mouse to leave the tab -> DOES NOT send un-hovered event since the tab is already in the foreground.

In this way, the MediaDecoder is not notified the "un-hovered" event so it keeps wrong states.

In this patch, we reset the state when a tab is selected.

Review commit: https://reviewboard.mozilla.org/r/160834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160834/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 4

[Dão Gottwald [:dao]]

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?

Comment 5

[Tzuhao Kuo [:kaku]]

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 6

[Dão Gottwald [:dao]]

Here:

http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/browser/base/content/tabbrowser.xml#1194-1209

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8889757 [details]
Bug 1383653 P1 - add debug messages for understanding the status of suspending video decoder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160832/diff/1-2/

Comment 8

[Tzuhao Kuo [:kaku]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160834/diff/1-2/

Comment 9

[Dão Gottwald [:dao]]

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 10

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1383653 P2 - call tabbrowser-tab::finishUnselectedTabHoverTimer() at tabbrowser::updateCurrentBrowser();

The original caller is tabbrowser-tab::_visuallySelected setter.

Review commit: https://reviewboard.mozilla.org/r/162360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162360/

Comment 11

[Tzuhao Kuo [:kaku]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160834/diff/2-3/

Comment 12

[Tzuhao Kuo [:kaku]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b06dbca9530762785424a4d6b79487dfe854ab6b

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8891166 [details]
Bug 1383653 P2 - call tabbrowser-tab::finishUnselectedTabHoverTimer() at tabbrowser::updateCurrentBrowser();

https://reviewboard.mozilla.org/r/162360/#review167842

Comment 14

[Dão Gottwald [:dao]]

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

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

Comment 15

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fbab9f755a8
https://hg.mozilla.org/mozilla-central/rev/de7f272d1f1e
https://hg.mozilla.org/mozilla-central/rev/8c470a91f0ff