[Spice-devel] [PATCH v2 1/2] Improve encapsulation of DisplayChannel
Pavel Grunt
pgrunt at redhat.com
Fri Sep 16 08:35:44 UTC 2016
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_has_canvas(DisplayChannel *display,
> + uint32_t surface_id)
> +{
> + return display->surfaces[surface_id].context.canvas != NULL;
> +}
> +
> static void streams_update_visible_region(DisplayChannel *display,
> Drawable *drawable)
> {
> Ring *ring;
> @@ -232,7 +239,7 @@ static void
> streams_update_visible_region(DisplayChannel *display, Drawable *dra
> }
>
> FOREACH_CLIENT(display, link, next, dcc) {
> - agent = dcc_get_stream_agent(dcc,
> get_stream_id(display, stream));
> + agent = dcc_get_stream_agent(dcc,
> display_channel_get_stream_id(display, stream));
>
> if (region_intersects(&agent->vis_region, &drawable-
> >tree_item.base.rgn)) {
> region_exclude(&agent->vis_region, &drawable-
> >tree_item.base.rgn);
> @@ -955,7 +962,7 @@ static int validate_drawable_bbox(DisplayChannel
> *display, RedDrawable *drawable
> /* surface_id must be validated before calling into
> * validate_drawable_bbox
> */
> - if (!validate_surface(display, drawable->surface_id)) {
> + if (!display_channel_validate_surface(display, drawable-
> >surface_id)) {
> return FALSE;
> }
> context = &display->surfaces[surface_id].context;
> @@ -998,7 +1005,7 @@ static Drawable
> *display_channel_get_drawable(DisplayChannel *display, uint8_t e
> }
> for (x = 0; x < 3; ++x) {
> if (red_drawable->surface_deps[x] != -1
> - && !validate_surface(display, red_drawable-
> >surface_deps[x])) {
> + && !display_channel_validate_surface(display,
> red_drawable->surface_deps[x])) {
> return NULL;
> }
> }
> @@ -1669,7 +1676,7 @@ void display_channel_update(DisplayChannel
> *display,
> SpiceRect rect;
> RedSurface *surface;
>
> - spice_return_if_fail(validate_surface(display, surface_id));
> + spice_return_if_fail(display_channel_validate_surface(display,
> surface_id));
>
> red_get_rect_ptr(&rect, area);
> display_channel_draw(display, &rect, surface_id);
> @@ -1712,7 +1719,7 @@ static void
> display_channel_destroy_surface(DisplayChannel *display, uint32_t su
>
> void display_channel_destroy_surface_wait(DisplayChannel *display,
> uint32_t surface_id)
> {
> - if (!validate_surface(display, surface_id))
> + if (!display_channel_validate_surface(display, surface_id))
> return;
> if (!display->surfaces[surface_id].context.canvas)
> return;
> @@ -1882,7 +1889,7 @@ static SpiceCanvas
> *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
> {
> DisplayChannel *display = SPICE_CONTAINEROF(surfaces,
> DisplayChannel, image_surfaces);
>
> - spice_return_val_if_fail(validate_surface(display, surface_id),
> NULL);
> + spice_return_val_if_fail(display_channel_validate_surface(displ
> ay, surface_id), NULL);
>
> return display->surfaces[surface_id].context.canvas;
> }
> @@ -2038,3 +2045,55 @@ void
> display_channel_gl_draw_done(DisplayChannel *display)
> {
> set_gl_draw_async_count(display, display->gl_draw_async_count -
> 1);
> }
> +
> +int display_channel_get_stream_id(DisplayChannel *display, Stream
> *stream)
> +{
> + return (int)(stream - display->streams_buf);
> +}
> +
> +gboolean display_channel_validate_surface(DisplayChannel *display,
> uint32_t surface_id)
> +{
> + if SPICE_UNLIKELY(surface_id >= display->n_surfaces) {
> + spice_warning("invalid surface_id %u", surface_id);
> + return 0;
> + }
> + if (!display->surfaces[surface_id].context.canvas) {
> + spice_warning("canvas address is %p for %d (and is
> NULL)\n",
> + &(display->surfaces[surface_id].context.canvas),
> surface_id);
> + spice_warning("failed on %d", surface_id);
> + return 0;
> + }
> + return 1;
should be changed to gboolean values (it is in the current code, so
not issue of this patch)
> +}
> +
> +void display_channel_update_monitors_config(DisplayChannel
> *display,
> + QXLMonitorsConfig
> *config,
> + uint16_t count,
> uint16_t max_allowed)
> +{
> +
> + if (display->monitors_config)
> + monitors_config_unref(display->monitors_config);
> +
> + display->monitors_config =
> + monitors_config_new(config->heads, count, max_allowed);
> +}
> +
> +void set_monitors_config_to_primary(DisplayChannel *display)
> +{
> + DrawContext *context = &display->surfaces[0].context;
> + QXLHead head = { 0, };
> +
> + spice_return_if_fail(display->surfaces[0].context.canvas);
> +
> + if (display->monitors_config)
> + monitors_config_unref(display->monitors_config);
> +
> + head.width = context->width;
> + head.height = context->height;
> + display->monitors_config = monitors_config_new(&head, 1, 1);
> +}
> +
> +void display_channel_reset_image_cache(DisplayChannel *self)
> +{
> + image_cache_reset(&self->image_cache);
> +}
> 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,
> +
> uint32_t surface_id);
> void display_channel_current_flush
> (DisplayChannel *display,
>
> int surface_id);
> int display_channel_wait_for_migrate_data
> (DisplayChannel *display);
> @@ -281,20 +290,12 @@
> void display_channel_gl_draw
> (DisplayCha
>
> SpiceMsgDisplayGlDraw *draw);
> void display_channel_gl_draw_done
> (DisplayChannel *display);
>
> -static inline int validate_surface(DisplayChannel *display,
> uint32_t surface_id)
> -{
> - if SPICE_UNLIKELY(surface_id >= display->n_surfaces) {
> - spice_warning("invalid surface_id %u", surface_id);
> - return 0;
> - }
> - if (!display->surfaces[surface_id].context.canvas) {
> - spice_warning("canvas address is %p for %d (and is
> NULL)\n",
> - &(display->surfaces[surface_id].context.canvas),
> surface_id);
> - spice_warning("failed on %d", surface_id);
> - return 0;
> - }
> - return 1;
> -}
> +void display_channel_update_monitors_config(DisplayChannel
> *display, QXLMonitorsConfig *config,
> + uint16_t count,
> uint16_t max_allowed);
> +void set_monitors_config_to_primary(DisplayChannel *display);
> +
> +gboolean display_channel_validate_surface(DisplayChannel *display,
> uint32_t surface_id);
> +void display_channel_reset_image_cache(DisplayChannel *self);
>
> static inline int is_equal_path(SpicePath *path1, SpicePath *path2)
> {
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 590412b..6df6420 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -247,7 +247,7 @@ static int red_process_display(RedWorker
> *worker, int *ring_is_empty)
> &update, ext_cmd.cmd.data)) {
> break;
> }
> - if (!validate_surface(worker->display_channel,
> update.surface_id)) {
> + if (!display_channel_validate_surface(worker-
> >display_channel, update.surface_id)) {
> spice_warning("Invalid surface in QXL_CMD_UPDATE");
> } else {
> display_channel_draw(worker->display_channel,
> &update.area, update.surface_id);
> @@ -587,18 +587,6 @@ static void handle_dev_destroy_surfaces(void
> *opaque, void *payload)
> cursor_channel_reset(worker->cursor_channel);
> }
>
> -static void display_update_monitors_config(DisplayChannel *display,
> - QXLMonitorsConfig
> *config,
> - uint16_t count, uint16_t
> max_allowed)
> -{
> -
> - if (display->monitors_config)
> - monitors_config_unref(display->monitors_config);
> -
> - display->monitors_config =
> - monitors_config_new(config->heads, count, max_allowed);
> -}
> -
> static void red_worker_push_monitors_config(RedWorker *worker)
> {
> DisplayChannelClient *dcc;
> @@ -609,21 +597,6 @@ static void
> red_worker_push_monitors_config(RedWorker *worker)
> }
> }
>
> -static void set_monitors_config_to_primary(DisplayChannel *display)
> -{
> - DrawContext *context = &display->surfaces[0].context;
> - QXLHead head = { 0, };
> -
> - spice_return_if_fail(display->surfaces[0].context.canvas);
> -
> - if (display->monitors_config)
> - monitors_config_unref(display->monitors_config);
> -
> - head.width = context->width;
> - head.height = context->height;
> - display->monitors_config = monitors_config_new(&head, 1, 1);
> -}
> -
> static void dev_create_primary_surface(RedWorker *worker, uint32_t
> surface_id,
> QXLDevSurfaceCreate surface)
> {
> @@ -689,12 +662,12 @@ static void destroy_primary_surface(RedWorker
> *worker, uint32_t surface_id)
> {
> DisplayChannel *display = worker->display_channel;
>
> - if (!validate_surface(display, surface_id))
> + if (!display_channel_validate_surface(display, surface_id))
> return;
> spice_warn_if_fail(surface_id == 0);
>
> spice_debug(NULL);
> - if (!display->surfaces[surface_id].context.canvas) {
> + if (!display_channel_surface_has_canvas(display, surface_id)) {
> spice_warning("double destroy of primary surface");
> return;
> }
> @@ -704,7 +677,7 @@ static void destroy_primary_surface(RedWorker
> *worker, uint32_t surface_id)
> display_channel_surface_unref(display, 0);
>
> spice_warn_if_fail(ring_is_empty(&display->streams));
> - spice_warn_if_fail(!display-
> >surfaces[surface_id].context.canvas);
> + spice_warn_if_fail(!display_channel_surface_has_canvas(display,
> surface_id));
>
> cursor_channel_reset(worker->cursor_channel);
> }
> @@ -828,9 +801,8 @@ static void handle_dev_reset_cursor(void
> *opaque, void *payload)
> static void handle_dev_reset_image_cache(void *opaque, void
> *payload)
> {
> RedWorker *worker = opaque;
> - DisplayChannel *display = worker->display_channel;
>
> - image_cache_reset(&display->image_cache);
> + display_channel_reset_image_cache(worker->display_channel);
> }
>
> static void handle_dev_destroy_surface_wait_async(void *opaque,
> void *payload)
> @@ -950,9 +922,9 @@ static void
> handle_dev_monitors_config_async(void *opaque, void *payload)
> /* TODO: raise guest bug (requires added QXL interface) */
> return;
> }
> - display_update_monitors_config(worker->display_channel,
> dev_monitors_config,
> - MIN(count, msg->max_monitors),
> - MIN(max_allowed, msg-
> >max_monitors));
> + display_channel_update_monitors_config(worker->display_channel,
> dev_monitors_config,
> + MIN(count, msg-
> >max_monitors),
> + MIN(max_allowed, msg-
> >max_monitors));
> red_worker_push_monitors_config(worker);
> }
>
> diff --git a/server/stream.c b/server/stream.c
> index 4819723..8997c10 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -101,11 +101,11 @@ void stream_stop(DisplayChannel *display,
> Stream *stream)
> 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));
> + spice_debug("stream %d", display_channel_get_stream_id(display,
> stream));
> FOREACH_CLIENT(display, link, next, dcc) {
> StreamAgent *stream_agent;
>
> - stream_agent = dcc_get_stream_agent(dcc,
> get_stream_id(display, stream));
> + stream_agent = dcc_get_stream_agent(dcc,
> display_channel_get_stream_id(display, stream));
> region_clear(&stream_agent->vis_region);
> region_clear(&stream_agent->clip);
> if (stream_agent->video_encoder &&
> dcc_use_video_encoder_rate_control(dcc)) {
> @@ -302,7 +302,7 @@ static void attach_stream(DisplayChannel
> *display, Drawable *drawable, Stream *s
> StreamAgent *agent;
> QRegion clip_in_draw_dest;
>
> - agent = dcc_get_stream_agent(dcc, get_stream_id(display,
> stream));
> + agent = dcc_get_stream_agent(dcc,
> display_channel_get_stream_id(display, stream));
> region_or(&agent->vis_region, &drawable-
> >tree_item.base.rgn);
>
> region_init(&clip_in_draw_dest);
> @@ -349,7 +349,7 @@ static void
> before_reattach_stream(DisplayChannel *display,
> return;
> }
>
> - index = get_stream_id(display, stream);
> + index = display_channel_get_stream_id(display, stream);
> DRAWABLE_FOREACH_DPI_SAFE(stream->current, ring_item, next,
> dpi) {
> dcc = dpi->dcc;
> agent = dcc_get_stream_agent(dcc, index);
> @@ -759,7 +759,7 @@ static VideoEncoder*
> dcc_create_video_encoder(DisplayChannelClient *dcc,
>
> void dcc_create_stream(DisplayChannelClient *dcc, Stream *stream)
> {
> - StreamAgent *agent = dcc_get_stream_agent(dcc,
> get_stream_id(DCC_TO_DC(dcc), stream));
> + StreamAgent *agent = dcc_get_stream_agent(dcc,
> display_channel_get_stream_id(DCC_TO_DC(dcc), stream));
>
> spice_return_if_fail(region_is_empty(&agent->vis_region));
>
> @@ -796,7 +796,7 @@ void dcc_create_stream(DisplayChannelClient
> *dcc, Stream *stream)
> agent->report_id = rand();
> red_pipe_item_init(&report_pipe_item->pipe_item,
> RED_PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPOR
> T);
> - report_pipe_item->stream_id = get_stream_id(DCC_TO_DC(dcc),
> stream);
> + report_pipe_item->stream_id =
> display_channel_get_stream_id(DCC_TO_DC(dcc), stream);
> red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc),
> &report_pipe_item->pipe_item);
> }
> #ifdef STREAM_STATS
> @@ -839,7 +839,7 @@ static void
> dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
> Drawable
> *update_area_limit)
> {
> DisplayChannel *display = DCC_TO_DC(dcc);
> - int stream_id = get_stream_id(display, stream);
> + int stream_id = display_channel_get_stream_id(display, stream);
> StreamAgent *agent = dcc_get_stream_agent(dcc, stream_id);
>
> /* stopping the client from playing older frames at once*/
> @@ -936,12 +936,12 @@ void stream_detach_behind(DisplayChannel
> *display, QRegion *region, Drawable *dr
> item = ring_next(ring, item);
>
> FOREACH_CLIENT(display, link, next, dcc) {
> - StreamAgent *agent = dcc_get_stream_agent(dcc,
> get_stream_id(display, stream));
> + StreamAgent *agent = dcc_get_stream_agent(dcc,
> display_channel_get_stream_id(display, stream));
>
> if (region_intersects(&agent->vis_region, region)) {
> dcc_detach_stream_gracefully(dcc, stream,
> drawable);
> detach = 1;
> - spice_debug("stream %d", get_stream_id(display,
> stream));
> + spice_debug("stream %d",
> display_channel_get_stream_id(display, stream));
> }
> }
> if (detach && stream->current) {
>
Acked-by: Pavel Grunt <pgrunt at redhat.com>
More information about the Spice-devel
mailing list