Bug 153641 – [GStreamer] Use FakeSink to get a decoded texture from a pipeline

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gwang Yoon Hwang]

[GStreamer] Use FakeSink to get a decoded texture from a pipeline

[Gwang Yoon Hwang]

Created attachment 270183 [details]
Patch

[ChangSeok Oh]

Interesting. Quick question : What version of gst is required to apply your patch?

[Gwang Yoon Hwang]

(In reply to comment #2)
> Interesting. Quick question : What version of gst is required to apply your
> patch?

I didn't test it with 1.5.x but from the 1.6.x it should work.

[Philippe Normand]

Comment on attachment 270183 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Using GstGLImageSink to use GStreamerGL brings a lot of overheads such as a

Relying on GstGLImageSink...   .... such as window handling

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:722
> +        GstElement* upload = gst_element_factory_make("glupload", nullptr);
> +        GstElement* colorconvert = gst_element_factory_make("glcolorconvert", nullptr);

These 2 might not be available at runtime if gst-plugins-bad isn't installed. So you need to check the return value of gst_element_factory_make()

[Michael Catanzaro]

By the way, since ENABLE_GSTREAMER_GL is an experimental feature, it's totally fine to depend on whatever new version of GStreamer you want.

[Philippe Normand]

Comment on attachment 270183 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:742
> +            gst_object_unref(videoSink);

A WARN_MEDIA_MESSAGE() (or LOG) would be good here I think.

[Michael Catanzaro]

Comment on attachment 270183 [details]
Patch

Please respond to Philippe's review comments :)

[Gwang Yoon Hwang]

Created attachment 272848 [details]
Patch

[Gwang Yoon Hwang]

Comment on attachment 270183 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Using GstGLImageSink to use GStreamerGL brings a lot of overheads such as a
> 
> Relying on GstGLImageSink...   .... such as window handling

Thanks! I updated it. :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:722
>> +        GstElement* colorconvert = gst_element_factory_make("glcolorconvert", nullptr);
> 
> These 2 might not be available at runtime if gst-plugins-bad isn't installed. So you need to check the return value of gst_element_factory_make()

I split this code blocks into new method which is dedicated to create a videoSink using GstGL and FakeSink.
And added checks and logs as your comment.
It looks much cleaner for me. :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:742
>> +            gst_object_unref(videoSink);
> 
> A WARN_MEDIA_MESSAGE() (or LOG) would be good here I think.

Ditto.

[Philippe Normand]

Yoon, do you remember exactly what are the glimagesink bottlenecks we're hitting?

[Gwang Yoon Hwang]

(In reply to comment #10)
> Yoon, do you remember exactly what are the glimagesink bottlenecks we're
> hitting?

Because we are using our own compositor, most of logics inside of glimagesink is not suitable for our use cases.

1. glimagesink uses its window backend to schedule the redrawing the video frame, but it is efficient to use our compositor's scheduler directly.
2. glimagesink holds two texture to switch the video frame, but it will be done by our platform layer proxies. ( So we will hold almost 3 textures.)
3. glimagesink tried to create its own window when we create/delete the video element rapidly. (If I remember correctly about this scenario). And It looks a bit hard to fix at that time.

[Gwang Yoon Hwang]

Created attachment 279204 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 279232 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #13)
> Created attachment 279232 [details]
> Patch

Bump the gst-gl version to 1.8.0

[Philippe Normand]

Comment on attachment 279232 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:94
> +#define GST_WEBKIT_VIDEO_FORMATS "{ RGBA }" \
> +
> +#define GST_WEBKIT_VIDEO_CAPS \
> +    "video/x-raw(" GST_CAPS_FEATURE_MEMORY_GL_MEMORY "), "              \
> +    "format = (string) " GST_WEBKIT_VIDEO_FORMATS ", "                  \
> +    "width = " GST_VIDEO_SIZE_RANGE ", "                                \
> +    "height = " GST_VIDEO_SIZE_RANGE ", "                               \
> +    "framerate = " GST_VIDEO_FPS_RANGE

This can be moved to the place where the caps are created and no need for a define.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:724
> +    GstElement* videoSink = gst_bin_new("gstglsinkbin");

Please rename this bin to avoid confusions with glimagesink internals.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:770
> +    m_videoSink = createVideoSinkGL();

Shouldn't we also set videoSink ?

[Gwang Yoon Hwang]

Comment on attachment 279232 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:94
>> +    "framerate = " GST_VIDEO_FPS_RANGE
> 
> This can be moved to the place where the caps are created and no need for a define.

Okay I'll do it.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:724
>> +    GstElement* videoSink = gst_bin_new("gstglsinkbin");
> 
> Please rename this bin to avoid confusions with glimagesink internals.

Do you have any recommendation?
I'm thinking to use "webkitsinkbin" as a name.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:770
>> +    m_videoSink = createVideoSinkGL();
> 
> Shouldn't we also set videoSink ?

We don't have to. videoSink will be a m_videoSink or m_fpsSink (if we have fpsSink)

[Philippe Normand]

Comment on attachment 279232 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:724
>>> +    GstElement* videoSink = gst_bin_new("gstglsinkbin");
>> 
>> Please rename this bin to avoid confusions with glimagesink internals.
> 
> Do you have any recommendation?
> I'm thinking to use "webkitsinkbin" as a name.

The name doesn't seem strictly needed, so use nullptr.

[Philippe Normand]

Comment on attachment 279232 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:724
>>>> +    GstElement* videoSink = gst_bin_new("gstglsinkbin");
>>> 
>>> Please rename this bin to avoid confusions with glimagesink internals.
>> 
>> Do you have any recommendation?
>> I'm thinking to use "webkitsinkbin" as a name.
> 
> The name doesn't seem strictly needed, so use nullptr.

Ah well, having a name is nice though when dumping the pipeline graph :) webkitsinkbin sgtm

[Gwang Yoon Hwang]

Created attachment 279244 [details]
Patch

[Philippe Normand]

Comment on attachment 279244 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:735
> +    GRefPtr<GstCaps> caps = adoptGRef(gst_caps_from_string("video/x-raw(" GST_CAPS_FEATURE_MEMORY_GL_MEMORY ")," "format = (string) { RGBA }"));

nit: ")," "format..." -> "), format...."

[Gwang Yoon Hwang]

Created attachment 279246 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279246 [details]
Patch

Clearing flags on attachment: 279246

Committed r201076: <http://trac.webkit.org/changeset/201076>

[Michael Catanzaro]

RESOLVED -> FIXED?