Closed Bug 741682 Opened 12 years ago Closed 12 years ago

Support component alpha with BasicLayers

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(6 files, 8 obsolete files)

Approach 3: Repeat BuildContainerLayerFor v5
27.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
Add component alpha bounds and opaque regions to display list dumping
2.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
Add nsRegion::ScaleToInsidePixels
6.70 KB, patch
roc
: review+
Details | Diff | Splinter Review
Remove unnecessary scrollbars from test
1.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
Make nsTextBoxFrame::GetComponentAlphaBounds more accurate
848 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Use nsRegion::ScaleToInsidePixels when computing opaque regions
3.81 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Attached patch WIP v1 (obsolete) — — Splinter Review 
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.
Attachment #611711 - Flags: feedback?(roc)
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)
Attachment #612036 - Flags: review?(roc)
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.
Attachment #612439 - Flags: review?(roc)
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.
Attachment #611711 - Attachment is obsolete: true
Attachment #612036 - Attachment is obsolete: true
Attachment #612439 - Attachment is obsolete: true
Attachment #611711 - Flags: feedback?(roc)
Attachment #612036 - Flags: review?(roc)
Attachment #612439 - Flags: review?(roc)
Attachment #612449 - Flags: review?(roc)
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.
Attachment #612449 - Attachment is obsolete: true
Attachment #612449 - Flags: review?(roc)
Attachment #613481 - Flags: review?(roc)
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
Attachment #613481 - Attachment is obsolete: true
Attachment #613481 - Flags: review?(roc)
Attachment #618540 - Flags: review?(roc)
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.
Attached patch WIP fix (obsolete) — — Splinter Review 
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).
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.
Rebased many times. More or less identical to the previous one though, including (unfortunately) the android #define.
Attachment #618540 - Attachment is obsolete: true
Attachment #618890 - Attachment is obsolete: true
Attachment #618540 - Flags: review?(roc)
Attachment #631194 - Flags: review?(roc)
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.
Attachment #631194 - Flags: review?(roc) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/65410094add4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
This was backed out with bug 539356.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This adds nsRegion::ScaleToInsidePixels and nsRegion::ScaleToNearestPixels.

It also attempts to preserve contiguous regions instead of naively snapping to pixels.
Attachment #644678 - Flags: review?(roc)
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
Attachment #644678 - Flags: review?(roc) → review+
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
Attachment #644679 - Attachment is obsolete: true
Attachment #644679 - Flags: review?(roc)
Attachment #644685 - Flags: review+
This landing caused a tscroll regression which appears to be fixed by bug 772690.
Depends on: 772690
Depends on: 777617
Depends on: 777654
Depends on: 781414
Depends on: 789396
Blocks: 790124
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: