Bug 212191 - Fix issues of the Picture-in-Picture API under stress tests
Summary: Fix issues of the Picture-in-Picture API under stress tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-21 00:35 PDT by Peng Liu
Modified: 2020-05-21 17:20 PDT (History)
12 users (show)

See Also:


Attachments
Patch (42.08 KB, patch)
2020-05-21 01:04 PDT, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (42.45 KB, patch)
2020-05-21 16:02 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-21 00:35:07 PDT
Fix issues of the Picture-in-Picture API under stress test
Comment 1 Peng Liu 2020-05-21 00:37:09 PDT
Also, we need to refactor the layout tests for the Picture-in-Picture API with the Mock VideoPresentation Mode.
Comment 2 Peng Liu 2020-05-21 01:04:24 PDT
Created attachment 399941 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2020-05-21 01:05:01 PDT
<rdar://problem/63483308>
Comment 4 Eric Carlson 2020-05-21 10:01:30 PDT
Comment on attachment 399941 [details]
Patch

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

> Source/WebCore/html/HTMLVideoElement.cpp:458
> +static inline HTMLVideoElement::VideoPresentationMode toPresentationMode(HTMLMediaElementEnums::VideoFullscreenMode mode)
> +{
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard)
> +        return HTMLVideoElement::VideoPresentationMode::Fullscreen;
> +
> +    if (mode & HTMLMediaElementEnums::VideoFullscreenModePictureInPicture)
> +        return HTMLVideoElement::VideoPresentationMode::PictureInPicture;
> +
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModeNone)
> +        return HTMLVideoElement::VideoPresentationMode::Inline;
> +
> +    ASSERT_NOT_REACHED();
> +    return HTMLVideoElement::VideoPresentationMode::Inline;
> +}

Do we need both enums? VideoFullscreenMode is a bitfield, but it doesn't look like it needs to be so it would be nice to just use VideoPresentationMode if possible. 

If that works, it is likely to be a big change so another patch is fine.

> Source/WebCore/html/HTMLVideoElement.cpp:472
> +#if ENABLE(PICTURE_IN_PICTURE_API)

Do we still need ENABLE_PICTURE_IN_PICTURE_API?

> Source/WebCore/html/HTMLVideoElement.cpp:474
> +                m_exitingPictureInPicture = true;

Since you can't be entering and exiting PiP at the same time (I hope!), can you replace m_enteringPictureInPicture and m_exitingPictureInPicture with a single enum?
Comment 5 Peng Liu 2020-05-21 14:16:53 PDT
Comment on attachment 399941 [details]
Patch

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

>> Source/WebCore/html/HTMLVideoElement.cpp:458
>> +}
> 
> Do we need both enums? VideoFullscreenMode is a bitfield, but it doesn't look like it needs to be so it would be nice to just use VideoPresentationMode if possible. 
> 
> If that works, it is likely to be a big change so another patch is fine.

Filed a bug webkit.org/b/212229 for it.

>> Source/WebCore/html/HTMLVideoElement.cpp:472
>> +#if ENABLE(PICTURE_IN_PICTURE_API)
> 
> Do we still need ENABLE_PICTURE_IN_PICTURE_API?

No, the change is for the Picture-in-Picture feature in general, not only for the API. So I will remove it.

>> Source/WebCore/html/HTMLVideoElement.cpp:474
>> +                m_exitingPictureInPicture = true;
> 
> Since you can't be entering and exiting PiP at the same time (I hope!), can you replace m_enteringPictureInPicture and m_exitingPictureInPicture with a single enum?

Make sense! Will fix it.
Comment 6 Peng Liu 2020-05-21 16:02:46 PDT
Created attachment 399991 [details]
Patch for landing
Comment 7 EWS 2020-05-21 17:20:36 PDT
Committed r262038: <https://trac.webkit.org/changeset/262038>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399991 [details].