Bug 211645 - Enable the mock video presentation mode in related layout tests and fix test failures
Summary: Enable the mock video presentation mode in related layout tests and fix test ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
: 137311 175195 187618 193399 200957 (view as bug list)
Depends on:
Blocks: 163291
  Show dependency treegraph
 
Reported: 2020-05-08 14:39 PDT by Peng Liu
Modified: 2020-05-18 23:41 PDT (History)
17 users (show)

See Also:


Attachments
WIP patch (30.21 KB, patch)
2020-05-09 16:11 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch v2 (30.79 KB, patch)
2020-05-09 23:10 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch v3 (57.54 KB, patch)
2020-05-10 11:41 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch v4 (61.94 KB, patch)
2020-05-10 15:36 PDT, Peng Liu
darin: review+
Details | Formatted Diff | Diff
Patch for landing (61.91 KB, patch)
2020-05-11 12:29 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
A follow-up patch to fix build failures (2.43 KB, patch)
2020-05-11 15:34 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-05-08 14:39:30 PDT
Enable mock video presentation mode in layout test media/media-fullscreen-loop-inline.html
Comment 1 Peng Liu 2020-05-09 16:11:23 PDT
Created attachment 398943 [details]
WIP patch
Comment 2 Radar WebKit Bug Importer 2020-05-09 16:16:29 PDT
<rdar://problem/63057695>
Comment 3 Peng Liu 2020-05-09 23:10:45 PDT
Created attachment 398960 [details]
WIP patch v2
Comment 4 Peng Liu 2020-05-10 11:41:57 PDT
Created attachment 398979 [details]
WIP patch v3
Comment 5 Peng Liu 2020-05-10 15:36:28 PDT
Created attachment 398985 [details]
WIP patch v4
Comment 6 Darin Adler 2020-05-10 23:13:47 PDT
Comment on attachment 398985 [details]
WIP patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review

> Source/WebCore/ChangeLog:3
> +        Enable the mock video presentation mode in related layout tests and fix test failures

This title is confusing. This is a patch that fixes some bugs in our media code. I think that ideally the bug title would make it clear what those bugs are, not just the technique used to test to find them.

Or does this patch's cleanup of internal state only relate to enabling more tests, and not fix any bugs?
Comment 7 Peng Liu 2020-05-11 11:19:27 PDT
Comment on attachment 398985 [details]
WIP patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review

>> Source/WebCore/ChangeLog:3
>> +        Enable the mock video presentation mode in related layout tests and fix test failures
> 
> This title is confusing. This is a patch that fixes some bugs in our media code. I think that ideally the bug title would make it clear what those bugs are, not just the technique used to test to find them.
> 
> Or does this patch's cleanup of internal state only relate to enabling more tests, and not fix any bugs?

Thanks for the reviewing and sorry for the confusing title.

Initially, this patch was to un-skip some tests competing resources (specifically Picture-in-Picture and video fullscreen) by enabling the mock video presentation mode. But after un-skipping these tests, I found other test flakiness (timeout and crash due to assertion failures). So the patch got bigger and bigger. To summary, this patch does the following things:

(1) Enable the mock video presentation mode for some tests related to picture-in-picture and video fullscreen to make sure they can run in parallel.
(2) Cleanup the internal state variables of HTMLMediaElement to fix an assertion failure in a special case only happens in the test (on Mac, and element fullscreen API is disabled).
(3) Fix a mistake in r202274 (again, the mistake only affects tests like item (2). 
(4) Update HTMLVideoElement::webkitDisplayingFullscreen() to fix the timing issue of some tests. The updated function returns true after the entering fullscreen/Picture-in-Picture is really completed. Many tests use that function to check state and proceed the test when its return value is changed.
(5) There are some minor clean-up in this patch as well, such as adding/fixing comments in the tests, and removing unnecessary code in HTMLMediaElement::exitFullscreen().

So this patch only fixes issues found in the tests and make the tests deterministic.
Comment 8 Darin Adler 2020-05-11 11:24:14 PDT
(In reply to Peng Liu from comment #7)
> So this patch only fixes issues found in the tests and make the tests
> deterministic.

OK, so to confirm, no benefit outside the tests?
Comment 9 Peng Liu 2020-05-11 11:29:29 PDT
Correct.
Comment 10 Peng Liu 2020-05-11 11:32:18 PDT
The issues fixed by this patch only affect tests as far as I know.
Comment 11 Darin Adler 2020-05-11 11:34:19 PDT
Comment on attachment 398985 [details]
WIP patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226
> +    return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture());

Too many parentheses here.

> LayoutTests/media/media-fullscreen-loop-inline.html:22
> +        window.internals.settings.setAllowsInlineMediaPlayback(false);
> +        window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true);
> +        // Disable the Fullscreen API (element fullscreen) support
> +        window.internals.settings.setFullScreenEnabled(false);
> +        window.internals.setMockVideoPresentationModeEnabled(true);
> +        window.internals.setMediaElementRestrictions(video, "NoRestrictions");

