If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Suspend and resume foreground video decoders according to visibility events.

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → kaku
Blocks: 1276556
Depends on: 1224973, 1157546
(Assignee)

Updated

a year ago
No longer blocks: 1276556
(Assignee)

Updated

a year ago
Blocks: 1276556
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
Created attachment 8766244 [details]
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/
Attachment #8766244 - Flags: review?(dglastonbury)
Attachment #8766244 - Flags: review?(cpearce)
Attachment #8766245 - Flags: review?(seth)
(Assignee)

Comment 3

a year ago
Created attachment 8766245 [details]
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/
Attachment #8766245 - Flags: review?(seth) → review+
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!
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.
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 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.
Attachment #8766244 - Flags: review?(cpearce) → review+
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.
Attachment #8766244 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 9

a year ago
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.
(Assignee)

Comment 10

a year ago
https://reviewboard.mozilla.org/r/61220/#review58332

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

Comment 11

a year ago
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/
(Assignee)

Comment 12

a year ago
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/
Priority: -- → P2
(Assignee)

Comment 13

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

Updated

a year ago
Keywords: checkin-needed

Comment 14

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1bbb1ab928c7
https://hg.mozilla.org/mozilla-central/rev/103dc4eddacf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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.
Depends on: 1306945
You need to log in before you can comment on or make changes to this bug.