[Spice-devel] [PATCH spice-gtk 1/2] gstreamer: use custom playbin sink

[Snir Sheriber]

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