[Spice-devel] [client] gstreamer: Fix the decoding time when not using GstVideoOverlay
Snir Sheriber
ssheribe at redhat.com
Wed Jun 12 11:54:05 UTC 2019
Hi,
On 6/11/19 9:42 PM, Francois Gouget wrote:
> schedule_frame() only pulls frames out of GStreamer's pipeline once all
> previous decoded frames have been displayed. Thus when the video delay
IIRC we used to pull when samples arrived but it was changed to this so
pending frames will be queued
inside gstreamer and let it do throttling (or something similar)
> is high a decoded frame may have to wait for several frame intervals
> before get_decoded_frame() looks at it and computes its decoding time.
>
> So attach decoded frame samples to the corresponding decoding_queue
> entry as soon as new_sample() is called, and compute the decoding time
> and associated statistics then. Similarly, match sink_event_probe()'s
> new buffer to a decoding_queue entry and update the statistics in the
> process. This ensures that there is no extra delay in either case.
>
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
> src/channel-display-gst.c | 183 ++++++++++++++++++++------------------
> 1 file changed, 96 insertions(+), 87 deletions(-)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 91ece0fa..fed73592 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -1,6 +1,6 @@
> /* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> /*
> - Copyright (C) 2015-2016 CodeWeavers, Inc
> + Copyright (C) 2015-2016, 2019 CodeWeavers, Inc
>
> This library is free software; you can redistribute it and/or
> modify it under the terms of the GNU Lesser General Public
> @@ -54,9 +54,9 @@ typedef struct SpiceGstDecoder {
>
> GMutex queues_mutex;
> GQueue *decoding_queue;
> + guint decoded_frames;
> SpiceGstFrame *display_frame;
> guint timer_id;
> - guint pending_samples;
> } SpiceGstDecoder;
>
> #define VALID_VIDEO_CODEC_TYPE(codec) \
> @@ -120,8 +120,6 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
> /* ---------- GStreamer pipeline ---------- */
>
> static void schedule_frame(SpiceGstDecoder *decoder);
> -static void fetch_pending_sample(SpiceGstDecoder *decoder);
> -static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buffer);
>
> RECORDER(frames_stats, 64, "Frames statistics");
>
> @@ -191,21 +189,26 @@ static void schedule_frame(SpiceGstDecoder *decoder)
> g_mutex_lock(&decoder->queues_mutex);
>
> while (!decoder->timer_id) {
> - while (decoder->display_frame == NULL && decoder->pending_samples) {
> - fetch_pending_sample(decoder);
> + GList *head = g_queue_peek_head_link(decoder->decoding_queue);
> + if (!head) {
> + /* No frame in the queue */
> + break;
> }
> -
> - SpiceGstFrame *gstframe = decoder->display_frame;
> - if (!gstframe) {
> + SpiceGstFrame *gstframe = head->data;
> + if (!gstframe->decoded_sample) {
> + /* This frame has not been decoded yet */
> break;
> }
> + g_queue_pop_head(decoder->decoding_queue);
> + decoder->display_frame = gstframe;
> + decoder->decoded_frames--;
>
> if (spice_mmtime_diff(gstframe->encoded_frame->mm_time, now) >= 0) {
> decoder->timer_id = g_timeout_add(gstframe->encoded_frame->mm_time - now,
> display_frame, decoder);
> - } else if (decoder->display_frame && !decoder->pending_samples) {
> - /* Still attempt to display the least out of date frame so the
> - * video is not completely frozen for an extended period of time.
> + } else if (decoder->display_frame && decoder->decoded_frames == 0) {
> + /* This is the least out of date frame. Display it since that's
> + * better than having frozen video for an extended period of time.
> */
> decoder->timer_id = g_timeout_add(0, display_frame, decoder);
> } else {
> @@ -221,12 +224,16 @@ static void schedule_frame(SpiceGstDecoder *decoder)
> g_mutex_unlock(&decoder->queues_mutex);
> }
>
> -/* Get the decoded frame relative to buffer or NULL if not found.
> - * Dequeue the frame from decoding_queue and return it, caller
> - * is responsible to free the pointer.
> +/* Returns the decoding queue entry that matches the specified GStreamer buffer.
> + *
> + * The entry is identified based on the buffer timestamp. However sometimes
> + * GStreamer returns the same buffer twice (and the second time the entry may
> + * have been removed already) or buffers that have a modified, and thus
> + * unrecognizable, timestamp. In such cases NULL is returned.
> + *
> * queues_mutex must be held.
> */
> -static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buffer)
> +static GList *find_decoded_entry(SpiceGstDecoder *decoder, GstBuffer *buffer)
> {
> GstClockTime buffer_ts = GST_BUFFER_PTS(buffer);
> #if GST_CHECK_VERSION(1,14,0)
> @@ -238,81 +245,71 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buf
> }
> #endif
>
> - /* Gstreamer sometimes returns the same buffer twice
> - * or buffers that have a modified, and thus unrecognizable, PTS.
> - * Blindly removing frames from the decoding_queue until we find a
> - * match would only empty the queue, resulting in later buffers not
> - * finding a match either, etc. So check the buffer has a matching
> - * frame first.
> - */
> - SpiceGstFrame *gstframe = NULL;
> GList *l = g_queue_peek_head_link(decoder->decoding_queue);
> while (l) {
> - gstframe = l->data;
> + SpiceGstFrame *gstframe = l->data;
> if (gstframe->timestamp == buffer_ts) {
> - break;
> + /* Update the statistics */
> + const SpiceFrame *frame = gstframe->encoded_frame;
> + int64_t duration = g_get_monotonic_time() - frame->creation_time;
> + record(frames_stats,
> + "frame mm_time %u size %u creation time %" PRId64
> + " decoded time %" PRId64 " queue %u",
> + frame->mm_time, frame->size, frame->creation_time,
> + duration, decoder->decoding_queue->length);
> + return l;
> }
> - gstframe = NULL;
> l = l->next;
> }
>
> - if (gstframe != NULL) {
> - /* Now that we know there is a match, remove it and the older
> - * frames from the decoding queue */
> - SpiceGstFrame *late_frame;
> - guint num_frames_dropped = 0;
> -
> - /* The GStreamer pipeline dropped the corresponding buffer. */
> - while ((late_frame = g_queue_pop_head(decoder->decoding_queue)) != gstframe) {
> - num_frames_dropped++;
> - free_gst_frame(late_frame);
> - }
> -
> - if (num_frames_dropped != 0) {
> - SPICE_DEBUG("the GStreamer pipeline dropped %u frames", num_frames_dropped);
> - }
> -
> - const SpiceFrame *frame = gstframe->encoded_frame;
> - int64_t duration = g_get_monotonic_time() - frame->creation_time;
> - record(frames_stats,
> - "frame mm_time %u size %u creation time %" PRId64
> - " decoded time %" PRId64 " queue %u",
> - frame->mm_time, frame->size, frame->creation_time,
> - duration, decoder->decoding_queue->length);
> - }
> - return gstframe;
> + return NULL;
> }
>
> -static void fetch_pending_sample(SpiceGstDecoder *decoder)
> +/* Attaches the specified GStreamer sample to the corresponding SpiceGstFrame
> + * in the decoding queue and returns TRUE. Returns FALSE if no match was found.
> + *
> + * This also removes any older frame that have not been decoded yet as
> + * it means GStreamer dropped them. This ensures the decoded frames form a
> + * contiguous block at the head of the decoding queue.
> + *
> + * queues_mutex must be held.
> + */
> +static gboolean attach_decoded_sample(SpiceGstDecoder *decoder, GstSample *sample)
> {
> - GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
> - if (sample) {
> - // account for the fetched sample
> - decoder->pending_samples--;
> + GstBuffer *buffer = gst_sample_get_buffer(sample);
> + GList *l = find_decoded_entry(decoder, buffer);
> + if (l == NULL) {
> + return FALSE;
Is it possible to have a sample with no matching entry? how this sample
is unrefed in that case?
> + }
>
> - GstBuffer *buffer = gst_sample_get_buffer(sample);
> + SpiceGstFrame *gstframe = l->data;
> + gstframe->decoded_sample = sample;
> + decoder->decoded_frames++;
>
> - /* gst_app_sink_pull_sample() sometimes returns the same buffer twice
> - * or buffers that have a modified, and thus unrecognizable, PTS.
> - * Blindly removing frames from the decoding_queue until we find a
> - * match would only empty the queue, resulting in later buffers not
> - * finding a match either, etc. So check the buffer has a matching
> - * frame first.
> - */
> - SpiceGstFrame *gstframe = get_decoded_frame(decoder, buffer);
> - if (gstframe) {
> - /* The frame is now ready for display */
> - gstframe->decoded_sample = sample;
> - decoder->display_frame = gstframe;
> - } else {
> - spice_warning("got an unexpected decoded buffer!");
> - gst_sample_unref(sample);
> + /* Any older undecoded frame must have been dropped by the GStreamer
> + * pipeline so there is no point in keeping it around.
> + */
> + guint num_frames_dropped = 0;
> + l = l->prev;
> + while (l) {
> + gstframe = l->data;
> + if (gstframe->decoded_sample) {
> + /* It's only decoded frames from there to the first one */
> + break;
> }
> - } else {
> - // no more samples to get, possibly some sample was dropped
> - decoder->pending_samples = 0;
> - spice_warning("GStreamer error: could not pull sample");
> +
> + num_frames_dropped++;
> + free_gst_frame(gstframe);
> + GList *p = l->prev;
> + g_queue_delete_link(decoder->decoding_queue, l);
> +
> + l = p;
> }
> + if (num_frames_dropped) {
> + SPICE_DEBUG("the GStreamer pipeline dropped %u frames", num_frames_dropped);
> + }
> +
> + return TRUE;
> }
>
> /* GStreamer thread
> @@ -327,15 +324,18 @@ static GstFlowReturn new_sample(GstAppSink *gstappsink, gpointer video_decoder)
> {
> SpiceGstDecoder *decoder = video_decoder;
>
> - g_mutex_lock(&decoder->queues_mutex);
> - decoder->pending_samples++;
> - if (decoder->timer_id && decoder->display_frame) {
> + GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
> + if (sample) {
> + g_mutex_lock(&decoder->queues_mutex);
> + if (!attach_decoded_sample(decoder, sample) || decoder->timer_id) {
> + g_mutex_unlock(&decoder->queues_mutex);
> + return GST_FLOW_OK;
> +
new line
> + }
> g_mutex_unlock(&decoder->queues_mutex);
> - return GST_FLOW_OK;
> - }
> - g_mutex_unlock(&decoder->queues_mutex);
>
> - schedule_frame(decoder);
> + schedule_frame(decoder);
> + }
>
> return GST_FLOW_OK;
> }
> @@ -429,10 +429,19 @@ sink_event_probe(GstPad *pad, GstPadProbeInfo *info, gpointer data)
> if (info->type & GST_PAD_PROBE_TYPE_BUFFER) { // Buffer arrived
> GstBuffer *buffer = GST_PAD_PROBE_INFO_BUFFER(info);
> g_mutex_lock(&decoder->queues_mutex);
> - SpiceGstFrame *gstframe = get_decoded_frame(decoder, buffer);
> - if (gstframe) {
> - free_gst_frame(gstframe);
> +
> + /* As a side-effect this updates the decoder statistics */
> + GList *l = find_decoded_entry(decoder, buffer);
> +
> + /* Drop all entries up to this one */
> + while (l) {
> + free_gst_frame((SpiceGstFrame*)l->data);
> +
> + GList *p = l->prev;
> + g_queue_delete_link(decoder->decoding_queue, l);
> + l = p;
>
Isn't it done on attach_decoded_sample?
Also would be nice to update the comment above spice_gst_decoder_queue_frame
with the current flow, will make it easier to follow.
Snir.
> }
> +
> g_mutex_unlock(&decoder->queues_mutex);
> }
> return GST_PAD_PROBE_OK;
More information about the Spice-devel
mailing list