1346116 - Consider a video element is in tree or not to suspend its video decoder

Status: RESOLVED FIXED

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

Description

[Tzuhao Kuo [:kaku]]

In Bug 1295921, we tried very hard to resume a video decoder synchronously, but we are not able to achieve it at this moment. We have split Bug 1295921 into bug 1345403 and bug 1345404; the former one is for tracking a video element as tainted and never suspend it again, the later one is for synchronous decoder-resume. We're going to land bug 1345403 at this moment but leave the bug 1345404 until bug 1338218 is landed.

Without, synchronous decoder-resume, we might 

The Telemetry data (https://mzl.la/2ncoeIj) shows that more that 70% of video elements, which are used as the source of drawImage() API, are not in-tree.

So, this bug is going to prevent those video elements which are not in-tree form suspended, so that we could alleviate the pain of not able to resume video decoder synchronously.

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1346116 part 1 - initialize MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible at HTMLMediaElement::FinishDecoderSetup();

Initialize the MediaDecoder::mIsElementVisible to be "!aOwner->IsHidden()" at the MediaDecoder's constructor is wrong.
Insted, we initialize both MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible to be false at the construtor,
and then assign the HTMLMediaElement's real values to them at HTMLMediaElement::FinishDecoderSetup() via the the MediaDecoder::SetActiviyChangesToDecoder().

The initialization values of MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible (in the constructor) do not matter because the valuse are
not read untile the first MediaDecoder::SetActiviyChangesToDecoder() method call.

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

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1346116 part 2 - consider a video is in-tree or not in the suspend-video-decoding policy;

We never suspend videos that is NOT in-tree because we found that, according to the Telemetry data, most (>70%) videos
which are used as the argument of drawImage() are not in-tree. So, by preventing suspending not-in-tree videos, we should
be able to alleviate the pain of not able to resume video decoders synchronously.

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

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1346116 part 3 - a test case for not suspend not-in-tree videos;

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

Comment 4

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc012bdfd0bb295d29a6cf9268a1c43b2bb1fd9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18de471e601734c05cd4bfc0b032c37b679f0c3b

Comment 5

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846407 [details]
Bug 1346116 part 1 - initialize MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible at HTMLMediaElement::FinishDecoderSetup();

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

Comment 6

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846408 [details]
Bug 1346116 part 2 - consider a video is in-tree or not in the suspend-video-decoding policy;

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

Comment 7

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846409 [details]
Bug 1346116 part 3 - a test case for not suspend not-in-tree videos;

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

Comment 8

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

Comment on attachment 8846407 [details]
Bug 1346116 part 1 - initialize MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible at HTMLMediaElement::FinishDecoderSetup();

https://reviewboard.mozilla.org/r/119452/#review121412

::: dom/html/HTMLMediaElement.cpp:4717
(Diff revision 2)
>      mDecoder->SetMinimizePrerollUntilPlaybackStarts();
>    }
>    // Notify the decoder of suspend taint.
>    mDecoder->SetSuspendTaint(mHasSuspendTaint);
> +  // Notify the decoder of the initial activity status.
> +  this->SetActiviyChangesToDecoder();

Remove "this->".

Comment 9

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

Comment on attachment 8846409 [details]
Bug 1346116 part 3 - a test case for not suspend not-in-tree videos;

https://reviewboard.mozilla.org/r/119456/#review121416

Comment 10

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

Comment on attachment 8846408 [details]
Bug 1346116 part 2 - consider a video is in-tree or not in the suspend-video-decoding policy;

https://reviewboard.mozilla.org/r/119454/#review121418

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846407 [details]
Bug 1346116 part 1 - initialize MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible at HTMLMediaElement::FinishDecoderSetup();

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

Comment 12

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846408 [details]
Bug 1346116 part 2 - consider a video is in-tree or not in the suspend-video-decoding policy;

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

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8846409 [details]
Bug 1346116 part 3 - a test case for not suspend not-in-tree videos;

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

Comment 14

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d58ae40ec66d357b9038019b4b0ee541f51cbc1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce13cf27550eabf5e753332ce7e11ac5ed082df

Comment 15

[Tzuhao Kuo [:kaku]]

Try looks good, thanks for review!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/096a4818b8c7
part 1 - initialize MediaDecoder::mIsDocumentVisible and MediaDecoder::mIsElementVisible at HTMLMediaElement::FinishDecoderSetup(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f7ca43d48a42
part 2 - consider a video is in-tree or not in the suspend-video-decoding policy; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c4688144319f
part 3 - a test case for not suspend not-in-tree videos; r=jwwang

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/096a4818b8c7
https://hg.mozilla.org/mozilla-central/rev/f7ca43d48a42
https://hg.mozilla.org/mozilla-central/rev/c4688144319f