Bug 232155 – [Cocoa] Fairplay encrypted video fails to play when loaded in a display:none element

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Fairplay encrypted video can never play if it is loaded in a display:none element

[Eric Carlson]

rdar://83419159

[Eric Carlson]

Created attachment 442177 [details]
Patch

[Jer Noble]

Comment on attachment 442177 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:112
> +    if (!haveBeenAskedToPaint() || (supportsAcceleratedRendering() && m_player->renderingCanBeAccelerated()))
> +        return MediaRenderingMode::MediaRenderingToLayer;

So this will just cause us to always create an AVPlayerLayer immediately after the AVAssetStatus moves to > Unknown.  Is that correct?

[Peng Liu]

Comment on attachment 442177 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:637
> +    if (!m_avPlayer || m_videoLayer)

Just curious. Is it possible that we need a video layer with a different size (or content scale) than the existing video layer.
Before this change, this function will always create a new video layer and set some parameters (from the player).

[Eric Carlson]

Comment on attachment 442177 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:637
>> +    if (!m_avPlayer || m_videoLayer)
> 
> Just curious. Is it possible that we need a video layer with a different size (or content scale) than the existing video layer.
> Before this change, this function will always create a new video layer and set some parameters (from the player).

No, this function is only called when `m_videoLayer` is NULL so this change isn't necessary.

[Eric Carlson]

Created attachment 442211 [details]
Patch

[Jer Noble]

Comment on attachment 442211 [details]
Patch

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

r=me with nit

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cpp:112
>  
> -    if (supportsAcceleratedRendering() && m_player->renderingCanBeAccelerated())
> -        return MediaRenderingToLayer;
> +    if ((m_readyState >= MediaPlayer::ReadyState::HaveMetadata && !haveBeenAskedToPaint()) || (supportsAcceleratedRendering() && m_player->renderingCanBeAccelerated()))
> +        return MediaRenderingMode::MediaRenderingToLayer;

Nit: I might break this out into two statements for readability, and a smaller diff:

if (supportsAcceleratedRendering() && m_player->renderingCanBeAccelerated())
    return MediaRenderingToLayer;

if (m_readyState >= MediaPlayer::ReadyState::HaveMetadata && !haveBeenAskedToPaint())
    return MediaRenderingToLayer;

[Eric Carlson]

Created attachment 442219 [details]
Patch

[Eric Carlson]

Created attachment 442221 [details]
Patch

[EWS]

Committed r284737 (243446@main): <https://commits.webkit.org/243446@main>

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

[Simon Fraser (smfr)]

Comment on attachment 442221 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.h:160
> +    enum class MediaRenderingMode : uint8_t {
> +        MediaRenderingNone,
> +        MediaRenderingToContext,
> +        MediaRenderingToLayer
> +    };

Remove "MediaRendering" from the values.