Re: [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 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.

> >      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.
> 
> >  	} 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.

> 
> >      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.

> >      /* 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.

> >  	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.

> 
> 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