[spice-gtk,1/2] gstreamer: use custom playbin sink

Submitted by Snir Sheriber on Dec. 31, 2017, 3:46 p.m.

Details

Message ID 20171231154625.30010-2-ssheribe@redhat.com
State New
Headers show
Series "stream decoding using gstreamer" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Snir Sheriber Dec. 31, 2017, 3:46 p.m.
Use custom playbin sink instead of just appsink.
In order to allow playbin to choose decoders that requires
more complex pipelines (e.g. pipeline that requires color
space conversion & uses gl memory).
The new sink composed as such:
... ! autovideoconvert ! gldownload ! appsink
---
 src/channel-display-gst.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index f978602..af87fb5 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -38,6 +38,7 @@  typedef struct SpiceGstDecoder {
 
     GstAppSrc *appsrc;
     GstAppSink *appsink;
+    GstElement *sinkbin;
     GstElement *pipeline;
     GstClock *clock;
 
@@ -265,7 +266,7 @@  static void free_pipeline(SpiceGstDecoder *decoder)
 
     gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
     gst_object_unref(decoder->appsrc);
-    gst_object_unref(decoder->appsink);
+    gst_object_unref(decoder->sinkbin);
     gst_object_unref(decoder->pipeline);
     gst_object_unref(decoder->clock);
     decoder->pipeline = NULL;
@@ -346,7 +347,8 @@  static gboolean create_pipeline(SpiceGstDecoder *decoder)
     GstAppSinkCallbacks appsink_cbs = { NULL };
     GstBus *bus;
 #if GST_CHECK_VERSION(1,9,0)
-    GstElement *playbin, *sink;
+    GstElement *playbin, *sinkbin, *convert, *download, *appsink;
+    GstPad *pad, *ghost_pad;
     SpiceGstPlayFlags flags;
     GstCaps *caps;
 
@@ -356,26 +358,39 @@  static gboolean create_pipeline(SpiceGstDecoder *decoder)
         return FALSE;
     }
 
-    sink = gst_element_factory_make("appsink", "sink");
-    if (sink == NULL) {
-        spice_warning("error upon creation of 'appsink' element");
+    convert = gst_element_factory_make("autovideoconvert", "autovideoconvert");
+    download = gst_element_factory_make("gldownload", "gldownload");
+    appsink = gst_element_factory_make("appsink", "appsink");
+    if ( !appsink || !convert || !download ) {
+        spice_warning("error upon creation of one of the bin elements");
         gst_object_unref(playbin);
         return FALSE;
     }
 
     caps = gst_caps_from_string("video/x-raw,format=BGRx");
-    g_object_set(sink,
+    g_object_set(appsink,
                  "caps", caps,
                  "sync", FALSE,
                  "drop", FALSE,
                  NULL);
     gst_caps_unref(caps);
 
+    /* Create advanced playbin sink by binning the appsink, gldownload and
+     * autovideoconvert elements together */
+    sinkbin = gst_bin_new("sink_bin");
+    gst_bin_add_many (GST_BIN (sinkbin), convert, download, appsink, NULL);
+    gst_element_link_many(convert, download, appsink, NULL);
+    pad = gst_element_get_static_pad (convert, "sink");
+    ghost_pad = gst_ghost_pad_new ("sink", pad);
+    gst_pad_set_active (ghost_pad, TRUE);
+    gst_element_add_pad (sinkbin, ghost_pad);
+    gst_object_unref (pad);
+
     g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup), decoder);
 
     g_object_set(playbin,
                  "uri", "appsrc://",
-                 "video-sink", gst_object_ref(sink),
+                 "video-sink", gst_object_ref(sinkbin),
                  NULL);
 
     /* Disable audio in playbin */
@@ -384,7 +399,8 @@  static gboolean create_pipeline(SpiceGstDecoder *decoder)
     g_object_set(playbin, "flags", flags, NULL);
 
     g_warn_if_fail(decoder->appsrc == NULL);
-    decoder->appsink = GST_APP_SINK(sink);
+    decoder->appsink = GST_APP_SINK(appsink);
+    decoder->sinkbin = sinkbin;
     decoder->pipeline = playbin;
 #else
     gchar *desc;

