[Spice-devel] [PATCH spice-gtk 2/3] Drop frames if the backlog is above some limit

[Victor Toso]

Hi,

On Thu, Jul 13, 2017 at 04:57:27PM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin at redhat.com>
>
> Experience has shown that if the machine running the guest is overloaded,
> it may pile up a lot of backlog in the frames queue. This patch clears
> the queue if it exceeds 100 entries. This value is arbitrary. It
> corresponds to a few seconds on a highly overloaded machine.

I guess it depends on the stream. On 4K it might be too small, for
800x600, too big. Shouldn't we take in consideration the payload size?

To the commit log, I'd also add the situation you had (OOM) and the
outcome with this patch. Any glitches? Black screen?

> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  src/channel-display-gst.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 3cf451b..17c2847 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -498,6 +498,9 @@ static void spice_gst_decoder_destroy(VideoDecoder *video_decoder)
>   * 9) display_frame() then frees the SpiceGstFrame, which frees the SpiceFrame
>   *    and decompressed frame with it.
>   */
> +
> +SPICE_TWEAK_DEFINE(gst_queue_max_length, 100, "Max length of GStreamer queue");

> +
>  static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>                                                SpiceFrame *frame, int latency)
>  {
> @@ -541,6 +544,16 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>      }
>  #endif
>
> +    if (SPICE_TWEAK(gst_queue_max_length) &&
> +        decoder->decoding_queue->length > SPICE_TWEAK(gst_queue_max_length)) {

I'm not sure if we should mix things here. The main point of this patch
is to avoid holding too much memory. The SPICE_TWEAK() is a proposal and
not something used in the code base (yet!).


> +       SpiceGstFrame *gstframe;
> +       g_mutex_lock(&decoder->queues_mutex);
> +        while ((gstframe = g_queue_pop_head(decoder->decoding_queue)))
> +            free_gst_frame(gstframe);

g_queue_free_full()

> +        g_mutex_unlock(&decoder->queues_mutex);
> +        return TRUE;

Not sure *how* but it would be great to identify an i-frame as we are
possibly dropping it... maybe a FIXME or a TODO about it.. not sure if
really necessary :)

Cheers,
    toso

> +    }
> +
>      /* ref() the frame data for the buffer */
>      frame->ref_data(frame->data_opaque);
>      GstBuffer *buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
> -- 
> 2.11.0 (Apple Git-81)
> 
> _______________________________________________
> 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/20170714/5dee3bbe/attachment.sig>