Bug 206289 – [GStreamer][WPE] Client-side video rendering support

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

Using the WPEBackend-FDO video-plane-display-dmabuf protocol, to be shipped in 1.6.

[Philippe Normand]

Created attachment 387790 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 387790 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:259
> +        wpe_video_plane_display_dmabuf_source_update(videoPlaneDisplayDmaBufSource, m_dmabufFD, rect.x(), rect.y(), m_size.width(), m_size.height(), m_dmabufStride, [](void* data) {
> +            gst_buffer_unref(GST_BUFFER_CAST(data));
> +        }, gst_buffer_ref(m_buffer.get()));

Is this lambda a C callback? If not, then it would be nice to capture the smart pointer (by copy!) inside the lambda.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:487
> +#if USE(WPE_VIDEO_PLANE_DISPLAY_DMABUF)
> +    if (m_wpeVideoPlaneDisplayDmaBuf)
> +        wpe_video_plane_display_dmabuf_source_destroy(m_wpeVideoPlaneDisplayDmaBuf);
> +#endif
> +

I think we can use GUniquePtr by defining this as deleter in GUniquePtrGStreamer.h, can't we?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3146
> +    GStreamerDMABufHolePunchClient(std::unique_ptr<GstVideoFrameHolder> frameHolder, struct wpe_video_plane_display_dmabuf_source* videoPlaneDisplayDmaBufSource)

I think we should be taking a std::unique_ptr<GstVideoFrameHolder>&& frameHolder, specially with the way you call this constructor.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3181
> +            proxy.pushNextBuffer(WTFMove(layerBuffer));

And from what I see, TextureMapperPlatformLayerProxy::pushNextBuffer lacks a && in the parameter as well. Either we add it or we are not taking advantage of all the WTFMove calling it.

[Philippe Normand]

Comment on attachment 387790 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:259
>> +        }, gst_buffer_ref(m_buffer.get()));
> 
> Is this lambda a C callback? If not, then it would be nice to capture the smart pointer (by copy!) inside the lambda.

I think it's what you call a C callback? https://github.com/Igalia/WPEBackend-fdo/blob/master/include/wpe-fdo/extensions/video-plane-display-dmabuf.h#L66

[Philippe Normand]

Created attachment 387904 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #2)
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3181
> > +            proxy.pushNextBuffer(WTFMove(layerBuffer));
> 
> And from what I see, TextureMapperPlatformLayerProxy::pushNextBuffer lacks a
> && in the parameter as well. Either we add it or we are not taking advantage
> of all the WTFMove calling it.

Is this coming in a follow up or did you forget?

[Philippe Normand]

Committed r254682: <https://trac.webkit.org/changeset/254682>

[Radar WebKit Bug Importer]

<rdar://problem/58644258>

[Andrey Lushnikov]

Hi Philippe!

This broke `//Tools/Scripts/update-webkitwpe-libs` script - the cmake config is complaining for libwpe. 

I filed the bug upstream to libwpe: https://github.com/WebPlatformForEmbedded/libwpe/issues/63

Meanwhile, is there anything we can do about this change to fix it?

Thanks,
Andrey

[Zan Dobersek]

(In reply to Andrey Lushnikov from comment #8)
> This broke `//Tools/Scripts/update-webkitwpe-libs` script - the cmake config
> is complaining for libwpe. 
> 
> I filed the bug upstream to libwpe:
> https://github.com/WebPlatformForEmbedded/libwpe/issues/63
> 

I have a fix for this. I'll push it into libwpe and then update the jhbuild tag.

[Zan Dobersek]

(In reply to Zan Dobersek from comment #9)
> (In reply to Andrey Lushnikov from comment #8)
> > This broke `//Tools/Scripts/update-webkitwpe-libs` script - the cmake config
> > is complaining for libwpe. 
> > 
> > I filed the bug upstream to libwpe:
> > https://github.com/WebPlatformForEmbedded/libwpe/issues/63
> > 
> 
> I have a fix for this. I'll push it into libwpe and then update the jhbuild
> tag.

Done in r255062.
http://trac.webkit.org/changeset/255062/webkit