Bug 153641 - [GStreamer] Use FakeSink to get a decoded texture from a pipeline
Summary: [GStreamer] Use FakeSink to get a decoded texture from a pipeline
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: Gwang Yoon Hwang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-28 21:52 PST by Gwang Yoon Hwang
Modified: 2016-05-18 08:18 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.45 KB, patch)
2016-01-28 21:54 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2016-03-04 03:22 PST, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2016-05-17 20:12 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (10.80 KB, patch)
2016-05-18 04:19 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2016-05-18 06:44 PDT, Gwang Yoon Hwang
pnormand: review+
Details | Formatted Diff | Diff
Patch (10.67 KB, patch)
2016-05-18 06:59 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gwang Yoon Hwang 2016-01-28 21:52:58 PST
[GStreamer] Use FakeSink to get a decoded texture from a pipeline
Comment 1 Gwang Yoon Hwang 2016-01-28 21:54:31 PST
Created attachment 270183 [details]
Patch
Comment 2 ChangSeok Oh 2016-01-28 22:02:58 PST
Interesting. Quick question : What version of gst is required to apply your patch?
Comment 3 Gwang Yoon Hwang 2016-01-28 22:07:25 PST
(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.
Comment 4 Philippe Normand 2016-01-28 23:39:12 PST
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()
Comment 5 Michael Catanzaro 2016-01-29 06:54:41 PST
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.
Comment 6 Philippe Normand 2016-02-01 07:27:37 PST
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.
Comment 7 Michael Catanzaro 2016-02-15 13:09:34 PST
Comment on attachment 270183 [details]
Patch

Please respond to Philippe's review comments :)
Comment 8 Gwang Yoon Hwang 2016-03-04 03:22:09 PST
Created attachment 272848 [details]
Patch
Comment 9 Gwang Yoon Hwang 2016-03-04 03:29:05 PST
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.
Comment 10 Philippe Normand 2016-05-14 08:18:07 PDT
Yoon, do you remember exactly what are the glimagesink bottlenecks we're hitting?
Comment 11 Gwang Yoon Hwang 2016-05-17 20:08:07 PDT
(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.
Comment 12 Gwang Yoon Hwang 2016-05-17 20:12:37 PDT
Created attachment 279204 [details]
Patch
Comment 13 Gwang Yoon Hwang 2016-05-18 04:19:36 PDT
Created attachment 279232 [details]
Patch
Comment 14 Gwang Yoon Hwang 2016-05-18 04:20:11 PDT
(In reply to comment #13)
> Created attachment 279232 [details]
> Patch

Bump the gst-gl version to 1.8.0
Comment 15 Philippe Normand 2016-05-18 04:27:48 PDT
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 ?
Comment 16 Gwang Yoon Hwang 2016-05-18 06:15:05 PDT
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)
Comment 17 Philippe Normand 2016-05-18 06:37:26 PDT
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.
Comment 18 Philippe Normand 2016-05-18 06:40:32 PDT
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
Comment 19 Gwang Yoon Hwang 2016-05-18 06:44:12 PDT
Created attachment 279244 [details]
Patch
Comment 20 Philippe Normand 2016-05-18 06:50:27 PDT
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...."
Comment 21 Gwang Yoon Hwang 2016-05-18 06:59:45 PDT
Created attachment 279246 [details]
Patch
Comment 22 WebKit Commit Bot 2016-05-18 07:29:43 PDT
Comment on attachment 279246 [details]
Patch

Clearing flags on attachment: 279246

Committed r201076: <http://trac.webkit.org/changeset/201076>
Comment 23 Michael Catanzaro 2016-05-18 08:18:24 PDT
RESOLVED -> FIXED?