Closed Bug 740815 Opened 8 years ago Closed 8 years ago

[Azure] Add support for component alpha

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(3 files)

We need to support component-alpha thebeslayers in Azure.
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.
Attachment #610889 - Flags: review?(jmuizelaar)
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.
Attachment #610887 - Flags: review?(jmuizelaar) → review+
Comment on attachment 610889 [details] [diff] [review]
Part 2: Generalize filltextures

This is useful non-azure as well correct?
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.
Attachment #610889 - Flags: review?(jmuizelaar) → review+
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?
Attachment #610890 - Flags: review?(jmuizelaar) → review+
(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.
(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.
Target Milestone: --- → mozilla14
Depends on: 743148
Depends on: 743393
Depends on: 743590
No longer blocks: 715768
Blocks: 715768
You need to log in before you can comment on or make changes to this bug.