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 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.

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 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?

(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.

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.

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.

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 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?

(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.

Or maybe remove it on a timer or something.

> 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.

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 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.

Fixed review comments

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.

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.

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!

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

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.

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

(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).

OK.

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.

OK

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

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.

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! :-(

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?

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.

This was backed out with bug 539356.

This adds nsRegion::ScaleToInsidePixels and nsRegion::ScaleToNearestPixels. It also attempts to preserve contiguous regions instead of naively snapping to pixels.

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 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

Carrying forward r=roc

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

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

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