[Spice-devel] [PATCH spice-gtk v3] channel-display: Instrument code to get frame and queue statistics

[Victor Toso]

Hi,

On Wed, Jan 23, 2019 at 08:32:22PM +0000, Frediano Ziglio wrote:
> This patch is based on some work from Victor Toso (statistics) and
> Snir Sheriber (GStreamer probing).
> All GstBuffers are queued into decoding_queue and a probe is
> attached to the sink in order to understand when the buffers
> are decoded.
> Previously we didn't add frames to decoding_queue in case of direct
> rendering as there was nothing to dequeue them, now we need it to
> compute the timing.
> When a buffer is decoded the time spent and queue length are
> computed.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  src/channel-display-gst.c  | 48 +++++++++++++++++++++++++++++++++-----
>  src/channel-display-priv.h |  3 +++
>  src/channel-display.c      |  1 +
>  3 files changed, 46 insertions(+), 6 deletions(-)
> 
> Changes since v2:
> - rename add_elem_cb to deep_element_added_cb;
> - more comments (both code and commit message);
> - some style change.
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f700a28a..1251314c 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -250,6 +250,12 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder, GstBuffer *buf
>      if (num_frames_dropped != 0) {
>          SPICE_DEBUG("the GStreamer pipeline dropped %u frames", num_frames_dropped);
>      }
> +    if (gstframe) {
> +        const SpiceFrame *frame = gstframe->encoded_frame;
> +        uint64_t duration = g_get_monotonic_time() - frame->creation_time;
> +        SPICE_DEBUG("frame mm_time %u size %u decoded time %" G_GUINT64_FORMAT " queue %u",
> +                    frame->mm_time, frame->size, duration, decoder->decoding_queue->length);
> +    }
>      return gstframe;
>  }
>  
> @@ -393,6 +399,37 @@ static void app_source_setup(GstElement *pipeline G_GNUC_UNUSED,
>      gst_caps_unref(caps);
>      decoder->appsrc = GST_APP_SRC(gst_object_ref(source));
>  }
> +
> +static GstPadProbeReturn
> +sink_event_probe(GstPad *pad, GstPadProbeInfo *info, gpointer data)
> +{
> +    SpiceGstDecoder *decoder = (SpiceGstDecoder*)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);
> +        }
> +        g_mutex_unlock(&decoder->queues_mutex);
> +    }
> +    return GST_PAD_PROBE_OK;
> +}
> +
> +/* This function is called to used to set a probe on the sink */
> +static void
> +deep_element_added_cb(GstBin *pipeline, GstBin *bin, GstElement *element,
> +                      SpiceGstDecoder *decoder)
> +{
> +    /* attach a probe to the sink in case we don't have appsink (that
> +     * is the frames does not go to new_sample */
> +    if (GST_IS_BASE_SINK(element) && decoder->appsink == NULL) {

My question on the other thread was to know if we are only
targeting the overlay code path. AFAICS, the check is not needed,
thus I'm asking :)

Otherwise, looks good

> +        GstPad *pad = gst_element_get_static_pad(element, "sink");
> +        gst_pad_add_probe(pad, GST_PAD_PROBE_TYPE_BUFFER, sink_event_probe, decoder, NULL);
> +        gst_object_unref(pad);
> +    }
> +}
>  #endif
>  
>  static gboolean create_pipeline(SpiceGstDecoder *decoder)
> @@ -457,6 +494,7 @@ static gboolean create_pipeline(SpiceGstDecoder *decoder)
>  #endif
>      }
>  
> +    g_signal_connect(playbin, "deep-element-added", G_CALLBACK(deep_element_added_cb), decoder);
>      g_signal_connect(playbin, "source-setup", G_CALLBACK(app_source_setup), decoder);
>  
>      g_object_set(playbin,
> @@ -667,12 +705,10 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
>      GST_BUFFER_PTS(buffer) = pts;
>  
> -    if (decoder->appsink != NULL) {
> -        SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
> -        g_mutex_lock(&decoder->queues_mutex);
> -        g_queue_push_tail(decoder->decoding_queue, gst_frame);
> -        g_mutex_unlock(&decoder->queues_mutex);
> -    }
> +    SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
> +    g_mutex_lock(&decoder->queues_mutex);
> +    g_queue_push_tail(decoder->decoding_queue, gst_frame);
> +    g_mutex_unlock(&decoder->queues_mutex);
>  
>      if (gst_app_src_push_buffer(decoder->appsrc, buffer) != GST_FLOW_OK) {
>          SPICE_DEBUG("GStreamer error: unable to push frame");
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index c5c3f5c8..495df7ac 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -45,6 +45,9 @@ struct SpiceFrame {
>      uint8_t *data;
>      uint32_t size;
>      gpointer data_opaque;
> +
> +    /* stats */
> +    gint64 creation_time;
>  };
>  
>  typedef struct VideoDecoder VideoDecoder;
> diff --git a/src/channel-display.c b/src/channel-display.c
> index ae2c4fbc..d46b3a37 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1657,6 +1657,7 @@ static SpiceFrame *spice_frame_new(display_stream *st,
>      frame->size = data_size;
>      frame->data_opaque = in;
>      spice_msg_in_ref(in);
> +    frame->creation_time = g_get_monotonic_time();
>      return frame;
>  }
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- 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/20190123/6b7827cb/attachment.sig>