1374189 - Mochitest for verify VideoPlaybackQuality.totalVideoFrames.

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

From 1370487 comment 24.
After the bug 1371188 landing, I think we can verify the VideoPlaybackQuality.totalVideoFrames in mochitest to ensure the playback pipeline doesn't drop frames abnormally.

Comment 1

[Blake Wu [:bwu][:blakewu]]

How do you tell the dropped frames are abnormal or the machine is just running too slow?

Comment 2

[Benjamin Chen [:bechen]]

Attachment: Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

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

Comment 3

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

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

https://reviewboard.mozilla.org/r/151268/#review156102

::: dom/media/test/test_videoPlaybackQuality_totalFrames.html:23
(Diff revision 1)
> +  document.body.appendChild(v);
> +  v.src = test.name;
> +
> +  function ended(event) {
> +    var v = event.target;
> +    is(v.getVideoPlaybackQuality().totalVideoFrames, test.totalFrameCount," totalFrames should match!");

mind the spaces.

, "total...

::: dom/media/test/test_videoPlaybackQuality_totalFrames.html:31
(Diff revision 1)
> +  }
> +  v.addEventListener("ended", ended);
> +  v.play();
> +};
> +
> +SimpleTest.waitForExplicitFinish();

Just add an element to gTestPrefs to change prefs without calling SpecialPowers.pushPrefEnv().

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

https://reviewboard.mozilla.org/r/151268/#review156156

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

https://reviewboard.mozilla.org/r/151268/#review156102

> Just add an element to gTestPrefs to change prefs without calling SpecialPowers.pushPrefEnv().

Only this test need to set "media.decoder.skip-to-next-key-frame.enabled" false.

Comment 6

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

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

https://reviewboard.mozilla.org/r/151268/#review156102

> Only this test need to set "media.decoder.skip-to-next-key-frame.enabled" false.

No, I mean to change gTestPrefs only in this test.

Comment 7

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

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

Comment 8

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

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

Comment 9

[Benjamin Chen [:bechen]]

Attachment: Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

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

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

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

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

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

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

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

Comment 13

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

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

https://reviewboard.mozilla.org/r/155176/#review161124

::: dom/media/mediasink/VideoSink.cpp:475
(Diff revision 3)
>    AssertOwnerThread();
>    // All frames are rendered, Let's resolve the promise.
>    if (VideoQueue().IsFinished() &&
>        VideoQueue().GetSize() <= 1 &&
>        !mVideoSinkEndRequest.Exists()) {
> +    if (VideoQueue().GetSize() == 1) {

It is prudent to also remove the last frame from the queue so we won't never count it twice.

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

https://reviewboard.mozilla.org/r/155176/#review161124

> It is prudent to also remove the last frame from the queue so we won't never count it twice.

If we remove the last frame from VideoQueue, we must swap |MaybeResolveEndPromise()| and |RenderVideoFrames(...)| .

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

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

Comment 16

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155176/diff/4-5/

Comment 17

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

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

https://reviewboard.mozilla.org/r/155176/#review161528

::: dom/media/mediasink/VideoSink.cpp:476
(Diff revision 5)
>    // All frames are rendered, Let's resolve the promise.
>    if (VideoQueue().IsFinished() &&
>        VideoQueue().GetSize() <= 1 &&
>        !mVideoSinkEndRequest.Exists()) {
> +    if (VideoQueue().GetSize() == 1) {
> +      // Drop the last frame since we have sent all frames to compositor.

'Drop' means something else. Say 'remove'.

::: dom/media/mediasink/VideoSink.cpp:477
(Diff revision 5)
>    if (VideoQueue().IsFinished() &&
>        VideoQueue().GetSize() <= 1 &&
>        !mVideoSinkEndRequest.Exists()) {
> +    if (VideoQueue().GetSize() == 1) {
> +      // Drop the last frame since we have sent all frames to compositor.
> +      RefPtr<VideoData> frame = VideoQueue().PopFront();

You can say:
Unused << VideoQueue().PopFront();

Comment 18

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155176/diff/5-6/

Comment 19

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155176/diff/6-7/

Comment 20

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

https://reviewboard.mozilla.org/r/151266/#review161896

Rollback to ||RefPtr<VideoData> frame = VideoQueue().PopFront();|

https://treeherder.mozilla.org/#/jobs?repo=try&revision=896ccda3782f5b86c3e6f8c2f9f3bb9f5f37f440&selectedJob=113550959

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 aa1ce4e5dfc0 -d 91c6be339537: rebasing 406985:aa1ce4e5dfc0 "Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false. r=jwwang,kaku"
merging dom/media/test/manifest.js
merging dom/media/test/mochitest.ini
warning: conflicts while merging dom/media/test/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 22

[Carsten Book [:Tomcat]]

see comment 21

Comment 23

[Benjamin Chen [:bechen]]

Comment on attachment 8879859 [details]
Bug 1374189 - new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151268/diff/4-5/

Comment 24

[Benjamin Chen [:bechen]]

Comment on attachment 8884226 [details]
Bug 1374189 - Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155176/diff/7-8/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48875368bb68
new testcase to count the total frame when "skip-to-next-key-frame.enabled" is false. r=jwwang,kaku
https://hg.mozilla.org/integration/autoland/rev/4ed9cb85d459
Call NotifyPresentedFrame when resolve the mEndPromiseHolder promise. r=jwwang

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/48875368bb68
https://hg.mozilla.org/mozilla-central/rev/4ed9cb85d459