741682 - Support component alpha with BasicLayers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: WIP v1

As part of bug 539356 (display list invalidation) we need to support retained component alpha for BasicLayers instead of always redrawing like we currently do.

Attached is an initial prototype that implements this. This seems to work correctly with the limited testing I've done so far.

This really needs work to make it a lot faster. In particular an SSE2 fast path would really help, I'm not sure where to start with that though.

The current BlendPixel function is more or less a direct conversion from the OpenGL shaders / alpha blending docs. We can probably do better here too.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 611711 [details] [diff] [review]
WIP v1

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

Generally looks reasonable.

Split the patch into the alpha-blending functionality and the rest.

It might make sense to just put the new component-alpha blending code into gfxAlphaRecovery.

ThebesLayerBuffer needs a more direct way to tell if it's in component-alpha mode, say a helper method IsUsingComponentAlpha.

Performance on X may be bad. We should definitely create image surfaces, not offscreen surfaces, so that we get a gfxImageSurface instead of some Xlib thing which is horrible to read back from.

We should try to make sure that the destination surface for ThebesLayerBuffer drawing always supports GetAsImageSurface so no copy is required. I think this means using my putative CreateInMemorySurface to create temporary surfaces for container layers in some cases.

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +93,5 @@
> +    nsIntRect destRect(destRectGfx.x, destRectGfx.y, destRectGfx.width, destRectGfx.height);
> +
> +    //Create a temporary surface the size of our dest and copy the background up into it. PushSurfaceAndCopyBackground?
> +    nsRefPtr<gfxASurface> dest = gfxPlatform::GetPlatform()->CreateOffscreenSurface(destRect.Size(), gfxASurface::CONTENT_COLOR);
> +    nsRefPtr<gfxContext> ctx = new gfxContext(dest);

We should be able to do this without a temporary surface, if the destination supports GetAsImageSurface.

::: gfx/layers/ThebesLayerBuffer.h
@@ +147,5 @@
>                          PRUint32 aFlags);
>  
>    enum {
> +    ALLOW_REPEAT = 0x01,
> +    REQUIRE_PIXELS = 0x02

You'd better document this.

::: gfx/layers/basic/BasicLayers.cpp
@@ +862,5 @@
>                                       const nsIntSize& aSize, PRUint32 aFlags)
>  {
> +  if (aFlags & REQUIRE_PIXELS) {
> +    nsRefPtr<gfxASurface> surf = gfxPlatform::GetPlatform()->CreateOffscreenSurface(aSize, aType);
> +    return surf.forget();

I think we need a new method on gfxPlatform, say CreateInMemorySurface() that creates an offscreen surface that is fast for native usage (e.g. an DIB surface for Win32) but will return something from GetAsImageSurface without copying.

::: gfx/thebes/gfxComponentAlpha.cpp
@@ +66,5 @@
> +        aBlackSurface->GetDeviceOffset() != aWhiteSurface->GetDeviceOffset() ||
> +        (aBlackSurface->Format() != gfxASurface::ImageFormatARGB32 &&
> +         aBlackSurface->Format() != gfxASurface::ImageFormatRGB24) ||
> +        (aWhiteSurface->Format() != gfxASurface::ImageFormatARGB32 &&
> +         aWhiteSurface->Format() != gfxASurface::ImageFormatRGB24))

Can't we require RGB24 here?

@@ +132,5 @@
> +        }
> +        blackData += aBlackSurface->Stride();
> +        whiteData += aWhiteSurface->Stride();
> +        destData += aDestSurf->Stride();
> +    }

Probably worth having separate paths for aOpacity == 1.0 and aOpacity < 1.0.

At least for the 1.0 case, we should be able to do BlendPixel entirely in integer math and without an integer division. You may want to use the FAST_DIVIDE_BY_255 macro.

