1054838 - Convert gfxPattern to Moz2D

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

gfxPattern uses cairo patterns internally, but we only ever use them with Moz2D.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Remove dead code

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Remove callers of DrawTarget::Mask

This was dead code, just not all that obvious.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Remove DrawTarget::Mask

I'm not sure if we actually want to do this or not. We no longer have any callers in mozilla-central, but it might be worth keeping the functionality in Moz2D.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3 (was part 4): Convert gfxPattern to Moz2D

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8474359 [details] [diff] [review]
Part 3: Remove DrawTarget::Mask

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

I feel we shouldn't do this, this functionality could very well be useful in the future.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8474360 [details] [diff] [review]
Part 3 (was part 4): Convert gfxPattern to Moz2D

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

Awesome work!

::: gfx/thebes/gfxPattern.cpp
@@ +64,5 @@
>  void
>  gfxPattern::AddColorStop(gfxFloat offset, const gfxRGBA& c)
>  {
> +  if (mGfxPattern->GetType() != PatternType::LINEAR_GRADIENT &&
> +      mGfxPattern->GetType() != PatternType::RADIAL_GRADIENT) {

nit: little warning here maybe?

@@ +72,5 @@
> +  mStops = nullptr;
> +  gfxRGBA color = c;
> +  if (gfxPlatform::GetCMSMode() == eCMSMode_All) {
> +    qcms_transform *transform = gfxPlatform::GetCMSRGBTransform();
> +    if (transform)

nit { }

@@ +125,5 @@
>  Pattern*
>  gfxPattern::GetPattern(DrawTarget *aTarget, Matrix *aPatternTransform)
>  {
> +  if (!mStops &&
> +      !mStopsList.IsEmpty()) {

nit: MOZ_ASSERT(mGfxPattern->GetType() etc.

@@ +190,5 @@
>  
>  void
>  gfxPattern::SetFilter(GraphicsFilter filter)
>  {
> +  if (mGfxPattern->GetType() != PatternType::SURFACE) {

nit: Might want to add a little warning here to remind someone they're doing something silly.

::: gfx/thebes/gfxPattern.h
@@ +123,5 @@
>  
>      mozilla::RefPtr<mozilla::gfx::SourceSurface> mSourceSurface;
>      mozilla::gfx::Matrix mTransform;
>      mozilla::RefPtr<mozilla::gfx::GradientStops> mStops;
> +    nsTArray<mozilla::gfx::GradientStop> mStopsList;

I prefer std::vector, but it seems we can never figure out which one we prefer so I'll leave it up to you :).

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> Comment on attachment 8474359 [details] [diff] [review]
> Part 3: Remove DrawTarget::Mask
> 
> Review of attachment 8474359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel we shouldn't do this, this functionality could very well be useful in
> the future.

I figured you might say that :) No real harm in keeping except that it's probably pretty buggy (in some backends).

Comment 8

[Jonathan Watt [:jwatt]]

Happy days. :)

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4 (was part 5): Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing

Missed this ctor.

I also had to fuzz some tests to get it all to pass. Going through a cairo_pattern_t reduced all values to cairo_fixed_t precision which hid some bugs where the tests weren't actually the same.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8482462 [details] [diff] [review]
Part 4 (was part 5): Stop using the gfxASurface ctor for gfxPattern in gfxWindowsNativeDrawing

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

::: gfx/thebes/gfxWindowsNativeDrawing.cpp
@@ +268,5 @@
>          if (!gfxAlphaRecovery::RecoverAlpha(black, white)) {
>              NS_ERROR("Alpha recovery failure");
>              return;
>          }
> +        RefPtr<SourceSurface> source =

Any chance you could just convert this function to Moz2d?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Not easily! That code relies heavily on gfxWindowsSurface to create DIBs and get the DC out. We don't have equivalent code for Moz2D yet. I'd rather not block this bug on doing that.

Comment 12

[Jonathan Watt [:jwatt]]

I pushed Matt's patches (they required a fair bit of unbitrotting as well as changes to includes to get unified builds working):

https://hg.mozilla.org/integration/mozilla-inbound/rev/55948a19e455
https://hg.mozilla.org/integration/mozilla-inbound/rev/771d12cf1f21
https://hg.mozilla.org/integration/mozilla-inbound/rev/de43b84e2afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/936fe5626189

I also had to mark four reftests as passing now and change the fuzz for others:

https://hg.mozilla.org/integration/mozilla-inbound/rev/826f233564b4

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/55948a19e455
https://hg.mozilla.org/mozilla-central/rev/771d12cf1f21
https://hg.mozilla.org/mozilla-central/rev/de43b84e2afc
https://hg.mozilla.org/mozilla-central/rev/936fe5626189
https://hg.mozilla.org/mozilla-central/rev/826f233564b4
https://hg.mozilla.org/mozilla-central/rev/38b1ff4746d3