[cairo] [PATCH] skia: update the source to build with the latest skia
RAVI NANJUNDAPPA
nravi.n at samsung.com
Mon Jun 30 04:38:00 PDT 2014
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:cairo-bounces at cairographics.org] On Behalf Of RAVI
> NANJUNDAPPA
> Sent: Wednesday, June 25, 2014 5:01 PM
> To: 'Uli Schlachter'; cairo at cairographics.org
> 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:cairo-bounces at cairographics.org] On Behalf Of Uli
> > Schlachter
> > Sent: Wednesday, June 25, 2014 1:29 PM
> > To: cairo at cairographics.org
> > 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 <nravi.n at samsung.com>
> > > ---
> > > 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
> > cairo at cairographics.org
> > http://lists.cairographics.org/mailman/listinfo/cairo
>
> Thanks and Best Regards,
> N Ravi
>
>
> --
> cairo mailing list
> cairo at cairographics.org
> http://lists.cairographics.org/mailman/listinfo/cairo
Thanks and Best Regards,
N Ravi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-skia-update-the-source-to-build-with-the-latest-skia.patch
Type: application/octet-stream
Size: 10340 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20140630/0261d942/attachment-0001.obj>
More information about the cairo
mailing list