740815 - [Azure] Add support for component alpha

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

We need to support component-alpha thebeslayers in Azure.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add DrawTargetDual for drawing to two drawtargets

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Generalize filltextures

This creates an efficient method of filling the child textures. It's independent of the drawing method used and does both surfaces from a single drawing call, most likely improving performance a little in the progress.

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Use component alpha rendering when drawing content with azure

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610887 [details] [diff] [review]
Part 1: Add DrawTargetDual for drawing to two drawtargets

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

::: gfx/2d/DrawTargetDual.cpp
@@ +61,5 @@
> +  SourceSurface *mB;
> +};
> +
> +
> +class DualPattern

Add a comment someplace about why we only need to have separate patterns for Surface patterns

::: gfx/2d/DrawTargetDual.h
@@ +51,5 @@
> +#define FORWARD_FUNCTION(funcName) \
> +  virtual void funcName() { mA->funcName(); mB->funcName(); }
> +#define FORWARD_FUNCTION1(funcName, var1Type, var1Name) \
> +  virtual void funcName(var1Type var1Name) { mA->funcName(var1Name); mB->funcName(var1Name); }
> +

It's probably worth adding a description of what the semantics of this draw target are, especially any tricky cases.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610889 [details] [diff] [review]
Part 2: Generalize filltextures

This is useful non-azure as well correct?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610889 [details] [diff] [review]
Part 2: Generalize filltextures

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

Nothing objectionable jumps out at me.

::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
@@ +394,5 @@
> +    projection._22 = -2.0f / desc.Height;
> +    projection._33 = 0.0f;
> +    projection._41 = -1.0f;
> +    projection._42 = 1.0f;
> +    projection._44 = 1.0f;

It would be nice if this setup code could be made a bit briefer. I don't know if there's anything we can share with code elsewhere. Alternatively, if it's not too much pain you could break it out into a subfunction.

::: gfx/layers/d3d10/ThebesLayerD3D10.h
@@ +97,5 @@
>    /* Create a new texture */
>    void CreateNewTextures(const gfxIntSize &aSize, SurfaceMode aMode);
>  
> +  // Fill textures with opaque black and white in the specified region.
> +  void FillTextures(const nsIntRegion& aRegion, const nsIntPoint& aOffset);

I think this needs a better name than FillTextures that makes it more obvious that it's doing black and white.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610890 [details] [diff] [review]
Part 3: Use component alpha rendering when drawing content with azure

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

It feels a bit like these changes are pretty intertwined and not that well separated out. I don't know if there's much we can do to fix that for now.

::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
@@ +482,5 @@
>    context->NewPath();
>    const nsIntRect *iterRect;
>    while ((iterRect = iter.Next())) {
>      context->Rectangle(gfxRect(iterRect->x, iterRect->y, iterRect->width, iterRect->height));      
> +    if (mDrawTarget && aMode == SURFACE_SINGLE_CHANNEL_ALPHA) {

I don't understand why this change is here.

@@ +570,5 @@
> +      mDrawTarget = nsnull;
> +    }
> +  }
> +
> +  if (gfxPlatform::UseAzureContentDrawing() && !mDrawTarget) {

Shouldn't this be an else if on the condition above?

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 610890 [details] [diff] [review]
> Part 3: Use component alpha rendering when drawing content with azure
> 
> Review of attachment 610890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It feels a bit like these changes are pretty intertwined and not that well
> separated out. I don't know if there's much we can do to fix that for now.
> 
> ::: gfx/layers/d3d10/ThebesLayerD3D10.cpp
> @@ +482,5 @@
> >    context->NewPath();
> >    const nsIntRect *iterRect;
> >    while ((iterRect = iter.Next())) {
> >      context->Rectangle(gfxRect(iterRect->x, iterRect->y, iterRect->width, iterRect->height));      
> > +    if (mDrawTarget && aMode == SURFACE_SINGLE_CHANNEL_ALPHA) {
> 
> I don't understand why this change is here.

This prevents needless clearing and prevents us clearing out the white we just used to fill the DrawTarget. It's an improvement and it creates consistency with non-Azure behavior lower down.

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> Comment on attachment 610890 [details] [diff] [review]
> Part 3: Use component alpha rendering when drawing content with azure
> 
> Review of attachment 610890 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +570,5 @@
> > +      mDrawTarget = nsnull;
> > +    }
> > +  }
> > +
> > +  if (gfxPlatform::UseAzureContentDrawing() && !mDrawTarget) {
> 
> Shouldn't this be an else if on the condition above?

No, it shouldn't, the idea here is in the Azure world we don't keep track of our DrawTargets separately so it always creates its drawtarget at the end of the function when both textures are prepared and ready. This is, sadly, different from the cairo situation.

Comment 10

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd886d95c55
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36fabc31211
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fa58c6060c5

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/3dd886d95c55
https://hg.mozilla.org/mozilla-central/rev/e36fabc31211
https://hg.mozilla.org/mozilla-central/rev/9fa58c6060c5