1043745 - [Skia] Stop using SkBitmap::Config

Status: NEW

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

Upstream removed the notion of SkBitmap 'Configs' (roughly equivalent to Moz2D's 'Format').

Comment 1

[George Wright (:gw280) (needinfo me!)]

It seems the new descriptor is called SkColorType

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1043745-Use-SkColorType-and-SkImageInfo-instead-.patch

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8469016 [details] [diff] [review]
0001-Bug-1043745-Use-SkColorType-and-SkImageInfo-instead-.patch

Review of attachment 8469016 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetSkia.cpp
@@ +114,5 @@
> +  SkImageInfo info = SkImageInfo::Make(surf->GetSize().width,
> +                                       surf->GetSize().height,
> +                                       GfxFormatToSkiaColorType(surf->GetFormat()),
> +                                       alphaType);
> +  result.mBitmap.setInfo(info);

You need to pass surf->Stride() for the second (optional, but not for us) parameter.

@@ +584,5 @@
>  
>      SkMatrix transform = maskPaint.getShader()->getLocalMatrix();
>      transform.postTranslate(SkFloatToScalar(aOffset.x), SkFloatToScalar(aOffset.y));
> +    SkShader* matrixShader = SkShader::CreateLocalMatrixShader(maskPaint.getShader(), transform);
> +    maskPaint.setShader(matrixShader);

Don't we need to unref the return value here too?

@@ +823,5 @@
> +  SkImageInfo info = SkImageInfo::Make(aSize.width,
> +                                       aSize.height,
> +                                       GfxFormatToSkiaColorType(aFormat),
> +                                       alphaType);
> +  bitmap.setInfo(info);

Stride!

::: gfx/2d/HelpersSkia.h
@@ +38,5 @@
>    }
>  }
>  
> +static inline SurfaceFormat
> +SkiaConfigToGfxFormat(SkBitmap::Config config)

Are either of these actually used? I don't see it.

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +71,5 @@
> +  SkImageInfo info = SkImageInfo::Make(aSize.width,
> +                                       aSize.height,
> +                                       GfxFormatToSkiaColorType(aFormat),
> +                                       alphaType);
> +  temp.setInfo(info);

oh, the poor strides

::: other-licenses/skia-npapi/SkANP.cpp
@@ +69,5 @@
>              break;
>      }
>      
> +    SkImageInfo info = SkImageInfo::Make(src.width, src.height, colorType, kPremul_SkAlphaType);
> +    dst->setInfo(info, src.rowBytes);

stride success!

Comment 4

[George Wright (:gw280) (needinfo me!)]

Good to see you're taking my errors in stride.

Comment 5

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6e5610d6a8

Comment 6

[Mike Hommey [:glandium]]

... this was landed before the skia update, which means it broke the build. That didn't happen on tbpl because it was landed in the same push, but it brakes e.g. bisection.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Oops. That was unfortunate. Sorry.