Bug 207208 – [GStreamer] Client-side video rendering doesn't fallback to internal compositing

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

For instance when the EGLImage export fails, we should fallback to internal compositing (eg, no hole punching).

[Philippe Normand]

Created attachment 389674 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 389674 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3172
> +    auto internalCompositingOperation = [this](TextureMapperPlatformLayerProxy& proxy, std::unique_ptr<GstVideoFrameHolder> frameHolder) {

You're moving the frameholder when calling this, so I guess you should make this && as well.

Anyway, why do you create a lambda instead of a method considering that we are only capturing this?

[Zan Dobersek]

Comment on attachment 389674 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:218
> +        static std::once_flag onceFlag;
> +        std::call_once(onceFlag, [] {

Nit: I'd recommend using `s_onceFlag` as the name, the `s_` prefix more explicitly describing the storage nature.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:372
> +        return m_dmabufFD;

This should test for m_dmabufFD being >= 0, i.e. a valid fd. Related to this, m_dmabufFD should be initialized to -1 and in handoffVideoDmaBuf() reset to -1 after the hand-off and closure.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3203
> +            std::unique_ptr<GstVideoFrameHolder> frameHolder = makeUnique<GstVideoFrameHolder>(m_sample.get(), m_videoDecoderPlatform, m_textureMapperFlags, !m_isUsingFallbackVideoSink);
> +            if (frameHolder->hasDMABuf()) {
> +                std::unique_ptr<TextureMapperPlatformLayerBuffer> layerBuffer = makeUnique<TextureMapperPlatformLayerBuffer>(0, m_size, TextureMapperGL::ShouldNotBlend, GL_DONT_CARE);

Could use `auto object = makeUnique<>()` in both cases.

[Philippe Normand]

Comment on attachment 389674 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3172
>> +    auto internalCompositingOperation = [this](TextureMapperPlatformLayerProxy& proxy, std::unique_ptr<GstVideoFrameHolder> frameHolder) {
> 
> You're moving the frameholder when calling this, so I guess you should make this && as well.
> 
> Anyway, why do you create a lambda instead of a method considering that we are only capturing this?

I don't really see the point of adding a method for this. As the logic remains self-contained in pushTextureToCompositor() anyway. This new closure is an implementation detail of it.

[Philippe Normand]

Committed r255790: <https://trac.webkit.org/changeset/255790>

[Radar WebKit Bug Importer]

<rdar://problem/59182626>