[Spice-devel] [PATCH spice-gtk v2 5/5] display-gst: Use Playbin for GStreamer 1.9.0 onwards

[Victor Toso]

Hi,

On Tue, Jun 06, 2017 at 05:47:25PM +0200, Christophe Fergeau wrote:
> On Fri, May 12, 2017 at 09:25:44AM -0400, Frediano Ziglio wrote:
> > > +#if GST_CHECK_VERSION(1,9,0)
> > > +static void app_source_setup(GstElement *pipeline, GstElement *source,
> > > SpiceGstDecoder *decoder)
> > > +{
> > > +    GstCaps *caps;
> > > +
> > > +    /* - We schedule the frame display ourselves so set sync=false on
> > > appsink
> > > +     *   so the pipeline decodes them as fast as possible. This will also
> > > +     *   minimize the risk of frames getting lost when we rebuild the
> > > +     *   pipeline.
> > > +     * - Set max-bytes=0 on appsrc so it does not drop frames that may be
> > > +     *   needed by those that follow.
> > > +     */
> > > +    caps =
> > > gst_caps_from_string(gst_opts[decoder->base.codec_type].dec_caps);
> > > +    g_object_set(source,
> > > +                 "caps", caps,
> > > +                 "is-live", TRUE,
> > > +                 "format", GST_FORMAT_TIME,
> > > +                 "max-bytes", 0,
> > > +                 "block", TRUE,
> > > +                 NULL);
> > > +    gst_caps_unref(caps);
> > > +    decoder->appsrc = GST_APP_SRC(gst_object_ref(source));
> > > +}
> > > +#endif
> > > +
> > >  static gboolean create_pipeline(SpiceGstDecoder *decoder)
> > >  {
> > > -    gchar *desc;
> > >      GstAppSinkCallbacks appsink_cbs = { NULL };
> > > -    GError *err = NULL;
> > >      GstBus *bus;
> > > +#if GST_CHECK_VERSION(1,9,0)
> > > +    GstElement *playbin, *sink;
> > > +    gint flags;
> > > +    GstCaps *caps;
> > >  
> > > +    playbin = gst_element_factory_make("playbin", "playbin");
> > > +    if (playbin == NULL) {
> > > +        spice_warning("error upon creation of 'playbin' element");
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    sink = gst_element_factory_make("appsink", "sink");
> > > +    if (sink == NULL) {
> > > +        spice_warning("error upon creation of 'appsink' element");
> > > +        gst_object_unref(playbin);
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    caps = gst_caps_from_string("video/x-raw,format=BGRx");
> > > +    g_object_set(sink,
> > > +                 "caps", caps,
> > > +                 "sync", FALSE,
> > > +                 "drop", FALSE,
> > > +                 NULL);
> > > +    gst_caps_unref(caps);
> > > +
> > > +    g_object_set(playbin,
> > > +                 "uri", "appsrc://",
> > > +                 "video-sink", gst_object_ref(sink),
> > > +                 NULL);
> > > +
> > > +    g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
> > > decoder);
> > > +
> > 
> > I would still put this before setting the uri property.
> > Documentation state that "This signal is usually emitted from the context
> > of a GStreamer streaming thread." which looks like "we are free to change in
> > the future".
> 
> app_source_setup can run from GStreamer streaming thread and sets
> decoder->appsrc, which spice_gst_decoder_queue_frame() then reads and
> checks for NULL. Is spice_gst_decoder_queue_frame() running from the
> GStreamer streaming thread as well?

No. spice_gst_decoder_queue_frame() runs from coroutine context
actually, from either SPICE_MSG_DISPLAY_STREAM_DATA/_DATA_SIZED
messages.

Do you think we need a mutex here? (As drop the stream in case
decoder->appsrc is NULL, I don't think it is needed but I could be
wrong)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170628/e7c96e1d/attachment-0001.sig>