Comments

On Sun, Dec 31, 2017 at 05:46:24PM +0200, Snir Sheriber wrote:
> Use custom playbin sink instead of just appsink.
> In order to allow playbin to choose decoders that requires
> more complex pipelines (e.g. pipeline that requires color
> space conversion & uses gl memory).

What is gstreamer take on that? Are these playbin limitations something
we have to live with? Are they considered bugs which might be fixed in
future versions?

> The new sink composed as such:
> ... ! autovideoconvert ! gldownload ! appsink

Is autovideoconvert just a passthrough in the non-hardware accelerated
scenario? Or is this going to add some additional processing of the
data?
I think appsink is needed so that the decoded stream can be used with
the canvas. Do you expect situations where we are going to hardware
decode some stream, and then do some compositing using the canvas? Or
are the streams we want to hardware decode always going to correspond to
the full display, with no composition to do? In the latter case, I would
say that we don't even want to use appsink, but just use glsink or
whatever is appropriate.

Christophe
> 
> Use custom playbin sink instead of just appsink.
> In order to allow playbin to choose decoders that requires
> more complex pipelines (e.g. pipeline that requires color
> space conversion & uses gl memory).
> The new sink composed as such:
> ... ! autovideoconvert ! gldownload ! appsink

Adding more plugins add more should add more contraints.
Why GStreamer is not able to choose the best combination?
What happens if the decoder is not able to provide gl texture?

