[Spice-devel] [PATCH spice-gtk 5/5] display-gst: Use Playbin for GStreamer 1.9.0 onwards
Victor Toso
victortoso at redhat.com
Tue May 9 12:31:51 UTC 2017
Hi,
On Mon, May 08, 2017 at 04:31:55AM -0400, Frediano Ziglio 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.
> >
>
> On the other side you'll get also gibberish till next I frame
> which can happens seconds later.
Well, yes. But it does seem very unlikely event or it would be a bug in
playbin/gstreamer. I did a small patch [0] to do some measurements. From
the moment we set the pipeline state to PLAYING, how long it took to
call the callback where we setup GstAppSrc; And how long it takes to get
the first frame queued.
[0] https://paste.fedoraproject.org/paste/PNV8hPFWLmjs0Maz5Ur9zV5M1UNdIGYhyRLivL9gydE=
In my system, with:
- H264:
(1)
[app_source_setup] 0.7510 ms
[queue_frame] 44.0890 ms
[queue_frame] 109.6790 ms
(2)
[app_source_setup] 2.6540 ms
[queue_frame] 202.2910 ms
[queue_frame] 445.2000 ms
(3)
[app_source_setup] 1.6250 ms
[queue_frame] 342.5000 ms
[queue_frame] 443.7670 ms
- VP8:
(1)
[app_source_setup] 1.1740 ms
[queue_frame] 464.2960 ms
[queue_frame] 647.1510 ms
(2)
[app_source_setup] 1.1080 ms
[queue_frame] 388.3100 ms
[queue_frame] 554.2010 ms
>
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Signed-off-by: Victor Toso <me at victortoso.com>
> > ---
> > src/channel-display-gst.c | 88
> > +++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 86 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 2a20763..2b14745 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;
> > @@ -256,10 +259,13 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink,
> > gpointer video_decoder)
> > }
> > l = l->next;
> > }
> > +#if !GST_CHECK_VERSION(1,9,0)
> > + /* We down own this sample when using playbin, no need to unref it
> > */
>
> This looks a bug to me. gst_app_sink_pull_sample documentation state that
> you should call gst_sample_unref. Also note that if the frame is expected
> sample is attached to a SpiceFrame and later unreferenced in free_frame.
> So some samples will get unreferenced and others not.
You are right. I'll try to understand the issue that I had here
(crash).
>
> > if (!l) {
> > spice_warning("got an unexpected decoded buffer!");
> > gst_sample_unref(sample);
> > }
> > +#endif
> >
> > g_mutex_unlock(&decoder->queues_mutex);
> > schedule_frame(decoder);
> > @@ -276,8 +282,11 @@ static void free_pipeline(SpiceGstDecoder *decoder)
> > }
> >
> > gst_element_set_state(decoder->pipeline, GST_STATE_NULL);
> > +#if !GST_CHECK_VERSION(1,9,0)
> > + /* Owned by playbin on 1.9.0 onwards */
> > gst_object_unref(decoder->appsrc);
> > gst_object_unref(decoder->appsink);
>
> So we have pointer which we don't own, IMHO better to use g_object_ref.
Yes, I agree that _ref is better. Maybe even better would be to do
g_object_add_weak_pointer() on decoder->appsrc/appsink as they should
hold no meaning if the pipeline (playbin) does not exist anymore.
> > +#endif
> > gst_object_unref(decoder->pipeline);
> > gst_object_unref(decoder->clock);
> > decoder->pipeline = NULL;
> > @@ -305,13 +314,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);
> > + gst_caps_unref(caps);
> > + decoder->appsrc = GST_APP_SRC(source);
>
> g_object_ref
>
> > +}
> > +#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", sink,
>
> How this change the ownership of sink? Do we still own it?
We don't. Playbin will use g_value_take_object() on it.
>
> > + NULL);
> > +
> > + g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup),
> > decoder);
>
> isn't that racy? playbin could in theory send the signal before
> we connect it.
I can't see how. We have created the playbin here but we haven't started
it yet. It should start after setting the pipeline to PLAYING below. (if
I'm not mistaken, it should trigger the signal on READY state).
>
> > +
> > + /* Disable audio in playbin */
> > + g_object_get(playbin, "flags", &flags, NULL);
> > + flags &= ~(1 << 1);
>
> No mnemonic? gst-inspect report the flags.
I did not find in the past. I just looked again and in the source code,
the only header that exports that is
gst-plugins-base/gst/playback/gstplay-enum.h which is not installed.
So, I'll file a bug to get some feedback on why but in the meantime I'll
create a #define for it.
> Why is needed to disable? To save memory?
It will prevent the playbin to do anything related to audio so, I would
say it is more process optimization then what will be allocated.
> Should we not disable other stuff like subtitles?
I think we can, why not.
>
> > + g_object_set(playbin, "flags", flags, NULL);
> > +
> > + decoder->appsrc = NULL;
>
> This potentially will override the value written by app_source_setup.
It will not as the signal could not be triggered before starting the
pipeline. This chunk is mostly for documentation, one that sees it knows
it is null till app_source_setup is called, but should be harmless to
remove it.
> Would be better to use some mutex+condition in order to wait
> app_source_setup to set this value. Kind of
>
> lock(mtx);
> if (!appsrc)
> wait_cond(cond);
> unlock(mtx);
>
> here and
>
> lock(mtx);
> appsrc = g_object_ref(source);
> signal(cond);
> unlock(mtx);
>
> in app_source_setup.
>
> > + decoder->appsink = GST_APP_SINK(sink);
>
> g_object_ref? Not clear if we still own sink pointer here.
We don't own it at this time. I'll fix this in v2.
>
> > + decoder->pipeline = playbin;
> > +#else
> > + gchar *desc;
> > + GError *err = NULL;
> >
> > /* - We schedule the frame display ourselves so set sync=false on
> > appsink
> > * so the pipeline decodes them as fast as possible. This will also
> > @@ -337,6 +412,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
> >
> > decoder->appsrc =
> > GST_APP_SRC(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "src"));
> > decoder->appsink =
> > GST_APP_SINK(gst_bin_get_by_name(GST_BIN(decoder->pipeline), "sink"));
> > +#endif
> >
> > appsink_cbs.new_sample = new_sample;
> > gst_app_sink_set_callbacks(decoder->appsink, &appsink_cbs, decoder,
> > NULL);
> > @@ -447,6 +523,14 @@ static gboolean
> > spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> > return FALSE;
> > }
> >
> > +#if GST_CHECK_VERSION(1,9,0)
> > + if (decoder->appsrc == NULL) {
> > + spice_warning("Error: Playbin has not yet initialized the Appsrc
> > element");
> > + stream_dropped_frame_on_playback(decoder->base.stream);
> > + return TRUE;
> > + }
> > +#endif
> > +
>
> I'd find a way to remove this
We can remove it by using decodebin. IMHO this ain't too bad and I would
rather keep this chunk and also playbin.
>
> > /* ref() the frame_msg for the buffer */
> > spice_msg_in_ref(frame_msg);
> > GstBuffer *buffer =
> > gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
>
> Frediano
Thanks for the review!
toso
-------------- 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/20170509/3cb884af/attachment.sig>
More information about the Spice-devel
mailing list