These should all be just "internals", not "window.internals".

> LayoutTests/media/media-fullscreen-pause-inline.html:23
> +        window.internals.settings.setAllowsInlineMediaPlayback(false);
> +        window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true);
> +        // Disable the Fullscreen API (element fullscreen) support
> +        window.internals.settings.setFullScreenEnabled(false);
> +        window.internals.setMockVideoPresentationModeEnabled(true);
> +        window.internals.setMediaElementRestrictions(video, "NoRestrictions");

Ditto.

> LayoutTests/media/media-fullscreen-return-to-inline.html:23
> +        window.internals.settings.setAllowsInlineMediaPlayback(false);
> +        window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(false);
> +        // Disable the Fullscreen API (element fullscreen) support
> +        window.internals.settings.setFullScreenEnabled(false);
> +        window.internals.setMockVideoPresentationModeEnabled(true);

Ditto.
Comment 12 Darin Adler 2020-05-11 11:35:44 PDT
Comment on attachment 398985 [details]
WIP patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review

>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226
>> +    return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture());
> 
> Too many parentheses here.

The second set make sense, but the first set aren’t needed.
Comment 13 Peng Liu 2020-05-11 12:29:51 PDT
Created attachment 399044 [details]
Patch for landing
Comment 14 Peng Liu 2020-05-11 12:33:30 PDT
Comment on attachment 398985 [details]
WIP patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=398985&action=review

>>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:226
>>> +    return (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard) || (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture && supportsPictureInPicture());
>> 
>> Too many parentheses here.
> 
> The second set make sense, but the first set aren’t needed.

Right, fixed.

>> LayoutTests/media/media-fullscreen-loop-inline.html:22
>> +        window.internals.setMediaElementRestrictions(video, "NoRestrictions");
> 
> These should all be just "internals", not "window.internals".

Fixed.

>> LayoutTests/media/media-fullscreen-pause-inline.html:23
>> +        window.internals.setMediaElementRestrictions(video, "NoRestrictions");
> 
> Ditto.

Fixed.

>> LayoutTests/media/media-fullscreen-return-to-inline.html:23
>> +        window.internals.setMockVideoPresentationModeEnabled(true);
> 
> Ditto.

Fixed.

> LayoutTests/media/media-fullscreen-return-to-inline.html:47
> +        window.internals.settings.setAllowsInlineMediaPlaybackAfterFullscreen(true);

Replaced the "window.internals" with "internals".

> LayoutTests/media/video-fullscreen-only-playback.html:23
> +                window.internals.setMockVideoPresentationModeEnabled(true);

Replaced "window.internals" with "internals".
Comment 15 EWS 2020-05-11 14:03:29 PDT
Committed r261493: <https://trac.webkit.org/changeset/261493>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399044 [details].
Comment 16 Peng Liu 2020-05-11 14:14:45 PDT
*** Bug 175195 has been marked as a duplicate of this bug. ***
Comment 17 Peng Liu 2020-05-11 14:15:50 PDT
*** Bug 193399 has been marked as a duplicate of this bug. ***
Comment 18 Peng Liu 2020-05-11 14:16:16 PDT
*** Bug 187618 has been marked as a duplicate of this bug. ***
Comment 19 Peng Liu 2020-05-11 14:17:02 PDT
*** Bug 182571 has been marked as a duplicate of this bug. ***
Comment 20 Peng Liu 2020-05-11 14:17:38 PDT
*** Bug 137311 has been marked as a duplicate of this bug. ***
Comment 21 Peng Liu 2020-05-11 15:27:28 PDT
Reopen this bug to fix some build failures.
Comment 22 Peng Liu 2020-05-11 15:34:28 PDT
Created attachment 399060 [details]
A follow-up patch to fix build failures
Comment 23 EWS 2020-05-11 16:13:07 PDT
Committed r261501: <https://trac.webkit.org/changeset/261501>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399060 [details].
Comment 24 Peng Liu 2020-05-18 23:41:03 PDT
*** Bug 200957 has been marked as a duplicate of this bug. ***