[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);
> ...
> 

Actually.
Would not be easier to have a 

RedSurface *display_channel_get_surface(DisplayChannel *display, uint32_t surface_id);

which check surface_id and canvas and return the valid surface or NULL?

This at the same time replace validation of surface ids and accessing surfaces
array inside DisplayChannel.

Personally I don't like that much the display_channel_surface_has_canvas as
it gives implementation detail to the user which could ignore the presence
of a "canvas" in the surface.

Frediano