On Thu, Nov 12, 2015 at 3:01 PM, Pavel Grunt <pgrunt@xxxxxxxxxx> wrote:
> Hi,
>
> On Wed, 2015-11-11 at 00:29 +0100, Fabiano Fidêncio wrote:
>> On Tue, Nov 10, 2015 at 9:41 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> 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 ...)
>>
> I am not sure I understand: Would you prefer to have two functions
> - init_streams calling init_stream for each stream?
>
> static void init_stream(DisplayChannel *display, Stream *stream) {
> ring_item_init(&stream->link);
> display_stream_free(display, stream);
> }
>
> static void init_streams(DisplayChannel *display, Stream *stream) {
> int i;
> ring_init(&display->streams);
>
> for (i = 0; i < NUM_STREAMS; i++) {
> init_stream(display, &display->streams_buf[i]);
> }
> }
Hmm. I placed my comment in the wrong place.
The way this function is right now and its name are matching, IMHO.
But it was changed to stream_init() ... and clearly it's doing more
than just initialize _one_ stream.
>
> And yes, the name 'display_stream_free' is confusing I would preferred something
> like display_stream_mark_as_free.
Not sure about a proper name, but your suggestion is way better than
the current name :-)
>
> Pavel
>
>> > 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 (Displa
>> > yChannel *display);
>> > +void stream_stop (Displa
>> > yChannel *display,
>> > + Stream
>> > *stream);
>> > +void stream_unref (Displa
>> > yChannel *display,
>> > + Stream
>> > *stream);
>> > +void stream_agent_unref (Displa
>> > yChannel *display,
>> > + Stream
>> > Agent *agent);
>> > +void stream_agent_stats_print (Stream
>> > Agent *agent);
>> >
>> > #endif /* STREAM_H */
>> > --
>> > 2.4.3
>> >
>> > _______________________________________________
>> > Spice-devel mailing list
>> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Best Regards,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel