[GStreamer] Use FakeSink to get a decoded texture from a pipeline
Created attachment 270183 [details] Patch
Interesting. Quick question : What version of gst is required to apply your patch?
(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 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()
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 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 on attachment 270183 [details] Patch Please respond to Philippe's review comments :)
Created attachment 272848 [details] Patch
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.
Yoon, do you remember exactly what are the glimagesink bottlenecks we're hitting?
(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.
Created attachment 279204 [details] Patch
Created attachment 279232 [details] Patch
(In reply to comment #13) > Created attachment 279232 [details] > Patch Bump the gst-gl version to 1.8.0
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 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 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 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
Created attachment 279244 [details] Patch
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...."
Created attachment 279246 [details] Patch
Comment on attachment 279246 [details] Patch Clearing flags on attachment: 279246 Committed r201076: <http://trac.webkit.org/changeset/201076>
RESOLVED -> FIXED?