[Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel

[Frediano Ziglio]

> 
> Add a couple new functions to the header so that they can be called by
> other objects rather than poking into the internals of the struct.
> ---
>  server/dcc-send.c        | 16 +++++------
>  server/display-channel.c | 71
>  ++++++++++++++++++++++++++++++++++++++++++++----
>  server/display-channel.h | 37 +++++++++++++------------
>  server/red-worker.c      | 44 ++++++------------------------
>  server/stream.c          | 18 ++++++------
>  5 files changed, 109 insertions(+), 77 deletions(-)
> 
....
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 108e69b..56bb029 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -203,6 +203,13 @@ void display_channel_surface_unref(DisplayChannel
> *display, uint32_t surface_id)
>      spice_warn_if_fail(ring_is_empty(&surface->depend_on_me));
>  }
>  
> +/* TODO: perhaps rename to "ready" or "realized" ? */
> +bool display_channel_surface_has_canvas(DisplayChannel *display,
> +                                        uint32_t surface_id)
> +{
> +    return display->surfaces[surface_id].context.canvas != NULL;
> +}
> +

I honestly prefer bool and true/false but we decided to use gboolean and not bool.
This function is mainly doing the same think of validate_surface without
the logging stuff.
Perhaps adding a flag "warnings" to validate_surface is enough?


...

> diff --git a/server/display-channel.h b/server/display-channel.h
> index 7b71480..022cd93 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -208,10 +208,17 @@ struct DisplayChannel {
>      ImageEncoderSharedData encoder_shared_data;
>  };
>  
> -static inline int get_stream_id(DisplayChannel *display, Stream *stream)
> -{
> -    return (int)(stream - display->streams_buf);
> -}
> +
> +#define FOREACH_DCC(channel, _link, _next, _data)                   \
> +    for (_link = (channel ? RED_CHANNEL(channel)->clients : NULL), \
> +         _next = (_link ? _link->next : NULL), \
> +         _data = (_link ? _link->data : NULL); \
> +         _link; \
> +         _link = _next, \
> +         _next = (_link ? _link->next : NULL), \
> +         _data = (_link ? _link->data : NULL))
> +
> +int display_channel_get_stream_id(DisplayChannel *display, Stream *stream);
>  
>  typedef struct RedSurfaceDestroyItem {
>      RedPipeItem pipe_item;
> @@ -258,6 +265,8 @@ void
> display_channel_compress_stats_print      (const Disp
>  void                       display_channel_compress_stats_reset
>  (DisplayChannel *display);
>  void                       display_channel_surface_unref
>  (DisplayChannel *display,
>                                                                        uint32_t
>                                                                        surface_id);
> +bool                       display_channel_surface_has_canvas
> (DisplayChannel *display,
> +

Although this is consistent with the rest it does not follow our style.

> uint32_t
> surface_id);
>  void                       display_channel_current_flush
>  (DisplayChannel *display,
>                                                                        int
>                                                                        surface_id);
>  int                        display_channel_wait_for_migrate_data
>  (DisplayChannel *display);
...

Frediano