Re: [PATCH 01/11] worker: Move stream functions to stream.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Mon, Nov 16, 2015 at 12:06 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > From: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> > ---
> >  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?

> 
> > +
> > +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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Best Regards,
> 

I hope to have sum up every request.

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]