[Spice-devel] [PATCH v1 5/5] video-stream: Don't stop a stream if a gl_draw operation is pending

[Frediano Ziglio]

Il giorno gio 16 mar 2023 alle ore 06:05 Vivek Kasireddy
<vivek.kasireddy at intel.com> ha scritto:
>
> Before stopping a stream, we need to ensure that there is no ongoing
> gl_draw operation; otherwise, we may not get another opportunity to
> create a new stream if the current one gets stopped. And, once the
> stream is stopped, we need to clear the gl_draw_stream pointer
> associated with the relevant client (dcc).
>
> Cc: Gerd Hoffmann <kraxel at redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau at redhat.com>
> Cc: Dongwon Kim <dongwon.kim at intel.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> ---
>  server/video-stream.cpp | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/server/video-stream.cpp b/server/video-stream.cpp
> index 03a7d68d..b7f3d2c5 100644
> --- a/server/video-stream.cpp
> +++ b/server/video-stream.cpp
> @@ -18,6 +18,7 @@
>
>  #include "video-stream.h"
>  #include "display-channel-private.h"
> +#include "dcc-private.h"

This smells bad from design, it loose the "private" meaning.

>  #include "main-channel-client.h"
>  #include "red-client.h"
>
> @@ -115,6 +116,10 @@ void video_stream_stop(DisplayChannel *display, VideoStream *stream)
>          }
>          dcc->pipe_add(video_stream_destroy_item_new(stream_agent));
>          video_stream_agent_stats_print(stream_agent);
> +
> +        if (stream == dcc->priv->gl_draw_stream) {
> +            dcc->priv->gl_draw_stream = nullptr;

It looks like a "weak" pointer. Why is that? Maybe the bound between
stream and time expiration? Maybe for that specific stream time
expiration does not make much sense?

> +        }
>      }
>      display->priv->streams_size_total -= stream->width * stream->height;
>      ring_remove(&stream->link);
> @@ -974,6 +979,19 @@ void video_stream_detach_and_stop(DisplayChannel *display)
>      }
>  }
>
> +static bool is_stream_gl_draw(VideoStream *stream, DisplayChannel *display)
> +{
> +    DisplayChannelClient *dcc;
> +
> +    FOREACH_DCC(display, dcc) {
> +        if (stream == dcc->priv->gl_draw_stream &&
> +            dcc->priv->gl_draw_ongoing) {
> +            return TRUE;

Minor: true

> +        }
> +    }
> +    return FALSE;

Minor: false

Maybe use a boolean flag in VideoStream instead ?

> +}
> +
>  void video_stream_timeout(DisplayChannel *display)
>  {
>      Ring *ring = &display->priv->streams;
> @@ -984,7 +1002,8 @@ void video_stream_timeout(DisplayChannel *display)
>      while (item) {
>          VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream, link);
>          item = ring_next(ring, item);
> -        if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
> +        if (now >= (stream->last_time + RED_STREAM_TIMEOUT) &&
> +            !is_stream_gl_draw(stream, display)) {
>              detach_video_stream_gracefully(display, stream, nullptr);
>              video_stream_stop(display, stream);
>          }

Frediano