Closed Bug 1374189 Opened 2 years ago Closed 2 years ago

Mochitest for verify VideoPlaybackQuality.totalVideoFrames.

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

Attachments

(2 files)

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.
How do you tell the dropped frames are abnormal or the machine is just running too slow?
Priority: -- → P3
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().
Attachment #8879859 - Flags: review?(jwwang) → review+
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
Attachment #8879859 - Flags: review?(kaku) → review+
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 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 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.
Attachment #8884226 - Flags: review?(jwwang) → review+
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 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 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
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 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)
see comment 21
Flags: needinfo?(bechen)
Keywords: checkin-needed
Flags: needinfo?(bechen)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48875368bb68
https://hg.mozilla.org/mozilla-central/rev/4ed9cb85d459
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.