Re: [PATCH] skia: update the source to build with the latest skia

Thanks, LGTM as well.  I verified a build.  Pushed to master.

Bryce

On Mon, Jun 30, 2014 at 05:08:00PM +0530, RAVI NANJUNDAPPA wrote:
> Hi, 
> 
> Please see attached the patch after incorporating review comments from Uli.
> Thanking Uli for the comments.
> Please help me in reviewing this patch.
> 
> Please see further below for views from my side.
> 
> > -----Original Message-----
> > From: cairo [mailto:[email protected]] On Behalf Of RAVI
> > NANJUNDAPPA
> > Sent: Wednesday, June 25, 2014 5:01 PM
> > To: 'Uli Schlachter'; [email protected]
> > Subject: Re: [cairo] [PATCH] skia: update the source to build with the
> latest
> > skia
> > 
> > Hi,
> > 
> > Please my comments inline below.
> > 
> > 
> > > -----Original Message-----
> > > From: cairo [mailto:[email protected]] On Behalf Of Uli
> > > Schlachter
> > > Sent: Wednesday, June 25, 2014 1:29 PM
> > > To: [email protected]
> > > Subject: Re: [cairo] [PATCH] skia: update the source to build with the
> > latest
> > > skia
> > >
> > > Hi,
> > >
> > > since Bruce (:-P) wants me to complain, here are my complaints:
> > >
> > > Is it somehow possible to check the required minimum Skia version
> > > during configure? Mostly just for documentation purpose?
> > 
> > AFAIK, the latest skia is still at 1.0.0 version (as mentioned in
> > include/core/SkTypes.h file skia source dir).
> > Yes, we can check for the minimum required Skia version during configure.
> > But there is a problem. When we build skia source (either Debug or Release
> > build) it generates only libskia.so and not libskia.so.1.0 or
> libskia.so.1.0.0. And
> > also we don't get the skia binaries in the pkg format (for ex: no
> libskia-
> > 1.0.0.deb/rpm for run-time pkg, or libskia-dev-1.0.0.deb/rpm for
> > development pkgs).
> > In such a case, checking for the skia version in the standard way (using
> > PKG_CHECK_MODULES) will be bit difficult.
> > Please share your opinion on this.
> > 
> > >
> > > And Skia really doesn't have anything like a stable API? :-(
> > >
> > 
> > Yes. In my experience, Skia APIs are changing very rapidly. Not sure of
> the
> > reason though.
> > 
> > > On 23.06.2014 10:44, Ravi Nanjundappa wrote:
> > > > This fixes several build related issues for the skia backend which
> > > > is introduced due to skia source up-gradation.
> > > >
> > > > Signed-off-by: Ravi Nanjundappa <[email protected]>
> > > > ---
> > > >  configure.ac                    |   12 ++++----
> > > >  src/skia/cairo-skia-context.cpp |   61
> > > ++++++++++++++++++++++++++++++---------
> > > >  src/skia/cairo-skia-private.h   |    4 +--
> > > >  src/skia/cairo-skia-surface.cpp |    9 ++----
> > > >  4 files changed, 58 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/configure.ac b/configure.ac index fdcb2dc..04479ff
> > > > 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -249,16 +249,16 @@ CAIRO_ENABLE_SURFACE_BACKEND(skia, Skia,
> > > no, [
> > > >  			      [directory to find compiled skia sources])],
> > > >  	      [skia_DIR="$withval"],
> > > >  	      [skia_DIR="`pwd`/../skia"])
> > > > -  AC_ARG_WITH([skia-bulid],
> > > > -	      [AS_HELP_STRING([--with-skia-build=(Release|Debug)]
> > > > +  AC_ARG_WITH([skia-build-type],
> > > > +	      [AS_HELP_STRING([--with-skia-build-type=(Release|Debug)]
> > > >  			      [build of skia to link with, default is
> > Release])],
> > >
> > > Why is it necessary to rename the configure option to fix the build?
> > >
> > > > -	      [skia_BUILD="$withval"],
> > > > -	      [skia_BUILD="Release"])
> > > > +	      [skia_BUILD_TYPE="$withval"],
> > > > +	      [skia_BUILD_TYPE="Release"])
> > > >    skia_NONPKGCONFIG_CFLAGS="-I$skia_DIR/include/config -
> > > I$skia_DIR/include/core -I$skia_DIR/include/effects"
> > > > -  if test "x$skia_BUILD" = x"Release"; then
> > > > +  if test "x$skia_BUILD_TYPE" = "xRelease"; then
> > > >    	skia_NONPKGCONFIG_CFLAGS="-DSK_RELEASE -
> > > DSK_CAN_USE_FLOAT $skia_NONPKGCONFIG_CFLAGS"
> > > >    fi
> > > > -  skia_NONPKGCONFIG_LIBS="--start-group
> > > $skia_DIR/out/$skia_BUILD/obj.target/gyp/libeffects.a
> > > $skia_DIR/out/$skia_BUILD/obj.target/gyp/libimages.a
> > > $skia_DIR/out/$skia_BUILD/obj.target/gyp/libutils.a
> > > $skia_DIR/out/$skia_BUILD/obj.target/gyp/libopts.a
> > > $skia_DIR/out/$skia_BUILD/obj.target/gyp/libcore.a -end-group"
> > > > +  skia_NONPKGCONFIG_LIBS="-
> > > L$skia_DIR/out/$skia_BUILD_TYPE/lib.target/ -lskia -lstdc++"
> > >
> > > Urgh. The old options scare me, the new ones also do so a bit. But at
> > least
> > > this is better than what we had before.
> > >
> > > Although the new -lstdc++ sounds like a non-portable (gcc specific?)
> > > hack
> > to
> > > work around some deeper issues in how the skia backend is build.
> > > However, I'll just look the other way for this one....
> > 
> > -lstdc++ is required for new  and delete operators used in the skia
> > -lstdc++ source
> > files majorly.
> > 
> > >
> > > >    AC_SUBST(skia_DIR)
> > > >  ])
> > > >
> > > > diff --git a/src/skia/cairo-skia-context.cpp
> > > > b/src/skia/cairo-skia-context.cpp index bbe5507..c2d9fba 100644
> > > > --- a/src/skia/cairo-skia-context.cpp
> > > > +++ b/src/skia/cairo-skia-context.cpp
> > > > @@ -53,6 +53,7 @@
> > > >  #include "cairo-skia-private.h"
> > > >  #include "cairo-surface-backend-private.h"
> > > >
> > > > +#include <SkPaint.h>
> > > >  #include <SkShader.h>
> > > >  #include <SkColorShader.h>
> > > >  #include <SkGradientShader.h>
> > > > @@ -236,14 +237,15 @@ surface_to_sk_bitmap (cairo_surface_t
> > > > *surface, SkBitmap& bitmap)  {
> > > >      cairo_image_surface_t *img = (cairo_image_surface_t *) surface;
> > > >      SkBitmap::Config config;
> > > > +    SkColorType colorType;
> > > >      bool opaque;
> > > >
> > > >      if (unlikely (! format_to_sk_config (img->format, config,
> opaque)))
> > > >  	return false;
> > > >
> > > >      bitmap.reset ();
> > > > -    bitmap.setConfig (config, img->width, img->height, img->stride);
> > > > -    bitmap.setIsOpaque (opaque);
> > > > +    colorType = SkBitmapConfigToColorType(config);
> > > > +    bitmap.setInfo (SkImageInfo::Make(img->width, img->height,
> > > > + colorType, kPremul_SkAlphaType), img->stride);
> > > >      bitmap.setPixels (img->data);
> > >
> > > Opaque becomes an unused variable? Is that really the right thing?
> > > Ok. I looked at the code. Nope, it's not. "opaque" is the only thing
> > > that
> > tells
> > > apart ARGB32 and RGB24, both have config SkBitmap::kARGB_8888_Config.
> > >
> > 
> > I understand your point. I'll try to find a way to differentiate between
> > ARGB32 and RGB24 formats.
> > 
> 
> Modified the source code to consider the "opaque" value using setAlphaType()
> to differentiate between alpha and non-alpha formats.
> 
> > > >      return true;
> > > > @@ -330,9 +332,18 @@ source_to_sk_shader (cairo_skia_context_t *cr,
> > > >  	if (surface->type == CAIRO_SURFACE_TYPE_SKIA) {
> > > >  	    cairo_skia_surface_t *esurf = (cairo_skia_surface_t *)
> > > > surface;
> > > >
> > > > -	    shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> > > > +            if(! _cairo_matrix_is_identity (&pattern->matrix))
> > > > +	    {
> > > > +	        SkMatrix localMatrix =  matrix_inverse_to_sk
> > (pattern->matrix);
> > > > +	        shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> > > > +						   extend_to_sk (pattern-
> > > >extend),
> > > > +						   extend_to_sk (pattern-
> > > >extend),
> > > > +						   &localMatrix);
> > > > +	    } else {
> > > > +	        shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> > > >  						   extend_to_sk (pattern-
> > > >extend),
> > > >  						   extend_to_sk (pattern-
> > > >extend));
> > > > +	    }
> > >
> > > Does this really need the "if"? I would just always use the code with
> > > localMatric.
> > >
> > > (Also: It should be "if (! " instead of "if(! " and the indentation is
> > wrong
> > > (spaces instead of tabs and too many of them)
> > 
> > Thank for pointing it out. I'll correct these and re-submit the patch
> again.
> 
> Here we've followed the same method as used for pattern->type ==
> CAIRO_PATTERN_TYPE_LINEAR (code below this source snippet). So I feel this
> should be fine.
> Please feel free to comment on it. 
> 
> > >
> > > >  	} else {
> > > >  	    SkBitmap bitmap;
> > > >
> > > > @@ -351,9 +362,18 @@ source_to_sk_shader (cairo_skia_context_t *cr,
> > > >  	    if (unlikely (! surface_to_sk_bitmap (surface, bitmap)))
> > > >  		return NULL;
> > > >
> > > > -	    shader = SkShader::CreateBitmapShader (bitmap,
> > > > -						   extend_to_sk (pattern-
> > > >extend),
> > > > -						   extend_to_sk (pattern-
> > > >extend));
> > > > +	    if(! _cairo_matrix_is_identity (&pattern->matrix))
> > > > +	    {
> > > > +                SkMatrix localMatrix = matrix_inverse_to_sk
> > (pattern->matrix);
> > > > +                shader = SkShader::CreateBitmapShader (bitmap,
> > > > +                                                extend_to_sk
> > (pattern->extend),
> > > > +                                                extend_to_sk
> > (pattern->extend),
> > > > +                                                &localMatrix);
> > > > +            } else {
> > > > +                shader = SkShader::CreateBitmapShader (bitmap,
> > > > +	                                        extend_to_sk
> > (pattern->extend),
> > > > +                                                extend_to_sk
> > (pattern->extend));
> > > > +	    }
> > >
> > > Same as above
> > >
> > > >  	}
> > > >      } else if (pattern->type == CAIRO_PATTERN_TYPE_LINEAR
> > > >  	       /* || pattern->type == CAIRO_PATTERN_TYPE_RADIAL */) @@
> > > > -382,8 +402,17 @@ source_to_sk_shader (cairo_skia_context_t *cr,
> > > >  			   SkFloatToScalar (linear->pd1.y));
> > > >  	    points[1].set (SkFloatToScalar (linear->pd2.x),
> > > >  			   SkFloatToScalar (linear->pd2.y));
> > > > -	    shader = SkGradientShader::CreateLinear (points, colors, pos,
> > > gradient->n_stops,
> > > > +
> > > > +	    if(! _cairo_matrix_is_identity (&pattern->matrix))
> > > > +	    {
> > > > +		SkMatrix localMatrix = matrix_inverse_to_sk (pattern-
> > > >matrix);
> > > > +	        shader = SkGradientShader::CreateLinear (points, colors,
> > pos,
> > > gradient->n_stops,
> > > > +						     extend_to_sk (pattern-
> > > >extend),
> > > > +						     0, &localMatrix);
> > > > +	    } else {
> > > > +	        shader = SkGradientShader::CreateLinear (points, colors,
> > > > +pos, gradient->n_stops,
> > > >  						     extend_to_sk (pattern-
> > > >extend));
> > > > +	    }
> > >
> > > same
> > >
> > > >  	} else {
> > > >  	    // XXX todo -- implement real radial shaders in Skia
> > > >  	}
> > > > @@ -394,8 +423,8 @@ source_to_sk_shader (cairo_skia_context_t *cr,
> > > >  	}
> > > >      }
> > > >
> > > > -    if (shader && ! _cairo_matrix_is_identity (&pattern->matrix))
> > > > -	shader->setLocalMatrix (matrix_inverse_to_sk (pattern->matrix));
> > > > +    //if (shader && ! _cairo_matrix_is_identity (&pattern->matrix))
> > > > +	//shader->setLocalMatrix (matrix_inverse_to_sk (pattern->matrix));
> > >
> > >
> > > Why do you comment this out? Just delete it.
> > 
> > Sure. I'll delete these lines and re-submit the patch again.
> 
> Yep. I've taken care of this. 
> 
> > 
> > >
> > > >      return shader;
> > > >  }
> > > > @@ -446,6 +475,7 @@ _cairo_skia_context_set_source (void
> > *abstract_cr,
> > > >  	cr->paint->setColor (color);
> > > >      } else {
> > > >  	SkShader *shader = source_to_sk_shader (cr, source);
> > > > +	SkPaint::FilterLevel fLevel = (SkPaint::FilterLevel)
> > > > +pattern_filter_to_sk (source);
> > >
> > > This casts a boolean to SkPaint::FilterLevel and...
> > >
> > > >  	if (shader == NULL) {
> > > >  	    UNSUPPORTED;
> > > >  	    return CAIRO_STATUS_SUCCESS;
> > > > @@ -454,7 +484,9 @@ _cairo_skia_context_set_source (void
> > *abstract_cr,
> > > >  	cr->paint->setShader (shader);
> > > >  	shader->unref ();
> > > >
> > > > -	cr->paint->setFilterBitmap (pattern_filter_to_sk (source));
> > > > +        cr->paint->setFilterLevel(fLevel ?
> > > > +	(SkPaint::kLow_FilterLevel) :
> > > > +	(SkPaint::kNone_FilterLevel));
> > > >      }
> > >
> > > ...then uses that as a boolean again. Really?
> > >
> > > (Oh and: wrong indentation)
> > >
> > 
> > Sure. I'll recheck these and submit the changes again.
> 
> I've rectified this part of the source to avoid un-necessary conversions.
> Thanks for pointing out. I somehow missed it initially.
> 
> > 
> > > >      /* XXX change notification */
> > > > @@ -496,7 +528,10 @@ _cairo_skia_context_set_source_surface (void
> > > *abstract_cr,
> > > >  	cr->paint->setShader (shader);
> > > >  	shader->unref ();
> > > >
> > > > -	cr->paint->setFilterBitmap (true);
> > > > +	cr->paint->setFilterLevel(true ?
> > > > +	(SkPaint::kLow_FilterLevel) :
> > > > +	(SkPaint::kNone_FilterLevel));
> > > > +
> > >
> > > Why?! Can you fix this up, please? (Oh and of course: wrong
> > > indentation)
> > >
> > 
> > Sure. I'll recheck these and submit the changes again.
> 
> Same as above.
> 
> > 
> > > >  	return CAIRO_STATUS_SUCCESS;
> > > >      }
> > > > @@ -682,7 +717,7 @@ _cairo_skia_context_set_dash (void *abstract_cr,
> > > >  	    intervals[i++] = SkFloatToScalar (dashes[j]);
> > > >      } while (loop--);
> > > >
> > > > -    SkDashPathEffect *dash = new SkDashPathEffect (intervals,
> > > num_dashes, SkFloatToScalar (offset));
> > > > +    SkDashPathEffect *dash = SkDashPathEffect::Create (intervals,
> > > > + num_dashes, SkFloatToScalar (offset));
> > > >
> > > >      cr->paint->setPathEffect (dash);
> > > >      dash->unref ();
> > > > @@ -1264,7 +1299,7 @@ _cairo_skia_context_paint_with_alpha (void
> > > *abstract_cr,
> > > >      if (CAIRO_ALPHA_IS_OPAQUE (alpha))
> > > >  	return _cairo_skia_context_paint (cr);
> > > >
> > > > -    cr->paint->setAlpha(SkScalarRound(255*alpha));
> > > > +    cr->paint->setAlpha(SkScalarRoundToInt(255*alpha));
> > > >      status = _cairo_skia_context_paint (cr);
> > > >      cr->paint->setAlpha(255);
> > > >
> > > > diff --git a/src/skia/cairo-skia-private.h
> > > > b/src/skia/cairo-skia-private.h index de3897b..f538b48 100644
> > > > --- a/src/skia/cairo-skia-private.h
> > > > +++ b/src/skia/cairo-skia-private.h
> > > > @@ -111,11 +111,9 @@ format_to_sk_config (cairo_format_t format,
> > > >      case CAIRO_FORMAT_A8:
> > > >  	config = SkBitmap::kA8_Config;
> > > >  	break;
> > > > -    case CAIRO_FORMAT_A1:
> > > > -	config = SkBitmap::kA1_Config;
> > > > -	break;
> > > >      case CAIRO_FORMAT_RGB30:
> > > >      case CAIRO_FORMAT_INVALID:
> > > > +    case CAIRO_FORMAT_A1:
> > > >      default:
> > > >  	return false;
> > > >      }
> > >
> > > Did skia really drop A1 surfaces?!
> > 
> > Yes. It has dropped A1 surfaces  (as in the skia commit id : commit
> > 11f392ed531f05e8de6b6af6ae607f90aeb080c6)
> > >
> > > > diff --git a/src/skia/cairo-skia-surface.cpp
> > > > b/src/skia/cairo-skia-surface.cpp index bb785e1..682f3db 100644
> > > > --- a/src/skia/cairo-skia-surface.cpp
> > > > +++ b/src/skia/cairo-skia-surface.cpp
> > > > @@ -207,9 +207,6 @@ sk_config_to_pixman_format_code
> > > (SkBitmap::Config
> > > > config,
> > > >
> > > >      case SkBitmap::kA8_Config:
> > > >  	return PIXMAN_a8;
> > > > -
> > > > -    case SkBitmap::kA1_Config:
> > > > -	return PIXMAN_a1;
> > > >      case SkBitmap::kRGB_565_Config:
> > > >  	return PIXMAN_r5g6b5;
> > > >      case SkBitmap::kARGB_4444_Config:
> > > > @@ -217,7 +214,6 @@ sk_config_to_pixman_format_code
> > > (SkBitmap::Config
> > > > config,
> > > >
> > > >      case SkBitmap::kNo_Config:
> > > >      case SkBitmap::kIndex8_Config:
> > > > -    case SkBitmap::kRLE_Index8_Config:
> > > >      case SkBitmap::kConfigCount:
> > > >      default:
> > > >  	ASSERT_NOT_REACHED;
> > > > @@ -236,6 +232,7 @@ _cairo_skia_surface_create_internal
> > > (SkBitmap::Config config,
> > > >      cairo_skia_surface_t *surface;
> > > >      pixman_image_t *pixman_image;
> > > >      pixman_format_code_t pixman_format;
> > > > +    SkColorType colorType;
> > > >
> > > >      surface = (cairo_skia_surface_t *) malloc (sizeof
> > (cairo_skia_surface_t));
> > > >      if (unlikely (surface == NULL)) @@ -256,8 +253,8 @@
> > > > _cairo_skia_surface_create_internal
> > > (SkBitmap::Config config,
> > > >      _cairo_image_surface_init (&surface->image, pixman_image,
> > > > pixman_format);
> > > >
> > > >      surface->bitmap = new SkBitmap;
> > > > -    surface->bitmap->setConfig (config, width, height, surface-
> > > >image.stride);
> > > > -    surface->bitmap->setIsOpaque (opaque);
> > > > +    colorType = SkBitmapConfigToColorType(config);
> > > > +    surface->bitmap->setInfo (SkImageInfo::Make(width, height,
> > > > + colorType, kPremul_SkAlphaType), surface->image.stride);
> > > >      surface->bitmap->setPixels (surface->image.data);
> > > >
> > > >      surface->image.base.is_clear = data == NULL;
> > >
> > > Same as above: What happened to "opaque"? It's now unused and in my
> > > local git tree it is the only difference between RGB24 and ARGB32, so
> > > it seems kind of important to me. And no, AFAIK we cannot assume that
> > > ARGB32 surfaces have "the right" value in that unused byte.
> > >
> > > Note that I have no clue about skia and will not give my Reviewed-by
> > > to
> > any
> > > patches touching it. The above is just what I noticed from a quick look.
> > 
> > Thanks for your comments. I'll try to look at these and re-submit the
> patch
> > again with relevant changes, wherever necessary.
> > I hope that should be fine.
> 
> Yes. Taken care of the usage of "opaque" value to differentiate between
> alpha and non-alpha formats.
> 
> > 
> > >
> > > Cheers,
> > > Uli
> > > --
> > > "Some people are worth melting for." - Olaf
> > > --
> > > cairo mailing list
> > > [email protected]
> > > http://lists.cairographics.org/mailman/listinfo/cairo
> > 
> > Thanks and Best Regards,
> > N Ravi
> > 
> > 
> > --
> > cairo mailing list
> > [email protected]
> > http://lists.cairographics.org/mailman/listinfo/cairo
> 
> Thanks and Best Regards,
> N Ravi

> >From 9abf2d93e54933968e2ab87cbe415a7e61672b39 Mon Sep 17 00:00:00 2001
> From: Ravi Nanjundappa <[email protected]>
> Date: Mon, 30 Jun 2014 17:05:26 +0530
> Subject: [PATCH] skia: update the source to build with the latest skia
> 
> This fixes several build related issues for the skia backend
> which is introduced due to skia source up-gradation.
> 
> Signed-off-by: Ravi Nanjundappa <[email protected]>
> ---
>  configure.ac                    |   12 ++++----
>  src/skia/cairo-skia-context.cpp |   65 +++++++++++++++++++++++++++++----------
>  src/skia/cairo-skia-private.h   |    4 +--
>  src/skia/cairo-skia-surface.cpp |   13 +++-----
>  4 files changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index fdcb2dc..04479ff 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -249,16 +249,16 @@ CAIRO_ENABLE_SURFACE_BACKEND(skia, Skia, no, [
>  			      [directory to find compiled skia sources])],
>  	      [skia_DIR="$withval"],
>  	      [skia_DIR="`pwd`/../skia"])
> -  AC_ARG_WITH([skia-bulid],
> -	      [AS_HELP_STRING([--with-skia-build=(Release|Debug)]
> +  AC_ARG_WITH([skia-build-type],
> +	      [AS_HELP_STRING([--with-skia-build-type=(Release|Debug)]
>  			      [build of skia to link with, default is Release])],
> -	      [skia_BUILD="$withval"],
> -	      [skia_BUILD="Release"])
> +	      [skia_BUILD_TYPE="$withval"],
> +	      [skia_BUILD_TYPE="Release"])
>    skia_NONPKGCONFIG_CFLAGS="-I$skia_DIR/include/config -I$skia_DIR/include/core -I$skia_DIR/include/effects"
> -  if test "x$skia_BUILD" = x"Release"; then
> +  if test "x$skia_BUILD_TYPE" = "xRelease"; then
>    	skia_NONPKGCONFIG_CFLAGS="-DSK_RELEASE -DSK_CAN_USE_FLOAT $skia_NONPKGCONFIG_CFLAGS"
>    fi
> -  skia_NONPKGCONFIG_LIBS="--start-group $skia_DIR/out/$skia_BUILD/obj.target/gyp/libeffects.a $skia_DIR/out/$skia_BUILD/obj.target/gyp/libimages.a $skia_DIR/out/$skia_BUILD/obj.target/gyp/libutils.a $skia_DIR/out/$skia_BUILD/obj.target/gyp/libopts.a $skia_DIR/out/$skia_BUILD/obj.target/gyp/libcore.a -end-group"
> +  skia_NONPKGCONFIG_LIBS="-L$skia_DIR/out/$skia_BUILD_TYPE/lib.target/ -lskia -lstdc++"
>    AC_SUBST(skia_DIR)
>  ])
>  
> diff --git a/src/skia/cairo-skia-context.cpp b/src/skia/cairo-skia-context.cpp
> index bbe5507..9ffb8f6 100644
> --- a/src/skia/cairo-skia-context.cpp
> +++ b/src/skia/cairo-skia-context.cpp
> @@ -53,6 +53,7 @@
>  #include "cairo-skia-private.h"
>  #include "cairo-surface-backend-private.h"
>  
> +#include <SkPaint.h>
>  #include <SkShader.h>
>  #include <SkColorShader.h>
>  #include <SkGradientShader.h>
> @@ -236,16 +237,19 @@ surface_to_sk_bitmap (cairo_surface_t *surface, SkBitmap& bitmap)
>  {
>      cairo_image_surface_t *img = (cairo_image_surface_t *) surface;
>      SkBitmap::Config config;
> +    SkColorType colorType;
>      bool opaque;
>  
>      if (unlikely (! format_to_sk_config (img->format, config, opaque)))
>  	return false;
>  
>      bitmap.reset ();
> -    bitmap.setConfig (config, img->width, img->height, img->stride);
> -    bitmap.setIsOpaque (opaque);
> +    bitmap.setAlphaType (opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType);
> +    colorType = SkBitmapConfigToColorType(config);
> +    bitmap.setInfo (SkImageInfo::Make(img->width, img->height, colorType, kPremul_SkAlphaType), img->stride);
>      bitmap.setPixels (img->data);
>  
> +
>      return true;
>  }
>  
> @@ -330,9 +334,18 @@ source_to_sk_shader (cairo_skia_context_t *cr,
>  	if (surface->type == CAIRO_SURFACE_TYPE_SKIA) {
>  	    cairo_skia_surface_t *esurf = (cairo_skia_surface_t *) surface;
>  
> -	    shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> -						   extend_to_sk (pattern->extend),
> -						   extend_to_sk (pattern->extend));
> +		if (! _cairo_matrix_is_identity (&pattern->matrix))
> +		{
> +			SkMatrix localMatrix =  matrix_inverse_to_sk (pattern->matrix);
> +			shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> +					extend_to_sk (pattern->extend),
> +					extend_to_sk (pattern->extend),
> +					&localMatrix);
> +		} else {
> +			shader = SkShader::CreateBitmapShader (*esurf->bitmap,
> +					extend_to_sk (pattern->extend),
> +					extend_to_sk (pattern->extend));
> +		}
>  	} else {
>  	    SkBitmap bitmap;
>  
> @@ -351,9 +364,18 @@ source_to_sk_shader (cairo_skia_context_t *cr,
>  	    if (unlikely (! surface_to_sk_bitmap (surface, bitmap)))
>  		return NULL;
>  
> -	    shader = SkShader::CreateBitmapShader (bitmap,
> -						   extend_to_sk (pattern->extend),
> -						   extend_to_sk (pattern->extend));
> +		if (! _cairo_matrix_is_identity (&pattern->matrix))
> +		{
> +			SkMatrix localMatrix = matrix_inverse_to_sk (pattern->matrix);
> +			shader = SkShader::CreateBitmapShader (bitmap,
> +					extend_to_sk (pattern->extend),
> +					extend_to_sk (pattern->extend),
> +					&localMatrix);
> +		} else {
> +			shader = SkShader::CreateBitmapShader (bitmap,
> +			extend_to_sk (pattern->extend),
> +			extend_to_sk (pattern->extend));
> +		}
>  	}
>      } else if (pattern->type == CAIRO_PATTERN_TYPE_LINEAR
>  	       /* || pattern->type == CAIRO_PATTERN_TYPE_RADIAL */)
> @@ -382,8 +404,17 @@ source_to_sk_shader (cairo_skia_context_t *cr,
>  			   SkFloatToScalar (linear->pd1.y));
>  	    points[1].set (SkFloatToScalar (linear->pd2.x),
>  			   SkFloatToScalar (linear->pd2.y));
> -	    shader = SkGradientShader::CreateLinear (points, colors, pos, gradient->n_stops,
> -						     extend_to_sk (pattern->extend));
> +
> +	    if(! _cairo_matrix_is_identity (&pattern->matrix))
> +	    {
> +		SkMatrix localMatrix = matrix_inverse_to_sk (pattern->matrix);
> +	        shader = SkGradientShader::CreateLinear (points, colors, pos, gradient->n_stops,
> +						     extend_to_sk (pattern->extend),
> +						     0, &localMatrix);
> +	    } else {
> +	        shader = SkGradientShader::CreateLinear (points, colors, pos, gradient->n_stops,
> +							extend_to_sk (pattern->extend));
> +	    }
>  	} else {
>  	    // XXX todo -- implement real radial shaders in Skia
>  	}
> @@ -394,9 +425,6 @@ source_to_sk_shader (cairo_skia_context_t *cr,
>  	}
>      }
>  
> -    if (shader && ! _cairo_matrix_is_identity (&pattern->matrix))
> -	shader->setLocalMatrix (matrix_inverse_to_sk (pattern->matrix));
> -
>      return shader;
>  }
>  
> @@ -446,6 +474,7 @@ _cairo_skia_context_set_source (void *abstract_cr,
>  	cr->paint->setColor (color);
>      } else {
>  	SkShader *shader = source_to_sk_shader (cr, source);
> +	bool fLevel = pattern_filter_to_sk (source);
>  	if (shader == NULL) {
>  	    UNSUPPORTED;
>  	    return CAIRO_STATUS_SUCCESS;
> @@ -454,7 +483,8 @@ _cairo_skia_context_set_source (void *abstract_cr,
>  	cr->paint->setShader (shader);
>  	shader->unref ();
>  
> -	cr->paint->setFilterBitmap (pattern_filter_to_sk (source));
> +	cr->paint->setFilterLevel (fLevel ?
> +				(SkPaint::kLow_FilterLevel) : (SkPaint::kNone_FilterLevel));
>      }
>  
>      /* XXX change notification */
> @@ -496,7 +526,8 @@ _cairo_skia_context_set_source_surface (void *abstract_cr,
>  	cr->paint->setShader (shader);
>  	shader->unref ();
>  
> -	cr->paint->setFilterBitmap (true);
> +	cr->paint->setFilterLevel (true ?
> +		(SkPaint::kLow_FilterLevel) : (SkPaint::kNone_FilterLevel));
>  
>  	return CAIRO_STATUS_SUCCESS;
>      }
> @@ -682,7 +713,7 @@ _cairo_skia_context_set_dash (void *abstract_cr,
>  	    intervals[i++] = SkFloatToScalar (dashes[j]);
>      } while (loop--);
>  
> -    SkDashPathEffect *dash = new SkDashPathEffect (intervals, num_dashes, SkFloatToScalar (offset));
> +    SkDashPathEffect *dash = SkDashPathEffect::Create (intervals, num_dashes, SkFloatToScalar (offset));
>  
>      cr->paint->setPathEffect (dash);
>      dash->unref ();
> @@ -1264,7 +1295,7 @@ _cairo_skia_context_paint_with_alpha (void *abstract_cr,
>      if (CAIRO_ALPHA_IS_OPAQUE (alpha))
>  	return _cairo_skia_context_paint (cr);
>  
> -    cr->paint->setAlpha(SkScalarRound(255*alpha));
> +    cr->paint->setAlpha(SkScalarRoundToInt(255*alpha));
>      status = _cairo_skia_context_paint (cr);
>      cr->paint->setAlpha(255);
>  
> diff --git a/src/skia/cairo-skia-private.h b/src/skia/cairo-skia-private.h
> index de3897b..f538b48 100644
> --- a/src/skia/cairo-skia-private.h
> +++ b/src/skia/cairo-skia-private.h
> @@ -111,11 +111,9 @@ format_to_sk_config (cairo_format_t format,
>      case CAIRO_FORMAT_A8:
>  	config = SkBitmap::kA8_Config;
>  	break;
> -    case CAIRO_FORMAT_A1:
> -	config = SkBitmap::kA1_Config;
> -	break;
>      case CAIRO_FORMAT_RGB30:
>      case CAIRO_FORMAT_INVALID:
> +    case CAIRO_FORMAT_A1:
>      default:
>  	return false;
>      }
> diff --git a/src/skia/cairo-skia-surface.cpp b/src/skia/cairo-skia-surface.cpp
> index bb785e1..9b16bd2 100644
> --- a/src/skia/cairo-skia-surface.cpp
> +++ b/src/skia/cairo-skia-surface.cpp
> @@ -64,7 +64,7 @@ _cairo_skia_surface_create_similar (void *asurface,
>  
>      if (content == surface->image.base.content)
>      {
> -	config = surface->bitmap->getConfig ();
> +	config = surface->bitmap->config ();
>  	opaque = surface->bitmap->isOpaque ();
>      }
>      else if (! format_to_sk_config (_cairo_format_from_content (content),
> @@ -207,9 +207,6 @@ sk_config_to_pixman_format_code (SkBitmap::Config config,
>  
>      case SkBitmap::kA8_Config:
>  	return PIXMAN_a8;
> -
> -    case SkBitmap::kA1_Config:
> -	return PIXMAN_a1;
>      case SkBitmap::kRGB_565_Config:
>  	return PIXMAN_r5g6b5;
>      case SkBitmap::kARGB_4444_Config:
> @@ -217,8 +214,6 @@ sk_config_to_pixman_format_code (SkBitmap::Config config,
>  
>      case SkBitmap::kNo_Config:
>      case SkBitmap::kIndex8_Config:
> -    case SkBitmap::kRLE_Index8_Config:
> -    case SkBitmap::kConfigCount:
>      default:
>  	ASSERT_NOT_REACHED;
>  	return (pixman_format_code_t) -1;
> @@ -236,6 +231,7 @@ _cairo_skia_surface_create_internal (SkBitmap::Config config,
>      cairo_skia_surface_t *surface;
>      pixman_image_t *pixman_image;
>      pixman_format_code_t pixman_format;
> +    SkColorType colorType;
>  
>      surface = (cairo_skia_surface_t *) malloc (sizeof (cairo_skia_surface_t));
>      if (unlikely (surface == NULL))
> @@ -256,8 +252,9 @@ _cairo_skia_surface_create_internal (SkBitmap::Config config,
>      _cairo_image_surface_init (&surface->image, pixman_image, pixman_format);
>  
>      surface->bitmap = new SkBitmap;
> -    surface->bitmap->setConfig (config, width, height, surface->image.stride);
> -    surface->bitmap->setIsOpaque (opaque);
> +    colorType = SkBitmapConfigToColorType(config);
> +    surface->bitmap->setAlphaType (opaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType);
> +    surface->bitmap->setInfo (SkImageInfo::Make(width, height, colorType, kPremul_SkAlphaType), surface->image.stride);
>      surface->bitmap->setPixels (surface->image.data);
>  
>      surface->image.base.is_clear = data == NULL;
> -- 
> 1.7.9.5
> 

> -- 
> cairo mailing list
> [email protected]
> http://lists.cairographics.org/mailman/listinfo/cairo
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo