[Spice-devel] [PATCH spice-gtk v3 6/6] display-gst: Use Playbin for GStreamer 1.9.0 onwards
Christophe Fergeau
cfergeau at redhat.com
Tue Jun 6 15:57:13 UTC 2017
On Tue, May 16, 2017 at 04:48:18PM +0200, Victor Toso wrote:
> From: Victor Toso <me at victortoso.com>
>
> The Playbin can provide the full pipeline which reduces the
> overall maintenance in the code as we don't need to track which
> decoder can work with our stream type.
>
> We need to maintain the GstCaps per SPICE_VIDEO_CODEC_TYPE in order to
> tell Playbin the type of data we expect. This much should be covered
> by spice-protocol and very likely we will need to extend it in the
> future to allow more settings that might not possible to verify at
> runtime.
>
> This patch keeps previous code for compatibility reasons.
>
> Note that we have to wait Playbin to emit "source-setup" in order to
> configure GstAppSrc with the capabilities of input stream. If in the
> unlikely event of frames arriving while GstAppSrc is not setup, we
> will drop those frames.
>
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Signed-off-by: Victor Toso <me at victortoso.com>
> ---
> src/channel-display-gst.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index e675062..a359696 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -50,6 +50,9 @@ typedef struct SpiceGstDecoder {
> guint timer_id;
> } SpiceGstDecoder;
>
> +/* FIXME: With gstreamer version 1.9.0 and higher, we are using playbin to
> + * create the pipeline for us and for that reason we don't need to keep track of
> + * decoder's name anymore. */
> static struct {
> const gchar *dec_name;
> const gchar *dec_caps;
> @@ -83,6 +86,21 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> #define VALID_VIDEO_CODEC_TYPE(codec) \
> (codec > 0 && codec < G_N_ELEMENTS(gst_opts))
>
> +typedef enum {
> + GST_PLAY_FLAG_VIDEO = (1 << 0),
> + GST_PLAY_FLAG_AUDIO = (1 << 1),
> + GST_PLAY_FLAG_TEXT = (1 << 2),
> + GST_PLAY_FLAG_VIS = (1 << 3),
> + GST_PLAY_FLAG_SOFT_VOLUME = (1 << 4),
> + GST_PLAY_FLAG_NATIVE_AUDIO = (1 << 5),
> + GST_PLAY_FLAG_NATIVE_VIDEO = (1 << 6),
> + GST_PLAY_FLAG_DOWNLOAD = (1 << 7),
> + GST_PLAY_FLAG_BUFFERING = (1 << 8),
> + GST_PLAY_FLAG_DEINTERLACE = (1 << 9),
> + GST_PLAY_FLAG_SOFT_COLORBALANCE = (1 << 10),
> + GST_PLAY_FLAG_FORCE_FILTERS = (1 << 11),
> +} GstPlayFlags;
> +
> /* ---------- SpiceGstFrame ---------- */
>
> typedef struct SpiceGstFrame {
> @@ -298,13 +316,79 @@ static gboolean handle_pipeline_message(GstBus *bus, GstMessage *msg, gpointer v
> return TRUE;
> }
>
> +#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);
This is duplicated the comment and a part of
desc = g_strdup_printf("appsrc name=src is-live=true format=time max-bytes=0 block=true "
"caps=%s ! %s ! videoconvert ! appsink name=sink "
"caps=video/x-raw,format=BGRx sync=false drop=false",
gst_opts[decoder->base.codec_type].dec_caps,
gst_opts[decoder->base.codec_type].dec_name);
from create_pipeline(), dunno if a "set_appsrc_props()" helper would make a lot
of sense here (it would be very small).
> + 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_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup), decoder);
> +
> + g_object_set(playbin,
> + "uri", "appsrc://",
> + "video-sink", gst_object_ref(sink),
> + NULL);
Aren't you leaking 'sink' here?
I would have written this as:
sink = gst_element_factory_make("appsink", "sink");
g_object_set(playbin,
"uri", "appsrc://",
"video-sink", sink,
NULL);
decoder->appsink = GST_APP_SINK(sink);
(ie g_object_set() / playbin set_property vfunc should take care of any extra
reference they need themselves)
Apart from this, and the comments on the previous version, looks good to me (did not test it).
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170606/304e876a/attachment-0001.sig>
More information about the Spice-devel
mailing list