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

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[Tzuhao Kuo [:kaku]]

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.

Comment 1

[Tzuhao Kuo [:kaku]]

This should be the root cause of intermittent bug 922951 and probably of bug 1347332.

Comment 2

[Tzuhao Kuo [:kaku]]

Try: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b4a588d130bacfc879b02a52bb3c9dde203ea7
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b963fc4beadeeb006a424eaef1fe2be642f320be

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED;

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

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1347892 part 2 - don't pass incomplete visibility states into decoder;

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.

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

Comment 5

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

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 6

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

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.

Comment 7

[Tzuhao Kuo [:kaku]]

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 8

[Tzuhao Kuo [:kaku]]

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.

Comment 9

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1347892 part 2 - pass the visibility state of media element to media decoder as a Visibility variable;

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

Comment 10

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1347892 part 3 - dont change video decode mode if a media element is in-tree with UNTRACKED visibility state;

If a media element is in-tree with UNTRACKED visibility state, the information is incomplete, just ignore it.

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

Comment 11

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8597361d31fc2c3b247f0c73246fd57ffc106c5c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74c1d938cc77be4ae50546c3930791f735188c41

Comment 12

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

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

Comment 13

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

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

Comment 14

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

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

Comment 15

[Tzuhao Kuo [:kaku]]

Try looks good, thanks for the review!

Comment 16

[Carsten Book [:Tomcat]]

Tzuhao, seems there is a open issue in mozreview in part 1 (part 2 and 3 seems fine) could you take a look, thanks!

Comment 17

[Tzuhao Kuo [:kaku]]

(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!!!

Comment 18

[Mozilla Autoland]

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)

Comment 19

[Mozilla Autoland]

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)

Comment 20

[Tzuhao Kuo [:kaku]]

Rebased and tried, try looks good, lets check-in again!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbb97cf9c80f66989cc3cd7e74d1badf134bc08e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=641813d65106bd8f6ee9c7bc477806aeb13a307e

Comment 21

[Mozilla Autoland]

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)

Comment 22

[Tzuhao Kuo [:kaku]]

My fault, I didn't push new patches to review board.......

Comment 23

[Tzuhao Kuo [:kaku]]

Comment on attachment 8848115 [details]
Bug 1347892 part 1 - initialize a media element's mVisibilityState to be UNTRACKED;

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

Comment 24

[Tzuhao Kuo [:kaku]]

Comment on attachment 8848382 [details]
Bug 1347892 part 2 - pass the visibility state of media element to media decoder as a Visibility variable;

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

Comment 25

[Tzuhao Kuo [:kaku]]

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;

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

Comment 26

[Pulsebot]

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37adc8fa8e47
https://hg.mozilla.org/mozilla-central/rev/f9a5c34a9416
https://hg.mozilla.org/mozilla-central/rev/f8ed75883a87