602428 - Implement shadow layers for GL layers backend (basically, slowly)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I have patches to do this, but they uncovered several problems with the shadow tree setup that we've been "getting away with" in basic layers because of gfxContext.  GL layers follow the layer tree more closely; there's no CTM to bail us out in shadow land.

Comment 1

[Brian Crowder]

Making you (cjones) own this for now.  Disown as you deem necessary.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 0: Add NSPR layers logging to OGL backend and log ThebesLayer resolution

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Implement HW-decelerating ShadowColorLayerOGL

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement HW-decelerating ShadowCanvasLayerOGL

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Implement HW-decelerating ShadowImageLayerOGL

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Implement HW-decelerating ShadowThebesLayerOGL

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 5: Implement HW-decelerating ShadowContainerLayerOGL

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 6: Add allocators for shadow OGL layers

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 7: Enable GL shadow layers

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 482470 [details] [diff] [review]
part 1: Implement HW-decelerating ShadowColorLayerOGL

I don't actually see ShadowColorLayerOGL in this, only BasicShadowColorLayer, but that looks OK even so.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Implement HW-decelerating ShadowColorLayerOGL

Ugh, wrong patch.  Apologies.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 482472 [details] [diff] [review]
part 3: Implement HW-decelerating ShadowImageLayerOGL


>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp

>+already_AddRefed<gfxSharedImageSurface>
>+ShadowImageLayerOGL::Swap(gfxSharedImageSurface* aNewFront)
>+{
>+  if (!mDestroyed && mTexImage) {
>+    // XXX this is always just ridiculously slow
>+
>+    gfxSize sz = aNewFront->GetSize();
>+    nsIntRegion updateRegion(nsIntRect(0, 0, sz.width, sz.height));
>+    gfxContext* dest = mTexImage->BeginUpdate(updateRegion);

nsRefPtr<gfxContext> please.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #12)
> >diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp
> 
> >+already_AddRefed<gfxSharedImageSurface>
> >+ShadowImageLayerOGL::Swap(gfxSharedImageSurface* aNewFront)
> >+{
> >+  if (!mDestroyed && mTexImage) {
> >+    // XXX this is always just ridiculously slow
> >+
> >+    gfxSize sz = aNewFront->GetSize();
> >+    nsIntRegion updateRegion(nsIntRect(0, 0, sz.width, sz.height));
> >+    gfxContext* dest = mTexImage->BeginUpdate(updateRegion);
> 
> nsRefPtr<gfxContext> please.

That's a gfxContext owned by the TextureImage which isn't supposed to escape BeginUpdate/EndUpdate, which is why the bare pointer is used.  But I can use RefPtr with a comment about the lifetime.  (Kinda wish we had AutoUnassignableRefPtr or something.)

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 482471 [details] [diff] [review]
part 2: Implement HW-decelerating ShadowCanvasLayerOGL

>+already_AddRefed<gfxSharedImageSurface>
>+ShadowCanvasLayerOGL::Swap(gfxSharedImageSurface* aNewFront)
>+{
>+  if (!mDestroyed && mTexImage) {
>+    // XXX this is always just ridiculously slow
>+
>+    gfxSize sz = aNewFront->GetSize();
>+    nsIntRegion updateRegion(nsIntRect(0, 0, sz.width, sz.height));
>+    gfxContext* dest = mTexImage->BeginUpdate(updateRegion);
>+
>+    dest->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    dest->DrawSurface(aNewFront, aNewFront->GetSize());
>+
>+    mTexImage->EndUpdate();
>+  }

gfxSharedImageSurface is always an image surface, right?  In that case, we really should skip the entire BeginUpdate/EndUpdate path here -- all it's going to do is allocate a temporary surface so that we can do the texture upload, however it wants to do it.  Instead, we should add a new API on TextureImage that's something like SetImage that takes a gfxImageSurface and an optional origin (assumed to be 0,0) -- in which case it'll just call Tex[Sub]Image2D directly, skipping all the intermediate goop...

It might also want to be able to return false to say "you can't set this", but I think we should try to support it everywhere; I'm thinking there may be difficulties if we try to use xlib-backed texture-from-pixmap things, but in this case I don't think we'd even want to -- we'd never draw directly to the xlib surface.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 482474 [details] [diff] [review]
part 5: Implement HW-decelerating ShadowContainerLayerOGL

Container stuff looks fine, but I would much prefer if the templated methods, such as:

>+ContainerInsertAfter(Layer* aChild, Layer* aAfter, Container* aContainer)

took the container as the first parameter, instead of the last.  It's a bit nitpicky, but...

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 482471 [details] [diff] [review]
part 2: Implement HW-decelerating ShadowCanvasLayerOGL

looks fine though, the teximage2d comment can be implemented as a followup

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #15)
> Comment on attachment 482474 [details] [diff] [review]
> part 5: Implement HW-decelerating ShadowContainerLayerOGL
> 
> Container stuff looks fine, but I would much prefer if the templated methods,
> such as:
> 
> >+ContainerInsertAfter(Layer* aChild, Layer* aAfter, Container* aContainer)
> 
> took the container as the first parameter, instead of the last.  It's a bit
> nitpicky, but...

Done.

(In reply to comment #16)
> Comment on attachment 482471 [details] [diff] [review]
> part 2: Implement HW-decelerating ShadowCanvasLayerOGL
> 
> looks fine though, the teximage2d comment can be implemented as a followup

Filed bug 604101.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I also split up part 6 into a 0.5 to add allocators that return NULL and part 6 that returns real objects, so that the intermediate patches compile and qfolding them won't be needed.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/fa56d3dd6207
http://hg.mozilla.org/mozilla-central/rev/e6ea9412c9ab
http://hg.mozilla.org/mozilla-central/rev/74997e213b52
http://hg.mozilla.org/mozilla-central/rev/629729b11dc6
http://hg.mozilla.org/mozilla-central/rev/399761bb8968
http://hg.mozilla.org/mozilla-central/rev/22e4c83016ec
http://hg.mozilla.org/mozilla-central/rev/a177f5dea9fd
http://hg.mozilla.org/mozilla-central/rev/d31e498d50a6
http://hg.mozilla.org/mozilla-central/rev/0c9ae492cf5f