>
> This allows us to unref the stream directly rather than needing to pass
> the associated DisplayChannel to stream_unref(). The same is also true
> for stream_agent_unref, since the only reason that stream_agent_unref()
> required a DisplayChannel parameter was to pass it to stream_unref().
> This also resulted in some additional minor redesign due to the fact
> that streams are pre-allocated in a pool in the display channel, and
> freeing a stream simply means moving it to a different ring to be
> re-used.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> server/display-channel.c | 27 ++++++++++++++++++++++++++-
> server/display-channel.h | 1 +
> server/video-stream.c | 44 ++++++++++++--------------------------------
> server/video-stream.h | 8 ++++----
> 4 files changed, 43 insertions(+), 37 deletions(-)
>
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2caaa6433..d3db1e0db 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -272,7 +272,7 @@ static void stop_streams(DisplayChannel *display)
> VideoStream *stream = SPICE_CONTAINEROF(item, VideoStream, link);
> item = ring_next(ring, item);
> if (!stream->current) {
> - video_stream_stop(display, stream);
> + video_stream_stop(stream);
> } else {
> spice_debug("attached stream");
> }
> @@ -2279,6 +2279,31 @@ display_channel_init(DisplayChannel *self)
> self->priv->image_surfaces.ops = &image_surfaces_ops;
> }
>
> +static void display_channel_mark_stream_unused(DisplayChannel *display,
> VideoStream *stream)
> +{
> + stream->next = display->priv->free_streams;
> + display->priv->free_streams = stream;
> +}
> +
> +static void display_channel_init_video_streams(DisplayChannel *display)
> +{
> + int i;
> +
> + ring_init(&display->priv->streams);
> + display->priv->free_streams = NULL;
> + for (i = 0; i < NUM_STREAMS; i++) {
> + VideoStream *stream = display_channel_get_nth_video_stream(display,
> i);
> + ring_item_init(&stream->link);
> + display_channel_mark_stream_unused(display, stream);
> + }
> +}
> +
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream
> *stream)
> +{
> + display_channel_mark_stream_unused(stream->display, stream);
> + display->priv->stream_count--;
> +}
> +
> static void
> display_channel_constructed(GObject *object)
> {
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 4f7def216..a0470ec67 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -168,6 +168,7 @@ gboolean
> display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
> void display_channel_reset_image_cache(DisplayChannel *self);
>
> void display_channel_debug_oom(DisplayChannel *display, const char *msg);
> +void display_channel_free_video_stream(DisplayChannel *display, VideoStream
> *stream);
>
> G_END_DECLS
>
> diff --git a/server/video-stream.c b/server/video-stream.c
> index 65da3f8a0..a24c787ef 100644
> --- a/server/video-stream.c
> +++ b/server/video-stream.c
> @@ -66,8 +66,7 @@ static void video_stream_agent_stats_print(VideoStreamAgent
> *agent)
> static void video_stream_create_destroy_item_release(RedPipeItem *base)
> {
> StreamCreateDestroyItem *item = SPICE_UPCAST(StreamCreateDestroyItem,
> base);
> - DisplayChannel *display = DCC_TO_DC(item->agent->dcc);
> - video_stream_agent_unref(display, item->agent);
> + video_stream_agent_unref(item->agent);
> g_free(item);
> }
>
> @@ -94,8 +93,9 @@ static RedPipeItem
> *video_stream_destroy_item_new(VideoStreamAgent *agent)
> }
>
>
> -void video_stream_stop(DisplayChannel *display, VideoStream *stream)
> +void video_stream_stop(VideoStream *stream)
> {
> + DisplayChannel *display = stream->display;
> DisplayChannelClient *dcc;
> int stream_id = display_channel_get_video_stream_id(display, stream);
>
> @@ -125,53 +125,32 @@ void video_stream_stop(DisplayChannel *display,
> VideoStream *stream)
> }
> display->priv->streams_size_total -= stream->width * stream->height;
> ring_remove(&stream->link);
> - video_stream_unref(display, stream);
> + video_stream_unref(stream);
> }
>
> -static void video_stream_free(DisplayChannel *display, VideoStream *stream)
> -{
> - stream->next = display->priv->free_streams;
> - display->priv->free_streams = stream;
> -}
> -
> -void display_channel_init_video_streams(DisplayChannel *display)
> -{
> - int i;
> -
> - ring_init(&display->priv->streams);
> - display->priv->free_streams = NULL;
> - for (i = 0; i < NUM_STREAMS; i++) {
> - VideoStream *stream = display_channel_get_nth_video_stream(display,
> i);
> - ring_item_init(&stream->link);
> - video_stream_free(display, stream);
> - }
> -}
> -
> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
> +void video_stream_unref(VideoStream *stream)
> {
> if (--stream->refs != 0)
> return;
>
> spice_warn_if_fail(!ring_item_is_linked(&stream->link));
>
> - video_stream_free(display, stream);
> - display->priv->stream_count--;
> + display_channel_free_video_stream(stream->display, stream);
> }
>
> -void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> *agent)
> +void video_stream_agent_unref(VideoStreamAgent *agent)
> {
> - video_stream_unref(display, agent->stream);
> + video_stream_unref(agent->stream);
> }
>
> static void video_stream_clip_item_free(RedPipeItem *base)
> {
> g_return_if_fail(base != NULL);
> VideoStreamClipItem *item = SPICE_UPCAST(VideoStreamClipItem, base);
> - DisplayChannel *display = DCC_TO_DC(item->stream_agent->dcc);
>
> g_return_if_fail(item->base.refcount == 0);
>
> - video_stream_agent_unref(display, item->stream_agent);
> + video_stream_agent_unref(item->stream_agent);
> g_free(item->rects);
> g_free(item);
> }
> @@ -375,6 +354,7 @@ static VideoStream
> *display_channel_stream_try_new(DisplayChannel *display)
> }
> stream = display->priv->free_streams;
> display->priv->free_streams = display->priv->free_streams->next;
> + stream->display = display;
> return stream;
> }
>
> @@ -923,7 +903,7 @@ void video_stream_detach_and_stop(DisplayChannel
> *display)
> VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream,
> link);
>
> detach_video_stream_gracefully(display, stream, NULL);
> - video_stream_stop(display, stream);
> + video_stream_stop(stream);
> }
> }
>
> @@ -939,7 +919,7 @@ void video_stream_timeout(DisplayChannel *display)
> item = ring_next(ring, item);
> if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
> detach_video_stream_gracefully(display, stream, NULL);
> - video_stream_stop(display, stream);
> + video_stream_stop(stream);
> }
> }
> }
> diff --git a/server/video-stream.h b/server/video-stream.h
> index a4d149816..88d59c848 100644
> --- a/server/video-stream.h
> +++ b/server/video-stream.h
> @@ -125,11 +125,11 @@ struct VideoStream {
> uint32_t num_input_frames;
> uint64_t input_fps_start_time;
> uint32_t input_fps;
> + DisplayChannel *display;
> };
>
> -void display_channel_init_video_streams(DisplayChannel *display);
> -void video_stream_stop(DisplayChannel *display, VideoStream *stream);
> -void video_stream_unref(DisplayChannel *display, VideoStream *stream);
> +void video_stream_stop(VideoStream *stream);
> +void video_stream_unref(VideoStream *stream);
> void video_stream_trace_update(DisplayChannel *display, Drawable *drawable);
> void video_stream_maintenance(DisplayChannel *display, Drawable *candidate,
> Drawable *prev);
> @@ -139,7 +139,7 @@ void video_stream_trace_add_drawable(DisplayChannel
> *display, Drawable *item);
> void video_stream_detach_behind(DisplayChannel *display, QRegion *region,
> Drawable *drawable);
>
> -void video_stream_agent_unref(DisplayChannel *display, VideoStreamAgent
> *agent);
> +void video_stream_agent_unref(VideoStreamAgent *agent);
> void video_stream_agent_stop(VideoStreamAgent *agent);
>
> void video_stream_detach_drawable(VideoStream *stream);
Looks like this patch is trying to do multiple stuff.
Yes, having display directly in the VideoStream helps but for instance
there are different functions (like display_channel_get_stream_id) that
would benefit from this change.
On the other side there's no much rationale on some function move.
This for this patch but also 3/7.
What should the rule why a given function should be in video-stream.c
and not on display-channel.c/dcc.c/dcc-send.c? Not that I have a precise
idea myself but these functions had been moved around multiple time
and I think the main reason is that we don't have an agreed reasoning
of this split. Said that streaming handling is part of the display channel
where should be the code that detect new streams reside? For instance
looks like the code that initialize a VideoStream is in display-channel.c
but all the *_new functions are usually in the same file that should
handle the structure, in this case video-stream.c.
If the VideoStreamAgent is specifically related to the client (so
DisplayChannelClient) should the code reside in dcc.c or video-stream.c?
Why the definition is in video-stream.h and some code in dcc.c?
I tried to do some split like what I did for image encoding but
got not really far, the code is too much mingled.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel