[Spice-devel] [PATCH 1/5] worker: Move stream functions to stream.c
Fabiano Fidêncio
fidencio at redhat.com
Tue Nov 10 15:29:51 PST 2015
On Tue, Nov 10, 2015 at 9:41 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> ---
> server/display-channel.h | 17 +++++++-
> server/red_worker.c | 103 ++++-------------------------------------------
> server/stream.c | 70 ++++++++++++++++++++++++++++++++
> server/stream.h | 12 +++++-
> 4 files changed, 103 insertions(+), 99 deletions(-)
>
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 1566c21..8c1648b 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -56,8 +56,6 @@
> #include "tree.h"
> #include "stream.h"
>
> -typedef struct DisplayChannel DisplayChannel;
> -
> #define PALETTE_CACHE_HASH_SHIFT 8
> #define PALETTE_CACHE_HASH_SIZE (1 << PALETTE_CACHE_HASH_SHIFT)
> #define PALETTE_CACHE_HASH_MASK (PALETTE_CACHE_HASH_SIZE - 1)
> @@ -329,6 +327,21 @@ struct DisplayChannel {
> stat_info_t lz4_stat;
> #endif
> };
> +
> +#define LINK_TO_DCC(ptr) SPICE_CONTAINEROF(ptr, DisplayChannelClient, \
> + common.base.channel_link)
> +#define DCC_FOREACH_SAFE(link, next, dcc, channel) \
> + SAFE_FOREACH(link, next, channel, &(channel)->clients, dcc, LINK_TO_DCC(link))
> +
> +
> +#define FOREACH_DCC(display_channel, link, next, dcc) \
> + DCC_FOREACH_SAFE(link, next, dcc, RED_CHANNEL(display_channel))
> +
> +static inline int get_stream_id(DisplayChannel *display, Stream *stream)
> +{
> + return (int)(stream - display->streams_buf);
> +}
> +
> typedef struct SurfaceDestroyItem {
> SpiceMsgSurfaceDestroy surface_destroy;
> PipeItem pipe_item;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 23c54e8..8282b31 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -437,22 +437,6 @@ static void display_channel_client_release_item_before_push(DisplayChannelClient
> static void display_channel_client_release_item_after_push(DisplayChannelClient *dcc,
> PipeItem *item);
>
> -/*
> - * Macros to make iterating over stuff easier
> - * The two collections we iterate over:
> - * given a channel, iterate over it's clients
> - */
> -
> -#define LINK_TO_DCC(ptr) SPICE_CONTAINEROF(ptr, DisplayChannelClient, \
> - common.base.channel_link)
> -#define DCC_FOREACH_SAFE(link, next, dcc, channel) \
> - SAFE_FOREACH(link, next, channel, &(channel)->clients, dcc, LINK_TO_DCC(link))
> -
> -
> -#define FOREACH_DCC(display_channel, link, next, dcc) \
> - DCC_FOREACH_SAFE(link, next, dcc, RED_CHANNEL(display_channel))
> -
> -
> #define LINK_TO_DPI(ptr) SPICE_CONTAINEROF((ptr), DrawablePipeItem, base)
> #define DRAWABLE_FOREACH_DPI_SAFE(drawable, link, next, dpi) \
> SAFE_FOREACH(link, next, drawable, &(drawable)->pipes, dpi, LINK_TO_DPI(link))
> @@ -464,38 +448,12 @@ static void display_channel_client_release_item_after_push(DisplayChannelClient
> SAFE_FOREACH(link, next, drawable, &(drawable)->glz_ring, glz, LINK_TO_GLZ(link))
>
>
> -static int get_stream_id(DisplayChannel *display, Stream *stream)
> -{
> - return (int)(stream - display->streams_buf);
> -}
> -
> -static void display_stream_free(DisplayChannel *display, Stream *stream)
> -{
> - stream->next = display->free_streams;
> - display->free_streams = stream;
> -}
> -
> -static void display_stream_unref(DisplayChannel *display, Stream *stream)
> -{
> - if (--stream->refs != 0)
> - return;
> -
> - spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> - display_stream_free(display, stream);
> - display->stream_count--;
> -}
> -
> -static void display_stream_agent_unref(DisplayChannel *display, StreamAgent *agent)
> -{
> - display_stream_unref(display, agent->stream);
> -}
> -
> static void display_stream_clip_unref(DisplayChannel *display, StreamClipItem *item)
> {
> if (--item->refs != 0)
> return;
>
> - display_stream_agent_unref(display, item->stream_agent);
> + stream_agent_unref(display, item->stream_agent);
> free(item->rects);
> free(item);
> }
> @@ -560,40 +518,6 @@ void attach_stream(DisplayChannel *display, Drawable *drawable, Stream *stream)
> }
> }
>
> -static void stop_stream(DisplayChannel *display, Stream *stream)
> -{
> - DisplayChannelClient *dcc;
> - RingItem *item, *next;
> -
> - spice_assert(ring_item_is_linked(&stream->link));
> - spice_assert(!stream->current);
> - spice_debug("stream %d", get_stream_id(display, stream));
> - FOREACH_DCC(display, item, next, dcc) {
> - StreamAgent *stream_agent;
> -
> - stream_agent = &dcc->stream_agents[get_stream_id(display, stream)];
> - region_clear(&stream_agent->vis_region);
> - region_clear(&stream_agent->clip);
> - spice_assert(!pipe_item_is_linked(&stream_agent->destroy_item));
> - if (stream_agent->mjpeg_encoder && dcc->use_mjpeg_encoder_rate_control) {
> - uint64_t stream_bit_rate = mjpeg_encoder_get_bit_rate(stream_agent->mjpeg_encoder);
> -
> - if (stream_bit_rate > dcc->streams_max_bit_rate) {
> - spice_debug("old max-bit-rate=%.2f new=%.2f",
> - dcc->streams_max_bit_rate / 8.0 / 1024.0 / 1024.0,
> - stream_bit_rate / 8.0 / 1024.0 / 1024.0);
> - dcc->streams_max_bit_rate = stream_bit_rate;
> - }
> - }
> - stream->refs++;
This comment is not related to the patch itself ...
Why the stream's reference counter is incremented here?
> - red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &stream_agent->destroy_item);
> - stream_agent_stats_print(stream_agent);
> - }
> - display->streams_size_total -= stream->width * stream->height;
> - ring_remove(&stream->link);
> - display_stream_unref(display, stream);
> -}
> -
> /* fixme: move to display channel */
> DrawablePipeItem *drawable_pipe_item_new(DisplayChannelClient *dcc,
> Drawable *drawable)
> @@ -937,7 +861,7 @@ static void stop_streams(DisplayChannel *display)
> Stream *stream = SPICE_CONTAINEROF(item, Stream, link);
> item = ring_next(ring, item);
> if (!stream->current) {
> - stop_stream(display, stream);
> + stream_stop(display, stream);
> } else {
> spice_info("attached stream");
> }
> @@ -1781,7 +1705,7 @@ static void display_channel_streams_timeout(DisplayChannel *display)
> item = ring_next(ring, item);
> if (now >= (stream->last_time + RED_STREAM_TIMEOUT)) {
> detach_stream_gracefully(display, stream, NULL);
> - stop_stream(display, stream);
> + stream_stop(display, stream);
> }
> }
> }
> @@ -7623,7 +7547,7 @@ static void detach_and_stop_streams(DisplayChannel *display)
> Stream *stream = SPICE_CONTAINEROF(stream_item, Stream, link);
>
> detach_stream_gracefully(display, stream, NULL);
> - stop_stream(display, stream);
> + stream_stop(display, stream);
> }
> }
>
> @@ -8686,7 +8610,7 @@ static void display_channel_client_release_item_before_push(DisplayChannelClient
> }
> case PIPE_ITEM_TYPE_STREAM_CREATE: {
> StreamAgent *agent = SPICE_CONTAINEROF(item, StreamAgent, create_item);
> - display_stream_agent_unref(display, agent);
> + stream_agent_unref(display, agent);
> break;
> }
> case PIPE_ITEM_TYPE_STREAM_CLIP:
> @@ -8694,7 +8618,7 @@ static void display_channel_client_release_item_before_push(DisplayChannelClient
> break;
> case PIPE_ITEM_TYPE_STREAM_DESTROY: {
> StreamAgent *agent = SPICE_CONTAINEROF(item, StreamAgent, destroy_item);
> - display_stream_agent_unref(display, agent);
> + stream_agent_unref(display, agent);
> break;
> }
> case PIPE_ITEM_TYPE_UPGRADE:
> @@ -8749,19 +8673,6 @@ static void display_channel_release_item(RedChannelClient *rcc, PipeItem *item,
> }
> }
>
> -static void init_streams(DisplayChannel *display)
> -{
> - int i;
> -
> - ring_init(&display->streams);
> - display->free_streams = NULL;
> - for (i = 0; i < NUM_STREAMS; i++) {
> - Stream *stream = &display->streams_buf[i];
> - ring_item_init(&stream->link);
> - display_stream_free(display, stream);
> - }
> -}
> -
Not that related to the patch as well, as this function is removed
from here and added in the stream.c without any change, but it's doing
too much for "stream_init".
AFAIU, the stream_init part must be: given a stream, do the
ring_item_init and do the display_stream_free() (and doesn't look like
a good name for what the function does), the rest of the function
could be part of the caller (or in another function ...)
> static void display_channel_create(RedWorker *worker, int migrate)
> {
> DisplayChannel *display_channel;
> @@ -8807,10 +8718,10 @@ static void display_channel_create(RedWorker *worker, int migrate)
> display_channel->num_renderers = num_renderers;
> memcpy(display_channel->renderers, renderers, sizeof(display_channel->renderers));
> display_channel->renderer = RED_RENDERER_INVALID;
> - init_streams(display_channel);
> image_cache_init(&display_channel->image_cache);
> ring_init(&display_channel->current_list);
> drawables_init(display_channel);
> + stream_init(display_channel);
> }
>
> static void guest_set_client_capabilities(RedWorker *worker)
> diff --git a/server/stream.c b/server/stream.c
> index 6203f3d..0d8cbd9 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -53,6 +53,76 @@ void stream_agent_stats_print(StreamAgent *agent)
> #endif
> }
>
> +void stream_stop(DisplayChannel *display, Stream *stream)
> +{
> + DisplayChannelClient *dcc;
> + RingItem *item, *next;
> +
> + spice_return_if_fail(ring_item_is_linked(&stream->link));
> + spice_return_if_fail(!stream->current);
> +
> + spice_debug("stream %d", get_stream_id(display, stream));
> + FOREACH_DCC(display, item, next, dcc) {
> + StreamAgent *stream_agent;
> +
> + stream_agent = &dcc->stream_agents[get_stream_id(display, stream)];
> + region_clear(&stream_agent->vis_region);
> + region_clear(&stream_agent->clip);
> + spice_assert(!pipe_item_is_linked(&stream_agent->destroy_item));
> + if (stream_agent->mjpeg_encoder && dcc->use_mjpeg_encoder_rate_control) {
> + uint64_t stream_bit_rate = mjpeg_encoder_get_bit_rate(stream_agent->mjpeg_encoder);
> +
> + if (stream_bit_rate > dcc->streams_max_bit_rate) {
> + spice_debug("old max-bit-rate=%.2f new=%.2f",
> + dcc->streams_max_bit_rate / 8.0 / 1024.0 / 1024.0,
> + stream_bit_rate / 8.0 / 1024.0 / 1024.0);
> + dcc->streams_max_bit_rate = stream_bit_rate;
> + }
> + }
> + stream->refs++;
> + red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &stream_agent->destroy_item);
> + stream_agent_stats_print(stream_agent);
> + }
> + display->streams_size_total -= stream->width * stream->height;
> + ring_remove(&stream->link);
> + stream_unref(display, stream);
> +}
> +
> +static void stream_free(DisplayChannel *display, Stream *stream)
> +{
> + stream->next = display->free_streams;
> + display->free_streams = stream;
> +}
> +
> +void stream_init(DisplayChannel *display)
> +{
> + int i;
> +
> + ring_init(&display->streams);
> + display->free_streams = NULL;
> + for (i = 0; i < NUM_STREAMS; i++) {
> + Stream *stream = &display->streams_buf[i];
> + ring_item_init(&stream->link);
> + stream_free(display, stream);
> + }
> +}
> +
> +void stream_unref(DisplayChannel *display, Stream *stream)
> +{
> + if (--stream->refs != 0)
> + return;
> +
> + spice_warn_if_fail(!ring_item_is_linked(&stream->link));
> +
> + stream_free(display, stream);
> + display->stream_count--;
> +}
> +
> +void stream_agent_unref(DisplayChannel *display, StreamAgent *agent)
> +{
> + stream_unref(display, agent->stream);
> +}
> +
> StreamClipItem *stream_clip_item_new(DisplayChannelClient* dcc, StreamAgent *agent)
> {
> StreamClipItem *item = spice_new(StreamClipItem, 1);
> diff --git a/server/stream.h b/server/stream.h
> index f77fa96..4704937 100644
> --- a/server/stream.h
> +++ b/server/stream.h
> @@ -40,6 +40,9 @@
> #define RED_STREAM_DEFAULT_HIGH_START_BIT_RATE (10 * 1024 * 1024) // 10Mbps
> #define RED_STREAM_DEFAULT_LOW_START_BIT_RATE (2.5 * 1024 * 1024) // 2.5Mbps
>
> +/* move back to display_channel once struct private */
> +typedef struct DisplayChannel DisplayChannel;
> +
> typedef struct Stream Stream;
>
> typedef struct StreamActivateReportItem {
> @@ -132,6 +135,13 @@ struct Stream {
> uint32_t input_fps;
> };
>
> -void stream_agent_stats_print(StreamAgent *agent);
> +void stream_init (DisplayChannel *display);
> +void stream_stop (DisplayChannel *display,
> + Stream *stream);
> +void stream_unref (DisplayChannel *display,
> + Stream *stream);
> +void stream_agent_unref (DisplayChannel *display,
> + StreamAgent *agent);
> +void stream_agent_stats_print (StreamAgent *agent);
>
> #endif /* STREAM_H */
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list