Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel
> >
> > 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);
> > intdisplay_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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel
>
> 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);
> intdisplay_channel_wait_for_migrate_data
> (DisplayChannel *display);
...
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel
On Thu, 2016-09-15 at 11:22 -0500, Jonathon Jongsma wrote:
> 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/dcc-send.c b/server/dcc-send.c
> index 521e6a2..0afa25c 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -94,7 +94,7 @@ static int
> is_surface_area_lossy(DisplayChannelClient *dcc, uint32_t
> surface_id,
> QRegion lossy_region;
> DisplayChannel *display = DCC_TO_DC(dcc);
>
> -spice_return_val_if_fail(validate_surface(display, surface_id),
> FALSE);
> +spice_return_val_if_fail(display_channel_validate_surface(displ
> ay, surface_id), FALSE);
>
> surface = &display->surfaces[surface_id];
> surface_lossy_region = &dcc->priv-
> >surface_client_lossy_region[surface_id];
> @@ -414,7 +414,7 @@ static FillBitsType
> fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> RedSurface *surface;
>
> surface_id = simage->u.surface.surface_id;
> -if (!validate_surface(display, surface_id)) {
> +if (!display_channel_validate_surface(display, surface_id))
> {
> spice_warning("Invalid surface in
> SPICE_IMAGE_TYPE_SURFACE");
> pthread_mutex_unlock(&dcc->priv->pixmap_cache->lock);
> return FILL_BITS_TYPE_SURFACE;
> @@ -1706,7 +1706,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
> return FALSE;
> }
>
> -StreamAgent *agent = &dcc->priv-
> >stream_agents[get_stream_id(display, stream)];
> +StreamAgent *agent = &dcc->priv-
> >stream_agents[display_channel_get_stream_id(display, stream)];
> uint64_t time_now = spice_get_monotonic_time_ns();
>
> if (!dcc->priv->use_video_encoder_rate_control) {
> @@ -1752,7 +1752,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
>
> red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DATA, NULL);
>
> -stream_data.base.id = get_stream_id(display, stream);
> +stream_data.base.id =
> display_channel_get_stream_id(display, stream);
> stream_data.base.multi_media_time = frame_mm_time;
> stream_data.data_size = outbuf->size;
>
> @@ -1762,7 +1762,7 @@ static int
> red_marshall_stream_data(RedChannelClient *rcc,
>
> red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, NULL);
>
> -stream_data.base.id = get_stream_id(display, stream);
> +stream_data.base.id =
> display_channel_get_stream_id(display, stream);
> stream_data.base.multi_media_time = frame_mm_time;
> stream_data.data_size = outbuf->size;
> stream_data.width = copy->src_area.right - copy-
> >src_area.left;
> @@ -2171,7 +2171,7 @@ static void
> marshall_stream_start(RedChannelClient *rcc,
> SpiceClipRects clip_rects;
>
> stream_create.surface_id = 0;
> -stream_create.id = get_stream_id(DCC_TO_DC(dcc), stream);
> +stream_create.id =
> display_channel_get_stream_id(DCC_TO_DC(dcc), stream);
> stream_create.flags = stream->top_down ?
> SPICE_STREAM_FLAGS_TOP_DOWN : 0;
> stream_create.codec_type = agent->video_encoder->codec_type;
>
> @@ -2207,7 +2207,7 @@ static void
> marshall_stream_clip(RedChannelClient *rcc,
> red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_CLIP, &item->base);
> SpiceMsgDisplayStreamClip stream_clip;
>
> -stream_clip.id = get_stream_id(DCC_TO_DC(dcc), agent->stream);
> +stream_clip.id = display_channel_get_stream_id(DCC_TO_DC(dcc),
> agent->stream);
> stream_clip.clip.type = item->clip_type;
> stream_clip.clip.rects = item->rects;
>
> @@ -2221,7 +2221,7 @@ static void
> marshall_stream_end(RedChannelClient *rcc,
> SpiceMsgDisplayStreamDestroy destroy;
>
> red_channel_client_init_send_data(rcc,
> SPICE_MSG_DISPLAY_STREAM_DESTROY, NULL);
> -destroy.id = get_stream_id(DCC_TO_DC(dcc), agent->stream);
> +destroy.id = display_channel_get_stream_id(DCC_TO_DC(dcc),
> agent->stream);
> stream_agent_stop(agent);
> spice_marshall_msg_display_stream_destroy(base_marshaller,
> &destroy);
> }
> 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_h
