Last Comment Bug 740815 - [Azure] Add support for component alpha
: [Azure] Add support for component alpha
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Bas Schouten (:bas.schouten)
:
:
Mentors:
Depends on: 743148 743393 743590
Blocks: 715768
  Show dependency treegraph
 
Reported: 2012-03-30 08:18 PDT by Bas Schouten (:bas.schouten)
Modified: 2012-05-20 13:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add DrawTargetDual for drawing to two drawtargets (20.73 KB, patch)
2012-03-30 08:20 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 2: Generalize filltextures (7.99 KB, patch)
2012-03-30 08:27 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 3: Use component alpha rendering when drawing content with azure (7.14 KB, patch)
2012-03-30 08:28 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2012-03-30 08:18:36 PDT
We need to support component-alpha thebeslayers in Azure.
Comment 1 Bas Schouten (:bas.schouten) 2012-03-30 08:20:48 PDT
Created attachment 610887 [details] [diff] [review]
Part 1: Add DrawTargetDual for drawing to two drawtargets
Comment 2 Bas Schouten (:bas.schouten) 2012-03-30 08:27:58 PDT
Created attachment 610889 [details] [diff] [review]
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) 2012-03-30 08:28:46 PDT
Created attachment 610890 [details] [diff] [review]
Part 3: Use component alpha rendering when drawing content with azure
Comment 4 Jeff Muizelaar [:jrmuizel] 2012-04-02 12:27:54 PDT
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] 2012-04-02 12:32:08 PDT
Comment on attachment 610889 [details] [diff] [review]
Part 2: Generalize filltextures

This is useful non-azure as well correct?
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-04-02 14:31:37 PDT
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] 2012-04-02 14:48:41 PDT
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) 2012-04-03 12:37:54 PDT
(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) 2012-04-03 12:40:19 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.