[cairo] [PATCH] skia: update the source to build with the latest skia
Uli Schlachter
psychon at znc.in
Wed Jun 25 00:59:03 PDT 2014
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?
And Skia really doesn't have anything like a stable API? :-(
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....
> 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.
> 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)
> } 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.
> 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)
> /* 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)
> 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?!
> 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.
Cheers,
Uli
--
“Some people are worth melting for.” - Olaf
More information about the cairo
mailing list