Don't update media decoder mode if the visibility information is incomplete

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kaku, Assigned: kaku)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

NotifyOwnerDocumentActivityChanged() is a callback for receiving a media element's owner document's activility change.

OnVisibilityChange() is responsible for receiving the media element's own visibility status.

It's possible that a IN-TREE media element's NotifyOwnerDocumentActivityChanged() has been called but OnVisibilityChange() haven't. At that moment, we don't have full visibility states, so don't bother to pass incomplete information into media decoder.
This should be the root cause of intermittent bug 922951 and probably of bug 1347332.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Comment on attachment 8848115 [details]
Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED;

https://reviewboard.mozilla.org/r/121016/#review123252

::: dom/html/HTMLMediaElement.cpp:4545
(Diff revision 1)
>  
>  void HTMLMediaElement::UnbindFromTree(bool aDeep,
>                                        bool aNullParent)
>  {
>    mUnboundFromTree = true;
> +  mVisibilityState = Visibility::UNTRACKED;

Why this assignment?
Comment on attachment 8848116 [details]
Bug 1347892 part 2 - don't pass incomplete visibility states into decoder;

https://reviewboard.mozilla.org/r/121018/#review123256

::: dom/html/HTMLMediaElement.cpp:7412
(Diff revision 1)
> +    return;
> +  }
> +
> +  // Don't bother to pass information to decoder if the element is in-tree but
> +  // we don't know its visibility state yet.
> +  if (!IsHidden() && IsInUncomposedDoc() && mVisibilityState == Visibility::UNTRACKED) {

This kinda introduce policy code into the media element. We should just pass mVisibilityState to the decoder which has the policy to decide whether to suspend video decoding.
Attachment #8848116 - Flags: review?(jwwang) → review-
Comment on attachment 8848115 [details]
Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED;

https://reviewboard.mozilla.org/r/121016/#review123252

> Why this assignment?

I think this is the right state for a media element which has been removed from tree. While a media element is unbinded from tree, its OnVisibilityChanged() callback won't be called anymore, so its visibility state will keeps the value while it was in-tree.
Comment on attachment 8848116 [details]
Bug 1347892 part 2 - don't pass incomplete visibility states into decoder;

https://reviewboard.mozilla.org/r/121018/#review123256

> This kinda introduce policy code into the media element. We should just pass mVisibilityState to the decoder which has the policy to decide whether to suspend video decoding.

Yup, kind of. But, this is actually not a policy for deciding video decode mode, this is a logic to prevent sending invalid information to media decoder. 

Anyway, keeping all the logic snippets in the media decoder is a good idea, I think.
Priority: -- → P3
Attachment #8848116 - Attachment is obsolete: true
Summary: Don't pass visibility information to media decoder if a media element is in-tree but we don't know its visibility state yet → Don't update media decoder mode if the visibility information is incomplete
Comment on attachment 8848382 [details]
Bug 1347892 part 2 - pass the visibility state of media element to media decoder as a Visibility variable;

https://reviewboard.mozilla.org/r/121286/#review123320
Attachment #8848382 - Flags: review?(jwwang) → review+
Comment on attachment 8848383 [details]
Bug 1347892 part 3 - dont change video decode mode if a media element is in-tree with UNTRACKED visibility state;

https://reviewboard.mozilla.org/r/121288/#review123322
Attachment #8848383 - Flags: review?(jwwang) → review+
Comment on attachment 8848115 [details]
Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED;

https://reviewboard.mozilla.org/r/121016/#review123324
Attachment #8848115 - Flags: review?(jwwang) → review+
Try looks good, thanks for the review!
Keywords: checkin-needed
Tzuhao, seems there is a open issue in mozreview in part 1 (part 2 and 3 seems fine) could you take a look, thanks!
Flags: needinfo?(kaku)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #16)
> Tzuhao, seems there is a open issue in mozreview in part 1 (part 2 and 3
> seems fine) could you take a look, thanks!

Done, thanks!!!
Flags: needinfo?(kaku)
Keywords: checkin-needed
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 f439498407b9 -d 23cfb067130f: rebasing 382553:f439498407b9 "Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED; r=jwwang"
merging dom/html/HTMLMediaElement.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
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 f439498407b9 -d 9ef832b5f02e: rebasing 382676:f439498407b9 "Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED; r=jwwang"
merging dom/html/HTMLMediaElement.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
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 f439498407b9 -d 9c152fdd4d55: rebasing 382796:f439498407b9 "Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED; r=jwwang"
merging dom/html/HTMLMediaElement.cpp
warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
My fault, I didn't push new patches to review board.......
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37adc8fa8e47
part 1 - initialize a media element's mVisibilityState to be UNTRACKED; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f9a5c34a9416
part 2 - pass the visibility state of media element to media decoder as a Visibility variable; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f8ed75883a87
part 3 - dont change video decode mode if a media element is in-tree with UNTRACKED visibility state; r=jwwang
Keywords: checkin-needed
Blocks: 1347332
https://hg.mozilla.org/mozilla-central/rev/37adc8fa8e47
https://hg.mozilla.org/mozilla-central/rev/f9a5c34a9416
https://hg.mozilla.org/mozilla-central/rev/f8ed75883a87
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.