Re: [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 
>
> 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 
> ---
>  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(>queues_mutex);
> +while ((gstframe = g_queue_pop_head(decoder->decoding_queue)))
> +free_gst_frame(gstframe);

g_queue_free_full()

> +g_mutex_unlock(>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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Marc-André Lureau]

- Original Message -
> From: Christophe de Dinechin 
> 
> 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.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  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))
> {
> +   SpiceGstFrame *gstframe;
> +   g_mutex_lock(>queues_mutex);
> +while ((gstframe = g_queue_pop_head(decoder->decoding_queue)))
> +free_gst_frame(gstframe);
> +g_mutex_unlock(>queues_mutex);
> +return TRUE;
> +}
> +

I am surprised that GStreamer queues are not smart enough to deal with this 
kind of situation. Oh wait, why is the queue in spice-gtk side? Perhaps 
Francois can comment on this?

>  /* 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel