[Spice-devel] [PATCH] worker: move dcc_add_surface_area_image
Frediano Ziglio
fziglio at redhat.com
Fri Nov 20 05:40:40 PST 2015
>
> Reduced diff (I'm getting good at it!):
>
>
> --- before.c 2015-11-20 13:30:01.465786520 +0000
> +++ after.c 2015-11-20 13:27:42.931050275 +0000
> @@ -128,20 +128,21 @@
> spice_bitmap_utils.c \
> spice_server_utils.h \
> spice_image_cache.h \
> spice_image_cache.c \
> pixmap-cache.h \
> pixmap-cache.c \
> tree.h \
> tree.c \
> spice-bitmap-utils.h \
> spice-bitmap-utils.c \
> + utils.c \
> utils.h \
> stream.c \
> stream.h \
> dcc.c \
> dcc.h \
> display-limits.h \
> dcc-encoders.c \
> dcc-encoders.h \
> $(NULL)
>
> @@ -246,21 +247,21 @@
> RedChannel *channel = RED_CHANNEL(display);
> RedSurface *surface = &display->surfaces[surface_id];
> SpiceCanvas *canvas = surface->context.canvas;
> ImageItem *item;
> int stride;
> int width;
> int height;
> int bpp;
> int all_set;
>
> - spice_assert(area);
> + spice_return_val_if_fail(area, NULL);
>
> width = area->right - area->left;
> height = area->bottom - area->top;
> bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
> stride = width * bpp;
>
> item = (ImageItem *)spice_malloc_n_m(height, stride, sizeof(ImageItem));
>
> red_channel_pipe_item_init(channel, &item->link, PIPE_ITEM_TYPE_IMAGE);
>
I'll ditch this change. area is never NULL and considered a internal error.
> @@ -284,28 +285,26 @@
> if (!is_primary_surface(display, surface_id) &&
> item->image_format == SPICE_BITMAP_FMT_32BIT &&
> rgb32_data_has_alpha(item->width, item->height, item->stride,
> item->data, &all_set)) {
> if (all_set) {
> item->image_flags |= SPICE_IMAGE_FLAGS_HIGH_BITS_SET;
> } else {
> item->image_format = SPICE_BITMAP_FMT_RGBA;
> }
> }
>
> - if (!pos) {
> - red_pipe_add_image_item(dcc, item);
> + if (pos) {
> + red_channel_client_pipe_add_after(RED_CHANNEL_CLIENT(dcc),
> &item->link, pos);
> } else {
> - red_pipe_add_image_item_after(dcc, item, pos);
> + red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &item->link);
> }
>
> - release_image_item(item);
> -
> return item;
> }
>
This seems odd but the two functions called before were wrapped which incremented
always (if dcc is not NULL but at this point must be) refs so the release would
be now a no-op.
> void dcc_push_surface_image(DisplayChannelClient *dcc, int surface_id)
> {
> DisplayChannel *display;
> SpiceRect area;
> RedSurface *surface;
>
> if (!dcc) {
> @@ -1566,62 +1565,39 @@
> return (worker->display_channel && red_channel_is_connected(
> &worker->display_channel->common.base));
> }
>
> static int cursor_is_connected(RedWorker *worker)
> {
> return worker->cursor_channel &&
> red_channel_is_connected(RED_CHANNEL(worker->cursor_channel));
> }
>
> -static inline PipeItem *red_pipe_get_tail(DisplayChannelClient *dcc)
> +static PipeItem *dcc_get_tail(DisplayChannelClient *dcc)
> {
> - if (!dcc) {
> - return NULL;
> - }
> -
> return (PipeItem*)ring_get_tail(&RED_CHANNEL_CLIENT(dcc)->pipe);
> }
>
No reason for this extra check. I'll remove it.
> void red_pipes_remove_drawable(Drawable *drawable)
> {
> DrawablePipeItem *dpi;
> RingItem *item, *next;
>
> RING_FOREACH_SAFE(item, next, &drawable->pipes) {
> dpi = SPICE_CONTAINEROF(item, DrawablePipeItem, base);
> if (pipe_item_is_linked(&dpi->dpi_pipe_item)) {
> red_channel_client_pipe_remove_and_release(RED_CHANNEL_CLIENT(dpi->dcc),
> &dpi->dpi_pipe_item);
> }
> }
> }
>
> -static inline void red_pipe_add_image_item(DisplayChannelClient *dcc,
> ImageItem *item)
> -{
> - if (!dcc) {
> - return;
> - }
> - item->refs++;
> - red_channel_client_pipe_add(RED_CHANNEL_CLIENT(dcc), &item->link);
> -}
> -
> -static inline void red_pipe_add_image_item_after(DisplayChannelClient *dcc,
> ImageItem *item,
> - PipeItem *pos)
> -{
> - if (!dcc) {
> - return;
> - }
> - item->refs++;
> - red_channel_client_pipe_add_after(RED_CHANNEL_CLIENT(dcc), &item->link,
> pos);
> -}
> -
These are the wrappers (see my comment above).
> static void release_image_item(ImageItem *item)
> {
> if (!--item->refs) {
> free(item);
> }
> }
>
> static void upgrade_item_unref(DisplayChannel *display, UpgradeItem *item)
> {
> if (--item->refs != 0)
> @@ -3624,42 +3600,42 @@
> }
> }
>
> if (!sync_rendered) {
> // pushing the pipe item back to the pipe
> dcc_add_drawable(dcc, item, TRUE);
> // the surfaces areas will be sent as DRAW_COPY commands, that
> // will be executed before the current drawable
> for (i = 0; i < num_deps; i++) {
> dcc_add_surface_area_image(dcc, deps_surfaces_ids[i],
> deps_areas[i],
> - red_pipe_get_tail(dcc), FALSE);
> + dcc_get_tail(dcc), FALSE);
>
> }
> } else {
> int drawable_surface_id[1];
> SpiceRect *drawable_bbox[1];
>
> drawable_surface_id[0] = drawable->surface_id;
> drawable_bbox[0] = &drawable->bbox;
>
> // check if the other rendered images in the pipe have updated the
> drawable bbox
> if (pipe_rendered_drawables_intersect_with_areas(dcc,
> drawable_surface_id,
> drawable_bbox,
> 1)) {
> red_pipe_replace_rendered_drawables_with_images(dcc,
> drawable->surface_id,
> &drawable->bbox);
> }
>
> dcc_add_surface_area_image(dcc, drawable->surface_id,
> &drawable->bbox,
> - red_pipe_get_tail(dcc), TRUE);
> + dcc_get_tail(dcc), TRUE);
> }
> }
>
> static void red_marshall_qxl_draw_fill(RedChannelClient *rcc,
> SpiceMarshaller *base_marshaller,
> DrawablePipeItem *dpi)
> {
> Drawable *item = dpi->drawable;
> RedDrawable *drawable = item->red_drawable;
> DisplayChannelClient *dcc = RCC_TO_DCC(rcc);
> @@ -7766,21 +7742,44 @@
>
> return RED_CHANNEL(worker->display_channel);
> }
>
> clockid_t red_worker_get_clockid(RedWorker *worker)
> {
> spice_return_val_if_fail(worker, 0);
>
> return worker->clockid;
> }
> -static int rgb32_data_has_alpha(int width, int height, size_t stride,
> +/*
> + Copyright (C) 2009-2015 Red Hat, Inc.
> +
> + This library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + This library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> +*/
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <glib.h>
> +#include "utils.h"
> +
> +int rgb32_data_has_alpha(int width, int height, size_t stride,
> uint8_t *data, int *all_set_out)
> {
> uint32_t *line, *end, alpha;
> int has_alpha;
>
> has_alpha = FALSE;
> while (height-- > 0) {
> line = (uint32_t *)data;
> end = line + width;
> data += stride;
> @@ -7813,25 +7812,29 @@
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> Lesser General Public License for more details.
>
> You should have received a copy of the GNU Lesser General Public
> License along with this library; if not, see
> <http://www.gnu.org/licenses/>.
> */
> #ifndef UTILS_H_
> # define UTILS_H_
>
> +#include <stdint.h>
> #include <time.h>
> #include <stdint.h>
>
> typedef int64_t red_time_t;
>
I'll remove this double include. Probably the result of some wrong rebase.
> /* FIXME: consider g_get_monotonic_time (), but in microseconds */
> static inline red_time_t red_get_monotonic_time(void)
> {
> struct timespec time;
>
> clock_gettime(CLOCK_MONOTONIC, &time);
> return (red_time_t) time.tv_sec * (1000 * 1000 * 1000) + time.tv_nsec;
> }
>
> +int rgb32_data_has_alpha(int width, int height, size_t stride,
> + uint8_t *data, int *all_set_out);
> +
> #endif /* UTILS_H_ */
Frediano
More information about the Spice-devel
mailing list