> ---
>  src/channel-display-gst.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f978602..af87fb5 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -38,6 +38,7 @@ typedef struct SpiceGstDecoder {
>  
>      GstAppSrc *appsrc;
>      GstAppSink *appsink;
> +    GstElement *sinkbin;
>      GstElement *pipeline;
>      GstClock *clock;
>  
> @@ -265,7 +266,7 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>  
>      gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
>      gst_object_unref(decoder->appsrc);
> -    gst_object_unref(decoder->appsink);
> +    gst_object_unref(decoder->sinkbin);
>      gst_object_unref(decoder->pipeline);
>      gst_object_unref(decoder->clock);
>      decoder->pipeline = NULL;
> @@ -346,7 +347,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>      GstAppSinkCallbacks appsink_cbs = { NULL };
>      GstBus *bus;
>  #if GST_CHECK_VERSION(1,9,0)
> -    GstElement *playbin, *sink;
> +    GstElement *playbin, *sinkbin, *convert, *download, *appsink;
> +    GstPad *pad, *ghost_pad;
>      SpiceGstPlayFlags flags;
>      GstCaps *caps;
>  
> @@ -356,26 +358,39 @@ static gboolean create_pipeline(SpiceGstDecoder
> *decoder)
>          return FALSE;
>      }
>  
> -    sink = gst_element_factory_make("appsink", "sink");
> -    if (sink == NULL) {
> -        spice_warning("error upon creation of 'appsink' element");
> +    convert = gst_element_factory_make("autovideoconvert",
> "autovideoconvert");
> +    download = gst_element_factory_make("gldownload", "gldownload");
> +    appsink = gst_element_factory_make("appsink", "appsink");
> +    if ( !appsink || !convert || !download ) {
> +        spice_warning("error upon creation of one of the bin elements");
>          gst_object_unref(playbin);

This is potentially leaking, won't be better to use string based pipeline
instead of manually build like this?

>          return FALSE;
>      }



>  
>      caps = gst_caps_from_string("video/x-raw,format=BGRx");
> -    g_object_set(sink,
> +    g_object_set(appsink,
>                   "caps", caps,
>                   "sync", FALSE,
>                   "drop", FALSE,
>                   NULL);
>      gst_caps_unref(caps);
>  
> +    /* Create advanced playbin sink by binning the appsink, gldownload and

binning? Do you mean "binding"?

> +     * autovideoconvert elements together */
> +    sinkbin = gst_bin_new("sink_bin");
> +    gst_bin_add_many (GST_BIN (sinkbin), convert, download, appsink, NULL);
> +    gst_element_link_many(convert, download, appsink, NULL);
> +    pad = gst_element_get_static_pad (convert, "sink");
> +    ghost_pad = gst_ghost_pad_new ("sink", pad);
> +    gst_pad_set_active (ghost_pad, TRUE);
> +    gst_element_add_pad (sinkbin, ghost_pad);
> +    gst_object_unref (pad);

style

> +
>      g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
>      decoder);
>  
>      g_object_set(playbin,
>                   "uri", "appsrc://",
> -                 "video-sink", gst_object_ref(sink),
> +                 "video-sink", gst_object_ref(sinkbin),
>                   NULL);
>  
>      /* Disable audio in playbin */
> @@ -384,7 +399,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>      g_object_set(playbin, "flags", flags, NULL);
>  
>      g_warn_if_fail(decoder->appsrc == NULL);
> -    decoder->appsink = GST_APP_SINK(sink);
> +    decoder->appsink = GST_APP_SINK(appsink);
> +    decoder->sinkbin = sinkbin;
>      decoder->pipeline = playbin;
>  #else
>      gchar *desc;

Frediano
On Mon, Jan 08, 2018 at 04:47:26AM -0500, Frediano Ziglio wrote:
> > 
> > Use custom playbin sink instead of just appsink.
> > In order to allow playbin to choose decoders that requires
> > more complex pipelines (e.g. pipeline that requires color
> > space conversion & uses gl memory).
> > The new sink composed as such:
> > ... ! autovideoconvert ! gldownload ! appsink
> 
> Adding more plugins add more should add more contraints.
> Why GStreamer is not able to choose the best combination?
> What happens if the decoder is not able to provide gl texture?

I would think this falls back to passing through the buffer:
https://github.com/GStreamer/gst-plugins-bad/blob/1.12/ext/gl/gstgldownloadelement.c#L158

Christophe
Hi,


On 01/04/2018 11:26 AM, Christophe Fergeau wrote:
> On Sun, Dec 31, 2017 at 05:46:24PM +0200, Snir Sheriber wrote:
>> Use custom playbin sink instead of just appsink.
>> In order to allow playbin to choose decoders that requires
>> more complex pipelines (e.g. pipeline that requires color
>> space conversion & uses gl memory).
> What is gstreamer take on that? Are these playbin limitations something
> we have to live with? Are they considered bugs which might be fixed in
> future versions?

I haven't seen bugs related, I'll have another look (i tried to
ask on their irc about that but no reply yet..). but it does
sound plausible that playbin would be able to do that.

>
>> The new sink composed as such:
>> ... ! autovideoconvert ! gldownload ! appsink
> Is autovideoconvert just a passthrough in the non-hardware accelerated
> scenario? Or is this going to add some additional processing of the
> data?

hw decoders are often limited with their output colorspace
and hence it's likely that the element that is using it will be
also limited, for that reason color conversion element is also
likely to be needed.

> I think appsink is needed so that the decoded stream can be used with
> the canvas. Do you expect situations where we are going to hardware
> decode some stream, and then do some compositing using the canvas? Or
> are the streams we want to hardware decode always going to correspond to
> the full display, with no composition to do? In the latter case, I would
> say that we don't even want to use appsink, but just use glsink or
> whatever is appropriate.

Yes, if it's full screen case avoiding the canvas will be much
better.. using glsink or something similar is the next step.

>
> Christophe
Hi


On 01/08/2018 11:47 AM, Frediano Ziglio wrote:
>> Use custom playbin sink instead of just appsink.
>> In order to allow playbin to choose decoders that requires
>> more complex pipelines (e.g. pipeline that requires color
>> space conversion & uses gl memory).
>> The new sink composed as such:
>> ... ! autovideoconvert ! gldownload ! appsink
> Adding more plugins add more should add more contraints.
> Why GStreamer is not able to choose the best combination?

I saw it mentioned somewhere, can't find it now, playbin is
just not able to compose complex pipelines so custom sink
is required.

> What happens if the decoder is not able to provide gl texture?

Yep, as teuf mentioned , should be transparent

>
>> ---
>>   src/channel-display-gst.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
>> index f978602..af87fb5 100644
>> --- a/src/channel-display-gst.c
>> +++ b/src/channel-display-gst.c
>> @@ -38,6 +38,7 @@ typedef struct SpiceGstDecoder {
>>   
>>       GstAppSrc *appsrc;
>>       GstAppSink *appsink;
>> +    GstElement *sinkbin;
>>       GstElement *pipeline;
>>       GstClock *clock;
>>   
>> @@ -265,7 +266,7 @@ static void free_pipeline(SpiceGstDecoder *decoder)
>>   
>>       gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
>>       gst_object_unref(decoder->appsrc);
>> -    gst_object_unref(decoder->appsink);
>> +    gst_object_unref(decoder->sinkbin);
>>       gst_object_unref(decoder->pipeline);
>>       gst_object_unref(decoder->clock);
>>       decoder->pipeline = NULL;
>> @@ -346,7 +347,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>>       GstAppSinkCallbacks appsink_cbs = { NULL };
>>       GstBus *bus;
>>   #if GST_CHECK_VERSION(1,9,0)
>> -    GstElement *playbin, *sink;
>> +    GstElement *playbin, *sinkbin, *convert, *download, *appsink;
>> +    GstPad *pad, *ghost_pad;
>>       SpiceGstPlayFlags flags;
>>       GstCaps *caps;
>>   
>> @@ -356,26 +358,39 @@ static gboolean create_pipeline(SpiceGstDecoder
>> *decoder)
>>           return FALSE;
>>       }
>>   
>> -    sink = gst_element_factory_make("appsink", "sink");
>> -    if (sink == NULL) {
>> -        spice_warning("error upon creation of 'appsink' element");
>> +    convert = gst_element_factory_make("autovideoconvert",
>> "autovideoconvert");
>> +    download = gst_element_factory_make("gldownload", "gldownload");
>> +    appsink = gst_element_factory_make("appsink", "appsink");
>> +    if ( !appsink || !convert || !download ) {
>> +        spice_warning("error upon creation of one of the bin elements");
>>           gst_object_unref(playbin);
> This is potentially leaking, won't be better to use string based pipeline
> instead of manually build like this?
>
>>           return FALSE;
>>       }
>
>
>>   
>>       caps = gst_caps_from_string("video/x-raw,format=BGRx");
>> -    g_object_set(sink,
>> +    g_object_set(appsink,
>>                    "caps", caps,
>>                    "sync", FALSE,
>>                    "drop", FALSE,
>>                    NULL);
>>       gst_caps_unref(caps);
>>   
>> +    /* Create advanced playbin sink by binning the appsink, gldownload and
> binning? Do you mean "binding"?

I meant to binning- put in a bin (is correct to say it? )

>> +     * autovideoconvert elements together */
>> +    sinkbin = gst_bin_new("sink_bin");
>> +    gst_bin_add_many (GST_BIN (sinkbin), convert, download, appsink, NULL);
>> +    gst_element_link_many(convert, download, appsink, NULL);
>> +    pad = gst_element_get_static_pad (convert, "sink");
>> +    ghost_pad = gst_ghost_pad_new ("sink", pad);
>> +    gst_pad_set_active (ghost_pad, TRUE);
>> +    gst_element_add_pad (sinkbin, ghost_pad);
>> +    gst_object_unref (pad);
> style
>
>> +
>>       g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
>>       decoder);
>>   
>>       g_object_set(playbin,
>>                    "uri", "appsrc://",
>> -                 "video-sink", gst_object_ref(sink),
>> +                 "video-sink", gst_object_ref(sinkbin),
>>                    NULL);
>>   
>>       /* Disable audio in playbin */
>> @@ -384,7 +399,8 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>>       g_object_set(playbin, "flags", flags, NULL);
>>   
>>       g_warn_if_fail(decoder->appsrc == NULL);
>> -    decoder->appsink = GST_APP_SINK(sink);
>> +    decoder->appsink = GST_APP_SINK(appsink);
>> +    decoder->sinkbin = sinkbin;
>>       decoder->pipeline = playbin;
>>   #else
>>       gchar *desc;
> Frediano