Il giorno gio 16 mar 2023 alle ore 06:05 Vivek Kasireddy
<vivek.kasireddy@xxxxxxxxx> ha scritto:
>
> For non-gl/remote clients, if a stream does not exist or if any
> of the key parameters associated with gl_draw such as x/y or
> width/height are changed, then we create a new stream. Otherwise,
> we just update the current stream's timestamp.
>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> Cc: Dongwon Kim <dongwon.kim@xxxxxxxxx>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@xxxxxxxxx>
> ---
> server/dcc-private.h | 2 ++
> server/dcc.cpp | 14 +++++++++++++
> server/video-stream.cpp | 44 +++++++++++++++++++++++++++++++++++++++++
> server/video-stream.h | 2 ++
> 4 files changed, 62 insertions(+)
>
> diff --git a/server/dcc-private.h b/server/dcc-private.h
> index bf485aec..cd5b76a3 100644
> --- a/server/dcc-private.h
> +++ b/server/dcc-private.h
> @@ -69,6 +69,8 @@ struct DisplayChannelClientPrivate
> uint32_t streams_max_latency;
> uint64_t streams_max_bit_rate;
> bool gl_draw_ongoing;
> +
> + VideoStream *gl_draw_stream;
This raw pointer smells pretty bad. Defined here seems also to
indicate that every client could have a different draw stream, which
seems wrong.
> };
>
> #include "pop-visibility.h"
> diff --git a/server/dcc.cpp b/server/dcc.cpp
> index f0b355ca..9d91ca53 100644
> --- a/server/dcc.cpp
> +++ b/server/dcc.cpp
> @@ -525,6 +525,20 @@ RedPipeItemPtr dcc_gl_draw_item_new(RedChannelClient *rcc, void *data, int num)
> auto item = red::make_shared<RedGlDrawItem>();
> item->draw = *draw;
>
> + if (!dcc_is_gl_client(dcc)) {
> + VideoStream *stream = dcc->priv->gl_draw_stream;
> +
> + if (!stream ||
> + stream->dest_area.left != draw->x ||
> + stream->dest_area.top != draw->y ||
> + stream->width != draw->w || stream->height != draw->h) {
> + stream = display_channel_create_gl_draw_stream(dcc, draw);
> + } else {
> + stream->last_time = spice_get_monotonic_time_ns();
> + }
> + dcc->priv->gl_draw_stream = stream;
This rewrite with a create and not a destroy looks like a leak.
Why assign it even if you are not changing it?
> + }
> +
> return item;
> }
>
> diff --git a/server/video-stream.cpp b/server/video-stream.cpp
> index 056d0c31..03a7d68d 100644
> --- a/server/video-stream.cpp
> +++ b/server/video-stream.cpp
> @@ -415,6 +415,47 @@ static void display_channel_create_stream(DisplayChannel *display, Drawable *dra
> stream->input_fps);
> }
>
> +VideoStream *
> +display_channel_create_gl_draw_stream(DisplayChannelClient *dcc,
> + const SpiceMsgDisplayGlDraw *draw)
> +{
> + DisplayChannel *display = DCC_TO_DC(dcc);
> + VideoStream *stream;
> + SpiceRect dest_area = {
> + .left = draw->x,
> + .top = draw->y,
> + .right = draw->w,
> + .bottom = draw->h
> + };
> +
> + if (!(stream = display_channel_stream_try_new(display))) {
> + return nullptr;
> + }
> +
> + ring_add(&display->priv->streams, &stream->link);
> + stream->current = nullptr;
> + stream->last_time = spice_get_monotonic_time_ns();
> + stream->width = draw->w;
> + stream->height = draw->h;
> + stream->dest_area = dest_area;
> + stream->refs = 1;
> + stream->top_down = 1;
> + stream->input_fps = MAX_FPS;
> + stream->num_input_frames = 0;
> + stream->input_fps_start_time = spice_get_monotonic_time_ns();
> + display->priv->streams_size_total += stream->width * stream->height;
> + display->priv->stream_count++;
> +
> + dcc_create_stream(dcc, stream);
> + spice_debug("stream %d %dx%d (%d, %d) (%d, %d) %u fps",
> + display_channel_get_video_stream_id(display, stream), stream->width,
> + stream->height, stream->dest_area.left, stream->dest_area.top,
> + stream->dest_area.right, stream->dest_area.bottom,
> + stream->input_fps);
> +
> + return stream;
> +}
> +
This looks like a big copy&paste from another function. Could we not
reuse some code?
> // returns whether a stream was created
> static bool video_stream_add_frame(DisplayChannel *display,
> Drawable *frame_drawable,
> @@ -744,6 +785,9 @@ void dcc_create_stream(DisplayChannelClient *dcc, VideoStream *stream)
> if (stream->current) {
> region_clone(&agent->vis_region, &stream->current->tree_item.base.rgn);
> region_clone(&agent->clip, &agent->vis_region);
> + } else {
> + region_add(&agent->vis_region, &stream->dest_area);
> + region_clone(&agent->clip, &agent->vis_region);
> }
> agent->dcc = dcc;
>
> diff --git a/server/video-stream.h b/server/video-stream.h
> index 23b44ff5..a8b2c61c 100644
> --- a/server/video-stream.h
> +++ b/server/video-stream.h
> @@ -124,6 +124,8 @@ struct VideoStream {
> };
>
> void display_channel_init_video_streams(DisplayChannel *display);
> +VideoStream *display_channel_create_gl_draw_stream(DisplayChannelClient *dcc,
> + const SpiceMsgDisplayGlDraw *draw);
> void video_stream_stop(DisplayChannel *display, VideoStream *stream);
> void video_stream_trace_update(DisplayChannel *display, Drawable *drawable);
> void video_stream_maintenance(DisplayChannel *display, Drawable *candidate,
> --
> 2.37.2
>