::: gfx/thebes/gfxComponentAlpha.h
@@ +48,5 @@
> +                             const nsIntRect& aDestRect,
> +                             const gfxImageSurface *aBlackSurface,
> +                             const gfxImageSurface *aWhiteSurface,
> +                             const nsIntPoint& aSrcPoint,
> +                             gfxFloat aOpacity);

You'll want to document this.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't put fixed position items into their own layers

Alternative approach that stops content alpha content from ending up in it's own layer.

I can stop us from computing the opaque region / component alpha state twice for the chosen ThebesLayerData if we decide to go with this approach.

Looks like there's still a few bugs here too, it works well with https://bug582223.bugzilla.mozilla.org/attachment.cgi?id=460558, but not with about:home (pre bug 740235)

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 612036 [details] [diff] [review]
Don't put fixed position items into their own layers

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

This is getting hard to follow.

::: layout/base/FrameLayerBuilder.cpp
@@ +318,5 @@
> +                                                   nsIFrame* aActiveScrolledRoot,
> +                                                   bool aRefuseComponentAlpha);
> +
> +  nsIntRegion ComputeOpaqueRegion(nsDisplayItem *aItem, 
> +                                  const nsIntRegion& aRegion, 

Need to document that aRegion is unioned into the result.

@@ +1398,5 @@
>                                     const nsIntRect& aVisibleRect,
>                                     const nsIntRect& aDrawRect,
>                                     const FrameLayerBuilder::Clip& aClip,
> +                                   nsIFrame* aActiveScrolledRoot,
> +                                   bool aRefuseComponentAlpha)

Use a flags word. Beats true/false parameters.

@@ +1696,5 @@
> +        activeScrolledRoot = 
> +          nsLayoutUtils::GetActiveScrolledRootFor(viewportFrame,
> +                                                  mBuilder->ReferenceFrame());
> +
> +        // Attempt to find a thebes layer again, this will always find something

Are you sure?

What happens if we have a fixed background, then we create a new ThebesLayer on top of it containing a bunch of stuff that doesn't need component alpha (e.g., images), then we encounter a text display item that needs component alpha, and partially overlaps the images? Aren't we pretty much stuck at that point?

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> 
> Are you sure?
> 
> What happens if we have a fixed background, then we create a new ThebesLayer
> on top of it containing a bunch of stuff that doesn't need component alpha
> (e.g., images), then we encounter a text display item that needs component
> alpha, and partially overlaps the images? Aren't we pretty much stuck at
> that point?

We're not refusing component alpha layers on the second pass, so it'll put the text into the same layer as the images and just won't have subpixel AA when drawn.

It would be nice to flatten the two layers together to prevent this, but I'm not sure how to do that.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe this is a little crazy, but how about: at the end of FrameLayerBuilder::BuildContainerLayerFor, if we don't want to support component alpha (retained BasicLayerManager), but we have created a component alpha layer, rerun the body of BuildContainerLayerFor in a mode where ContainerState::ProcessDisplayItem makes LAYER_ACTIVE items inactive and assumes the active scrolled root of every frame we encounter is the container frame's active scrolled root. Probably, set a flag on the container frame so that this effect is permanent and subsequent layer tree rebuilds only take one pass through BuildContainerLayerFor.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Approach 3: Repeat BuildContainerLayerFor

This is a lot more complicated than I'd like (read: ugly).

We can't tag items added to mNewDisplayItems with the current container, since ProcessDisplayItems could recurse into another BuildContainerLayerFor call (with the same FrameLayerBuilder), and items created by that call would need to be tagged with both. I guess we could keep a list of containers that are in the callstack, but I don't think thats much of an improvement.

The code just makes a backup copy of all relevant FrameLayerBuilder state before building layers, and then restores it if we want to repeat the process. Plus a fair bit of fiddling to handle objects that really don't want to be copied.

I'll make this backup process only happen with BasicLayers too, no point doing it for other layer managers.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Approach 3: Repeat BuildContainerLayerFor v2

Much cleaner, using a generation counter to detect changed items.

