Re: [PATCH spice-gtk 2/5] widget: move canvas related data in its own structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

----- Original Message -----
> Hi,
> 
> On Tue, 2016-05-24 at 21:31 +0200, Marc-André Lureau wrote:
> > Group canvas related data to a sub-structure.
> I am ok with groupping part of the patch, but there are some changes which
> should be documented & go in a separate patch.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > ---
> >  src/spice-widget-cairo.c | 40 +++++++++++++++--------------
> >  src/spice-widget-priv.h  | 17 ++++++-------
> >  src/spice-widget.c       | 65
> >  +++++++++++++++++++++++++--------------------
> > ---
> >  3 files changed, 64 insertions(+), 58 deletions(-)
> > 
> > diff --git a/src/spice-widget-cairo.c b/src/spice-widget-cairo.c
> > index 5d93a9f..7f5f79d 100644
> > --- a/src/spice-widget-cairo.c
> > +++ b/src/spice-widget-cairo.c
> > @@ -27,22 +27,24 @@ int spicex_image_create(SpiceDisplay *display)
> >  {
> >      SpiceDisplayPrivate *d = display->priv;
> >  
> > -    if (d->ximage != NULL)
> > +    if (d->canvas.surface != NULL)
> >          return 0;
> >  
> > -    if (d->format == SPICE_SURFACE_FMT_16_555 ||
> > -        d->format == SPICE_SURFACE_FMT_16_565) {
> > -        d->convert = TRUE;
> > -        d->data = g_malloc0(d->area.width * d->area.height * 4);
> > +    if (d->canvas.format == SPICE_SURFACE_FMT_16_555 ||
> > +        d->canvas.format == SPICE_SURFACE_FMT_16_565) {
> > +        d->canvas.convert = TRUE;
> > +        d->canvas.data = g_malloc0(d->area.width * d->area.height * 4);
> >  
> > -        d->ximage = cairo_image_surface_create_for_data
> > -            (d->data, CAIRO_FORMAT_RGB24, d->area.width, d->area.height,
> > d-
> > >area.width * 4);
> > +        d->canvas.surface = cairo_image_surface_create_for_data
> > +            (d->canvas.data, CAIRO_FORMAT_RGB24,
> > +             d->area.width, d->area.height, d->area.width * 4);
> >  
> >      } else {
> > -        d->convert = FALSE;
> > +        d->canvas.convert = FALSE;
> >  
> > -        d->ximage = cairo_image_surface_create_for_data
> > -            (d->data, CAIRO_FORMAT_RGB24, d->width, d->height, d->stride);
> > +        d->canvas.surface = cairo_image_surface_create_for_data
> > +            (d->canvas.data, CAIRO_FORMAT_RGB24,
> > +             d->canvas.width, d->canvas.height, d->canvas.stride);
> >      }
> >  
> >      return 0;
> > @@ -53,10 +55,10 @@ void spicex_image_destroy(SpiceDisplay *display)
> >  {
> >      SpiceDisplayPrivate *d = display->priv;
> >  
> > -    g_clear_pointer(&d->ximage, cairo_surface_destroy);
> > -    if (d->convert)
> > -        g_clear_pointer(&d->data, g_free);
> > -    d->convert = FALSE;
> > +    g_clear_pointer(&d->canvas.surface, cairo_surface_destroy);
> > +    if (d->canvas.convert)
> > +        g_clear_pointer(&d->canvas.data, g_free);
> > +    d->canvas.convert = FALSE;
> >  }
> >  
> >  G_GNUC_INTERNAL
> > @@ -70,6 +72,8 @@ void spicex_draw_event(SpiceDisplay *display, cairo_t
> > *cr)
> >      int ww, wh;
> >      int w, h;
> >  
> > +    g_return_if_fail(d->canvas.surface != NULL);
> > +
> not related, it should go in a different patch

Ok, I'll actually remove it, as it is no longer needed, as you pointed out.

> 
> if it is always nonnull, then ...
> >      spice_display_get_scaling(display, &s, &x, &y, &w, &h);
> >  
> >      ww = gtk_widget_get_allocated_width(GTK_WIDGET(display));
> > @@ -85,7 +89,7 @@ void spicex_draw_event(SpiceDisplay *display, cairo_t
> > *cr)
> >      /* Optionally cut out the inner area where the pixmap
> >         will be drawn. This avoids 'flashing' since we're
> >         not double-buffering. */
> > -    if (d->ximage) {
> > +    if (d->canvas.surface) {
> ... this check is not needed

Yep

> >          rect.x = x;
> >          rect.y = y;
> >          rect.width = w;
> > @@ -103,13 +107,13 @@ void spicex_draw_event(SpiceDisplay *display, cairo_t
> > *cr)
> >      cairo_fill(cr);
> >  
> >      /* Draw the display */
> > -    if (d->ximage) {
> > +    if (d->canvas.surface) {
> >          cairo_translate(cr, x, y);
> >          cairo_rectangle(cr, 0, 0, w, h);
> >          cairo_scale(cr, s, s);
> > -        if (!d->convert)
> > +        if (!d->canvas.convert)
> >              cairo_translate(cr, -d->area.x, -d->area.y);
> > -        cairo_set_source_surface(cr, d->ximage, 0, 0);
> > +        cairo_set_source_surface(cr, d->canvas.surface, 0, 0);
> >          cairo_fill(cr);
> >  
> >          if (d->mouse_mode == SPICE_MOUSE_MODE_SERVER &&
> > diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
> > index 50d5ea1..266cc87 100644
> > --- a/src/spice-widget-priv.h
> > +++ b/src/spice-widget-priv.h
> > @@ -66,23 +66,22 @@ struct _SpiceDisplayPrivate {
> >      /* state */
> >      gboolean                ready;
> >      gboolean                monitor_ready;
> > -    enum SpiceSurfaceFmt    format;
> > -    gint                    width, height, stride;
> > -    gpointer                data_origin; /* the original display image
> > data
> > */
> > -    gpointer                data; /* converted if necessary to 32 bits */
> > -
> > +    struct {
> > +        enum SpiceSurfaceFmt    format;
> > +        gint                    width, height, stride;
> > +        gpointer                data_origin; /* the original display image
> > data */
> > +        gpointer                data; /* converted if necessary to 32 bits
> > */
> > +        bool                    convert;
> > +        cairo_surface_t         *surface;
> > +    } canvas;
> >      GdkRectangle            area;
> >      /* window border */
> >      gint                    ww, wh, mx, my;
> >  
> > -    bool                    convert;
> >      gboolean                allow_scaling;
> >      gboolean                only_downscale;
> >      gboolean                disable_inputs;
> >  
> > -    /* TODO: make a display object instead? */
> > -    cairo_surface_t         *ximage;
> > -
> >      SpiceSession            *session;
> >      SpiceGtkSession         *gtk_session;
> >      SpiceMainChannel        *main;
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index cbca5dc..f788dc1 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -178,7 +178,7 @@ static void scaling_updated(SpiceDisplay *display)
> >      GdkWindow *window = gtk_widget_get_window(GTK_WIDGET(display));
> >  
> >      recalc_geometry(GTK_WIDGET(display));
> > -    if (d->ximage && window) { /* if not yet shown */
> > +    if (d->canvas.surface && window) { /* if not yet shown */
> >          gtk_widget_queue_draw(GTK_WIDGET(display));
> >      }
> >      update_size_request(display);
> > @@ -320,7 +320,7 @@ void
> > spice_display_widget_update_monitor_area(SpiceDisplay
> > *display)
> >  whole:
> >      g_clear_pointer(&monitors, g_array_unref);
> >      /* by display whole surface */
> > -    update_area(display, 0, 0, d->width, d->height);
> > +    update_area(display, 0, 0, d->canvas.width, d->canvas.height);
> >      set_monitor_ready(display, true);
> >  }
> >  
> > @@ -1144,34 +1144,34 @@ static void recalc_geometry(GtkWidget *widget)
> >  static gboolean do_color_convert(SpiceDisplay *display, GdkRectangle *r)
> >  {
> >      SpiceDisplayPrivate *d = display->priv;
> > -    guint32 *dest = d->data;
> > -    guint16 *src = d->data_origin;
> > +    guint32 *dest = d->canvas.data;
> > +    guint16 *src = d->canvas.data_origin;
> >      gint x, y;
> >  
> >      g_return_val_if_fail(r != NULL, false);
> > -    g_return_val_if_fail(d->format == SPICE_SURFACE_FMT_16_555 ||
> > -                         d->format == SPICE_SURFACE_FMT_16_565, false);
> > +    g_return_val_if_fail(d->canvas.format == SPICE_SURFACE_FMT_16_555 ||
> > +                         d->canvas.format == SPICE_SURFACE_FMT_16_565,
> > false);
> >  
> > -    src += (d->stride / 2) * r->y + r->x;
> > +    src += (d->canvas.stride / 2) * r->y + r->x;
> >      dest += d->area.width * (r->y - d->area.y) + (r->x - d->area.x);
> >  
> > -    if (d->format == SPICE_SURFACE_FMT_16_555) {
> > +    if (d->canvas.format == SPICE_SURFACE_FMT_16_555) {
> >          for (y = 0; y < r->height; y++) {
> >              for (x = 0; x < r->width; x++) {
> >                  dest[x] = CONVERT_0555_TO_0888(src[x]);
> >              }
> >  
> >              dest += d->area.width;
> > -            src += d->stride / 2;
> > +            src += d->canvas.stride / 2;
> >          }
> > -    } else if (d->format == SPICE_SURFACE_FMT_16_565) {
> > +    } else if (d->canvas.format == SPICE_SURFACE_FMT_16_565) {
> >          for (y = 0; y < r->height; y++) {
> >              for (x = 0; x < r->width; x++) {
> >                  dest[x] = CONVERT_0565_TO_0888(src[x]);
> >              }
> >  
> >              dest += d->area.width;
> > -            src += d->stride / 2;
> > +            src += d->canvas.stride / 2;
> >          }
> >      }
> >  
> > @@ -1224,10 +1224,9 @@ static gboolean draw_event(GtkWidget *widget,
> > cairo_t
> > *cr, gpointer data)
> >      }
> >  #endif
> >  
> > -    if (d->mark == 0 || d->data == NULL ||
> > +    if (d->mark == 0 || d->canvas.data == NULL ||
> >          d->area.width == 0 || d->area.height == 0)
> >          return false;
> > -    g_return_val_if_fail(d->ximage != NULL, false);
> not related
> >  
> >      spicex_draw_event(display, cr);
> >      update_mouse_pointer(display);
> > @@ -1957,7 +1956,7 @@ static void update_image(SpiceDisplay *display)
> >      SpiceDisplayPrivate *d = display->priv;
> >  
> >      spicex_image_create(display);
> > -    if (d->convert)
> > +    if (d->canvas.convert)
> >          do_color_convert(display, &d->area);
> >  }
> >  
> > @@ -2306,11 +2305,15 @@ static void update_area(SpiceDisplay *display,
> >  #endif
> >      {
> >          primary = (GdkRectangle) {
> > -            .width = d->width,
> > -            .height = d->height
> > +            .width = d->canvas.width,
> > +            .height = d->canvas.height
> >          };
> >      }
> >  
> > +    SPICE_DEBUG("update area, primary: %dx%d, area: +%d+%d %dx%d",
> > +                primary.width, primary.height,
> > +                d->area.x, d->area.y, d->area.width, d->area.height);
> > +
> >      SPICE_DEBUG("primary: %dx%d", primary.width, primary.height);
> the debug message can be improved (not duplicated imo) in a different patch
> 

Not really needed, I wonder how it slipped in.
 
> Thanks,
> Pavel
> >      if (!gdk_rectangle_intersect(&primary, &d->area, &d->area)) {
> >          SPICE_DEBUG("The monitor area is not intersecting primary
> >          surface");
> > @@ -2340,11 +2343,11 @@ static void primary_create(SpiceChannel *channel,
> > gint
> > format,
> >      SpiceDisplay *display = data;
> >      SpiceDisplayPrivate *d = display->priv;
> >  
> > -    d->format = format;
> > -    d->stride = stride;
> > -    d->width = width;
> > -    d->height = height;
> > -    d->data_origin = d->data = imgdata;
> > +    d->canvas.format = format;
> > +    d->canvas.stride = stride;
> > +    d->canvas.width = width;
> > +    d->canvas.height = height;
> > +    d->canvas.data_origin = d->canvas.data = imgdata;
> >  
> >      spice_display_widget_update_monitor_area(display);
> >  }
> > @@ -2355,11 +2358,11 @@ static void primary_destroy(SpiceChannel *channel,
> > gpointer data)
> >      SpiceDisplayPrivate *d = display->priv;
> >  
> >      spicex_image_destroy(display);
> > -    d->width  = 0;
> > -    d->height = 0;
> > -    d->stride = 0;
> > -    d->data = NULL;
> > -    d->data_origin = NULL;
> > +    d->canvas.width  = 0;
> > +    d->canvas.height = 0;
> > +    d->canvas.stride = 0;
> > +    d->canvas.data = NULL;
> > +    d->canvas.data_origin = NULL;
> >      set_monitor_ready(display, false);
> >  }
> >  
> > @@ -2403,7 +2406,7 @@ static void invalidate(SpiceChannel *channel,
> >      if (!gdk_rectangle_intersect(&rect, &d->area, &rect))
> >          return;
> >  
> > -    if (d->convert)
> > +    if (d->canvas.convert)
> >          do_color_convert(display, &rect);
> >  
> >      spice_display_get_scaling(display, &s,
> > @@ -2874,12 +2877,12 @@ GdkPixbuf *spice_display_get_pixbuf(SpiceDisplay
> > *display)
> >          int x, y;
> >  
> >          /* TODO: ensure d->data has been exposed? */
> > -        g_return_val_if_fail(d->data != NULL, NULL);
> > +        g_return_val_if_fail(d->canvas.data != NULL, NULL);
> >          data = g_malloc0(d->area.width * d->area.height * 3);
> > -        src = d->data;
> > +        src = d->canvas.data;
> >          dest = data;
> >  
> > -        src += d->area.y * d->stride + d->area.x * 4;
> > +        src += d->area.y * d->canvas.stride + d->area.x * 4;
> >          for (y = 0; y < d->area.height; ++y) {
> >              for (x = 0; x < d->area.width; ++x) {
> >                  dest[0] = src[x * 4 + 2];
> > @@ -2887,7 +2890,7 @@ GdkPixbuf *spice_display_get_pixbuf(SpiceDisplay
> > *display)
> >                  dest[2] = src[x * 4 + 0];
> >                  dest += 3;
> >              }
> > -            src += d->stride;
> > +            src += d->canvas.stride;
> >          }
> >          pixbuf = gdk_pixbuf_new_from_data(data, GDK_COLORSPACE_RGB, false,
> >                                            8, d->area.width,
> >                                            d->area.height,
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]