[Spice-devel] [PATCH 01/11] worker: Move stream functions to stream.c
Victor Toso
victortoso at redhat.com
Mon Nov 16 07:06:57 PST 2015
Hi,
On Mon, Nov 16, 2015 at 09:58:33AM -0500, Frediano Ziglio wrote:
> >
> > On Mon, Nov 16, 2015 at 12:06 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > >
> > > ---
> > > server/display-channel.h | 17 +++++++-
> > > server/red_worker.c | 105
> > > ++++-------------------------------------------
> > > server/stream.c | 70 +++++++++++++++++++++++++++++++
> > > server/stream.h | 12 +++++-
> > > 4 files changed, 104 insertions(+), 100 deletions(-)
> > >
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index a9ae40a..9c82465 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -57,8 +57,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)
> > > @@ -345,6 +343,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 165e4c0..f72d304 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -330,22 +330,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))
> > > @@ -357,38 +341,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);
> > > }
> > > @@ -453,40 +411,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++;
> > > - 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)
> > > @@ -822,7 +746,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");
> > > }
> > > @@ -1555,7 +1479,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);
> > > }
> > > }
> > > }
> > > @@ -7400,7 +7324,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);
> > > }
> > > }
> > >
> > > @@ -8463,7 +8387,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:
> > > @@ -8471,7 +8395,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:
> > > @@ -8526,19 +8450,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);
> > > - }
> > > -}
> > > -
> > > static void display_channel_create(RedWorker *worker, int migrate, int
> > > stream_video)
> > > {
> > > DisplayChannel *display_channel;
> > > @@ -8587,11 +8498,11 @@ static void display_channel_create(RedWorker
> > > *worker, int migrate, int stream_vi
> > > display_channel->num_renderers = num_renderers;
> > > memcpy(display_channel->renderers, renderers,
> > > sizeof(display_channel->renderers));
> > > display_channel->renderer = RED_RENDERER_INVALID;
> > > - display_channel->stream_video = stream_video;
> > > - init_streams(display_channel);
> > > image_cache_init(&display_channel->image_cache);
> > > ring_init(&display_channel->current_list);
> > > drawables_init(display_channel);
> > > + display_channel->stream_video = stream_video;
> > > + stream_init(display_channel);
> > > }
> > >
> > > static void guest_set_client_capabilities(RedWorker *worker)
> > > diff --git a/server/stream.c b/server/stream.c
> > > index 7ef7876..d61cec1 100644
> > > --- a/server/stream.c
> > > +++ b/server/stream.c
> > > @@ -57,6 +57,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);
> > > + }
> > > +}
> >
> > It was already pointed out a few times since this patch was sent.
> > stream init is not exactly what this function is doing. It was,
> > previously, called init_streams(), what makes sense as it also
> > initializes the streams ring. IMHO we should differentiate between a
> > single stream operation and operations with the streams' ring. I don't
> > remember if we ended up with another suggestion, neither if it would
> > make sense to split more this patch.
> >
>
> Yes, I suggested to rename the function to display_channel_init_streams.
> Pavel did not like stream_free name and suggested stream_mark_as_free,
> personally I think stream_free is fine as is an allocation inside a static
> pool.
>
> Victor proposed to split the patch. Victor, did you manage to find some
> time to do it?
>
I did not. My plan was spliting in code move, then code rename and other
logic change if necessary to a third patch.
In the same way of last split from Jonathan, I think it would make it
easier to ack the patches.
But it is fine for me if you and Fabiano think this patch is okay to go
(besides the renaming of stream_init) ...
> >
> > > +
> > > +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
> >
> > Best Regards,
> >
>
> I hope to have sum up every request.
>
> Frediano
More information about the Spice-devel
mailing list