This still doesn't mark the container frame with a flag so that we can avoid this for subsequent paints. Are there any conditions where we would want to remove this flag? Or can it just remain for the lifetime of the frame.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 612449 [details] [diff] [review]
Approach 3: Repeat BuildContainerLayerFor v2

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1555,5 @@
>      LayerState layerState = item->GetLayerState(mBuilder, mManager);
>  
> +    nsIFrame* activeScrolledRoot;
> +    if (aFlags & NO_COMPONENT_ALPHA) {
> +      activeScrolledRoot = nsLayoutUtils::GetActiveScrolledRootFor(item->GetUnderlyingFrame(), mBuilder->ReferenceFrame());

We should be using the container frame's scrolled root right? We need to ensure all items have the same activeScrolledRoot.

::: layout/base/FrameLayerBuilder.h
@@ +441,5 @@
>      LayerState      mLayerState;
>      nsRefPtr<LayerManager> mInactiveManager;
>      nsRefPtr<nsDisplayItemGeometry> mGeometry;
>      bool            mUsed;
> +    PRUint32        mContainerLayerGeneration;

Move this up next to mDisplayItemKey, and move mLayerState down next to mUsed

@@ +523,5 @@
>      nsDisplayItem* mItem;
>      Clip mClip;
>      nsRefPtr<LayerManager> mInactiveLayer;
>      bool mShouldPaint;
> +    PRUint32 mContainerLayerGeneration;

Move this up above the bool

@@ +547,5 @@
>      // The translation set on this ThebesLayer before we started updating the
>      // layer tree.
>      nsIntPoint mLastPaintOffset;
>      bool mHasExplicitLastPaintOffset;
> +    PRUint32 mContainerLayerGeneration;

Move this up above the bool

@@ +609,5 @@
>     * during this paint.
>     */
>    bool                                mInvalidateAllLayers;
> +
> +  PRUint32                            mContainerLayerGeneration;

Should this be truly global across FrameLayerBuilders? I think so. Once you make FrameLayerBuilders per-layer-manager, we could possibly have collisions when a frame has one item that's in the retained layer tree and other items rendered using fallback BasicLayerManager?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> This still doesn't mark the container frame with a flag so that we can avoid
> this for subsequent paints. Are there any conditions where we would want to
> remove this flag? Or can it just remain for the lifetime of the frame.

I'd just let it remain the lifetime of the frame.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or maybe remove it on a timer or something.

Comment 11

[Matt Woodrow (:mattwoodrow)]

 
> Should this be truly global across FrameLayerBuilders? I think so. Once you
> make FrameLayerBuilders per-layer-manager, we could possibly have collisions
> when a frame has one item that's in the retained layer tree and other items
> rendered using fallback BasicLayerManager?

Will that matter? The outer display item (say the nsDisplayTransform) will still be in the retained layer tree's layer, and that will get removed.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Approach 3: Repeat BuildContainerLayerFor v3

Fixed review comments, and a few bugs.

One bug was with reusing generation id's, something that happened when we hit multiple BuildContainerLayerFor calls when processing a single display lists. These would get the same generation id, and could wipe out entries created by a previous one.

I've also stopped converting nsDisplayOwnLayer/nsDisplayZoom into inactive layers, since this breaks after paint events with DLBI. I think this is the right way to go, since I doubt we want these flattened into their parent regardless.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 613481 [details] [diff] [review]
Approach 3: Repeat BuildContainerLayerFor v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1560,5 @@
> +        nsLayoutUtils::GetActiveScrolledRootFor(mContainerFrame,
> +                                                mBuilder->ReferenceFrame());
> +      if (item->GetType() != nsDisplayItem::TYPE_OWN_LAYER && 
> +          item->GetType() != nsDisplayItem::TYPE_ZOOM) {
> +        layerState == LAYER_INACTIVE;

This test is not very efficient, and not very maintainable.

How about if layerState is LAYER_ACTIVE, switch it to INACTIVE. And ensure those two item types return LAYER_ACTIVE_FORCE.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Approach 3: Repeat BuildContainerLayerFor v4

Fixed review comments

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 618540 [details] [diff] [review]
Approach 3: Repeat BuildContainerLayerFor v4

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2311,5 @@
> +    pixBounds = state.ScaleToOutsidePixels(bounds, false);
> +    appUnitsPerDevPixel = state.GetAppUnitsPerDevPixel();
> +
> +#ifndef MOZ_GFX_OPTIMIZE_MOBILE
> +    if ((flags & Layer::CONTENT_COMPONENT_ALPHA) &&

This #ifndef isn't really right. Why do we need it here? I would have thought that on mobile we're not using subpixel AA so flags & Layer::CONTENT_COMPONENT_ALPHA will never be true.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Do we? I couldn't find the code where that happened.

I thought we created layers with the CONTENT_COMPONENT_ALPHA flag, but the backends just choose not to handle them that way.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When we aren't using component alpha for text, we should *definitely* let nsDisplayText etc know about that and return an empty region from GetComponentAlphaBounds. That will make things more efficient!

Comment 18

[Matt Woodrow (:mattwoodrow)]

Did you want that to be part of this patch/bug? Or should I file a follow-up?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should do it here. This #ifndef simply isn't correct if we're using component alpha on mobile.

I'm not sure of the best way to do this, but any easy way that's probably good enough would be to have a method on gfxPlatform that tells if component alpha is globally enabled or disabled for screen drawing. Then the various display items can quickly test that.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: WIP fix

I accidentally started working on this. Here's how far I got.

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> I think we should do it here. This #ifndef simply isn't correct if we're
> using component alpha on mobile.
> 
> I'm not sure of the best way to do this, but any easy way that's probably
> good enough would be to have a method on gfxPlatform that tells if component
> alpha is globally enabled or disabled for screen drawing. Then the various
> display items can quickly test that.

This might be better as a LayerManager decision, stored on the nsDisplayListBuilder.

This being because we don't necessarily disable component alpha on android as such, but instead it's a BasicLayers decision related to shadow layers (MustRetainContent).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Thinking about this more, I don't think it will work.

It appears that mobile *does* use subpixel text, just not when it's over transparent pixels within the layer.

We need GetComponentAlphaBounds() to continue returning valid areas so that we can set SetSubpixelAntialiasingEnabled(false) on the surface when we attempt to draw text over transparent pixels.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK

Comment 25

[Matt Woodrow (:mattwoodrow)]

Attachment: Approach 3: Repeat BuildContainerLayerFor v5

Rebased many times. More or less identical to the previous one though, including (unfortunately) the android #define.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 631194 [details] [diff] [review]
Approach 3: Repeat BuildContainerLayerFor v5

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

r+ if the #define is moved behind LayerManager::AreComponentAlphaLayersEnabled as discussed.

Comment 27

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/330a086f1570

Comment 28

[Ed Morley [:emorley]]

Ok, so this is pretty sad faces, but I've had to back this out for causing mochitest-4 permaorange (as well as the talos regressions in comment 237):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61fd66629c4f

eg https://tbpl.mozilla.org/php/getParsedLog.php?id=12544443&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08886a8cf22

It's for times like these that a relanding script would be pretty useful, for automating the qimport of a lange range of changesets (or at least it would make me feel less bad about having to back things like this out). 

Sorry! :-(

Comment 29

[Jonathan Kew [:jfkthame]]

A minor point, perhaps, but I found it a bit confusing that the bug title here is "Support component alpha with BasicLayers", but the patch has a check-in comment that says "Disable component alpha layers with BasicLayers".

So, umm, are we actually supporting or disabling component alpha here?

Comment 30

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65410094add4

Apologies, still left the confusing commit message here. We are collapsing layers that require component alpha into their parent layer so that we still get subpixel AA for text.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65410094add4

Comment 32

[Matt Woodrow (:mattwoodrow)]

This was backed out with bug 539356.

Comment 33

[Matt Woodrow (:mattwoodrow)]

Attachment: Add component alpha bounds and opaque regions to display list dumping

Comment 34

[Matt Woodrow (:mattwoodrow)]

Attachment: Add nsRegion::ScaleToInsidePixels

This adds nsRegion::ScaleToInsidePixels and nsRegion::ScaleToNearestPixels.

It also attempts to preserve contiguous regions instead of naively snapping to pixels.

Comment 35

[Matt Woodrow (:mattwoodrow)]

Attachment: Use nsRegion::ScaleToInsidePixels when computing opaque regions and return the correct component alpha region for nsTextBoxFrame

Comment 36

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove unnecessary scrollbars from test

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 644678 [details] [diff] [review]
Add nsRegion::ScaleToInsidePixels

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

::: gfx/2d/BaseRect.h
@@ +277,5 @@
>    T YMost() const { return y + height; }
>  
> +  void SetX(T aX) {
> +    width = XMost() - aX;
> +    x = aX;

SetLeftEdge? And add a comment to explain that this sets the left edge while preserving the right edge. Add an assertion that the new width >= 0.

@@ +281,5 @@
> +    x = aX;
> +  }
> +  void SetY(T aY) {
> +    height = YMost() - aY;
> +    y = aY;

SetTopEdge?

@@ +284,5 @@
> +    height = YMost() - aY;
> +    y = aY;
> +  }
> +  void SetXMost(T aXMost) { width = aXMost - x; }
> +  void SetYMost(T aYMost) { height = aYMost - y; }

SetRightEdge, SetBottomEdge?

::: gfx/src/nsRegion.cpp
@@ +1410,5 @@
> +nsIntRegion nsRegion::ScaleToInsidePixels (float aScaleX, float aScaleY,
> +                                           nscoord aAppUnitsPerPixel) const
> +{
> +  /* When scaling a rect walk forward through the rect list up until the y value
> +   * than our YMost() value. This checks all rects with equal or greater y coordinates.

The first sentence doesn't quite parse.

@@ +1438,5 @@
> +  {
> +    nsIntRect deviceRect =
> +      pRect->ScaleToInsidePixels(aScaleX, aScaleY, aAppUnitsPerPixel);
> +
> +    if (pRect != first && pRect->y <= first->YMost()) {

pRect can't be equal to first

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 644679 [details] [diff] [review]
Use nsRegion::ScaleToInsidePixels when computing opaque regions and return the correct component alpha region for nsTextBoxFrame

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

r+ on the FrameLayerBuilderChange; you could split that into a separate patch if you want

::: layout/xul/base/src/nsTextBoxFrame.cpp
@@ +928,5 @@
>  
>  nsRect
>  nsTextBoxFrame::GetComponentAlphaBounds()
>  {
> +  return mTextDrawRect;

handle the shadows

Comment 39

[Matt Woodrow (:mattwoodrow)]

Attachment: Make nsTextBoxFrame::GetComponentAlphaBounds more accurate

Comment 40

[Matt Woodrow (:mattwoodrow)]

Attachment: Use nsRegion::ScaleToInsidePixels when computing opaque regions

Carrying forward r=roc

Comment 41

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e543fbd7169a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f758ea29194a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a688930bc96
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32d7b59cec9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a98f847bb8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ee3b2e133a

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e543fbd7169a
https://hg.mozilla.org/mozilla-central/rev/f758ea29194a
https://hg.mozilla.org/mozilla-central/rev/3a688930bc96
https://hg.mozilla.org/mozilla-central/rev/d32d7b59cec9
https://hg.mozilla.org/mozilla-central/rev/7a98f847bb8e
https://hg.mozilla.org/mozilla-central/rev/a5ee3b2e133a

Comment 43

[Matt Brubeck (:mbrubeck)]

This landing caused a tscroll regression which appears to be fixed by bug 772690.