1282710 - Suspend and resume foreground video decoders according to visibility events.

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

Bug 1224973 has reduced the video decoder usage by suspending decoding of videos in the background tags.

This bug steps further to suspend videos in the foreground tag while they are off screen and turn the decoding on again while they are going to be visible later.

Comment 1

[Tzuhao Kuo [:kaku]]

Thanks to the bug 1157546 which generalized the visibility events to all kinds of nsIFrame, we are able to know the visibility status of video elements easily now. In specific, we get the visibility events of video elements here: http://searchfox.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#664

Moreover, the visibility resolution contains three levels: {NONVISIBLE, MAY_BECOME_VISIBLE, IN_DISPLAYPORT}. I think we could implement a conservative mechanism first which is:
(1) {MAY_BECOME_VISIBLE, IN_DISPLAYPORT} -> NONVISIBLE: notify the decoder to suspend.
(2) {NONVISIBLE, MAY_BECOME_VISIBLE} -> IN_DISPLAYPORT: notify the decoder to resume.
(3) IN_DISPLAYPORT -> MAY_BECOME_VISIBLE: do nothing here because users might scroll back immediately.
(4) NONVISIBLE -> MAY_BECOME_VISIBLE: notify the decoder to resume because users might continue their scroll direction and the video might be IN_DISPLAYPORT soon.

Patches with the above mechanism are coming for review and discussion.

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1282710 part 1 - implement the suspend and resume logics in HTMLMediaElement.cpp according to visibility events;

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

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1282710 part 2 - Plumb the visibility event from nsIFrame to nsIDOMMediaElemnt;

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

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8766245 [details]
Bug 1282710 part 2 - Plumb the visibility event from nsIFrame to nsIDOMMediaElemnt;

https://reviewboard.mozilla.org/r/61222/#review58286

Looks good!

Comment 5

[Seth Fowler [:seth] [:s2h]]

https://reviewboard.mozilla.org/r/61220/#review58294

Drive-by review of part 1. =)

::: dom/html/HTMLMediaElement.cpp:5736
(Diff revision 1)
> +//     notify the decoder to resume.
> +// (3) IN_DISPLAYPORT -> MAY_BECOME_VISIBLE:
> +//     do nothing here because users might scroll back immediately.
> +// (4) NONVISIBLE -> MAY_BECOME_VISIBLE:
> +//     notify the decoder to resume because users might continue their scroll
> +//     direction and the video might be IN_DISPLAYPORT soon.

You should be aware that the MAY_BECOME_VISIBLE area is *twice* the height of the displayport right now. I don't think what you're doing here is a good idea. You should probably only resume the decoder if it's within the displayport. The displayport is already much larger than the viewport when the user is scrolling.

::: dom/html/HTMLMediaElement.cpp:5755
(Diff revision 1)
> +      if (aOldVisibility == Visibility::NONVISIBLE) {
> +        mDecoder->NotifyOwnerActivityChanged(true);
> +      } else if (aOldVisibility == Visibility::IN_DISPLAYPORT) {
> +        // Do nothing.
> +      } else {
> +        MOZ_ASSERT_UNREACHABLE("Wrong visibility change sequence.");

Remove this assert. New visibility states are still being added.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://reviewboard.mozilla.org/r/61220/#review58294

> You should be aware that the MAY_BECOME_VISIBLE area is *twice* the height of the displayport right now. I don't think what you're doing here is a good idea. You should probably only resume the decoder if it's within the displayport. The displayport is already much larger than the viewport when the user is scrolling.

In some cases it takes several hundred milliseconds to restart the video decoder. So we want to restart the video decoder in advance of the video frame being needed to be painted. Otherwise the video is unlikely to be ready to be painted when it scrolls into view, and people will think that Firefox is broken.

I'm not familiar with the terminology here, but is the displayport the area on the screen that we're currently painting? If so, then resuming the decoder when we're withing 2X of that seems like a good idea to me.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8766244 [details]
Bug 1282710 part 1 - implement the suspend and resume logics in HTMLMediaElement.cpp according to visibility events;

https://reviewboard.mozilla.org/r/61220/#review58332

r+ with issues addressed.

::: dom/html/HTMLMediaElement.cpp:5725
(Diff revision 1)
>    }
>  
>    return true;
>  }
>  
> +// the visibility resolution contains three levels:

Capital letter at the start of sentence, i.e.:

// The visibility..."

And "resolution" means the number of dots per inch etc, which I don't think is what you meant here. How about:

"The visibility enumeration contains three states"

::: dom/html/HTMLMediaElement.cpp:5739
(Diff revision 1)
> +// (4) NONVISIBLE -> MAY_BECOME_VISIBLE:
> +//     notify the decoder to resume because users might continue their scroll
> +//     direction and the video might be IN_DISPLAYPORT soon.
> +void
> +HTMLMediaElement::OnVisibilityChange(Visibility aOldVisibility,
> +                                     Visibility aNewVisibility)

I think you should add MOZ_LOG() here, to track the transitions. It will make debugging easier.

::: dom/html/HTMLMediaElement.cpp:5741
(Diff revision 1)
> +//     direction and the video might be IN_DISPLAYPORT soon.
> +void
> +HTMLMediaElement::OnVisibilityChange(Visibility aOldVisibility,
> +                                     Visibility aNewVisibility)
> +{
> +  if (mDecoder) {

if (!mDecoder) {
  return;
}

Then you don't need to indent the rest of the function. This reduces the cognitive load required to read the code.

Code is read more often than it is written. So it should be optimized for readability.

::: dom/html/HTMLMediaElement.cpp:5743
(Diff revision 1)
> +HTMLMediaElement::OnVisibilityChange(Visibility aOldVisibility,
> +                                     Visibility aNewVisibility)
> +{
> +  if (mDecoder) {
> +    switch (aNewVisibility) {
> +    case Visibility::UNTRACKED:

Indent 'case' statements. Put braces {} around 'case' bodies.


i.e.:

switch (condition) {
  case foo: {
    // ...
    break;
  }
  case bar: {
    // ...
    break;
  }
}

::: dom/html/HTMLMediaElement.cpp:5763
(Diff revision 1)
> +    case Visibility::IN_DISPLAYPORT:
> +      mDecoder->NotifyOwnerActivityChanged(true);
> +      break;
> +    }
> +  }
> +}

Blank line after end of function. So readers can tell easily whether the braces end the function or something else.

Comment 8

[u480271]

Comment on attachment 8766244 [details]
Bug 1282710 part 1 - implement the suspend and resume logics in HTMLMediaElement.cpp according to visibility events;

https://reviewboard.mozilla.org/r/61220/#review58732

Chris' comments look thorough and I have nothing more to add.

Comment 9

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61220/#review58294

> In some cases it takes several hundred milliseconds to restart the video decoder. So we want to restart the video decoder in advance of the video frame being needed to be painted. Otherwise the video is unlikely to be ready to be painted when it scrolls into view, and people will think that Firefox is broken.
> 
> I'm not familiar with the terminology here, but is the displayport the area on the screen that we're currently painting? If so, then resuming the decoder when we're withing 2X of that seems like a good idea to me.

Seth, thanks for your reminding. Just likes cpearce said, we want to do this feature in a conservative way FOR NOW; we should log the behavior and do the optimization later.

Comment 10

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/61220/#review58332

Thanks for your detailed review! I will modify the code accordingly.

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766244 [details]
Bug 1282710 part 1 - implement the suspend and resume logics in HTMLMediaElement.cpp according to visibility events;

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

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8766245 [details]
Bug 1282710 part 2 - Plumb the visibility event from nsIFrame to nsIDOMMediaElemnt;

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

Comment 13

[Tzuhao Kuo [:kaku]]

Thanks for the review!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63b07c60885

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbb1ab928c7
Part 1 - implement the suspend and resume logics in HTMLMediaElement.cpp according to visibility events; r=cpearce r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/103dc4eddacf
part 2 - Plumb the visibility event from nsIFrame to nsIDOMMediaElemnt; r=seth

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1bbb1ab928c7
https://hg.mozilla.org/mozilla-central/rev/103dc4eddacf

Comment 16

[Timothy Nikkel (:tnikkel)]

For reference, I backed this bug out and then relanded a tweaked version (to handle the difference in the visibility api that these patches use). Bug 1284350 is where the back out and relanding happened. I also plan to request uplift of those patches to aurora.