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

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(8 attachments, 1 obsolete attachment)

2.47 KB, patch
vlad
: review+
Details | Diff | Splinter Review
5.51 KB, patch
vlad
: review+
Details | Diff | Splinter Review
5.41 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
11.25 KB, patch
vlad
: review+
Details | Diff | Splinter Review
16.43 KB, patch
vlad
: review+
Details | Diff | Splinter Review
5.10 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.88 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.60 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
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.
Blocks: 598864
tracking-fennec: --- → ?
Depends on: 602431

Updated

7 years ago
tracking-fennec: ? → 2.0b2+

Comment 1

7 years ago
Making you (cjones) own this for now.  Disown as you deem necessary.
Assignee: nobody → jones.chris.g
Created attachment 482469 [details] [diff] [review]
part 0: Add NSPR layers logging to OGL backend and log ThebesLayer resolution
Attachment #482469 - Flags: review?(vladimir)
Created attachment 482470 [details] [diff] [review]
part 1: Implement HW-decelerating ShadowColorLayerOGL
Attachment #482470 - Flags: review?(joe)
Created attachment 482471 [details] [diff] [review]
part 2: Implement HW-decelerating ShadowCanvasLayerOGL
Attachment #482471 - Flags: review?(vladimir)
Created attachment 482472 [details] [diff] [review]
part 3: Implement HW-decelerating ShadowImageLayerOGL
Attachment #482472 - Flags: review?(joe)
Created attachment 482473 [details] [diff] [review]
part 4: Implement HW-decelerating ShadowThebesLayerOGL
Attachment #482473 - Flags: review?(vladimir)
Created attachment 482474 [details] [diff] [review]
part 5: Implement HW-decelerating ShadowContainerLayerOGL
Attachment #482474 - Flags: review?(joe)
Created attachment 482475 [details] [diff] [review]
part 6: Add allocators for shadow OGL layers
Attachment #482475 - Flags: review?(vladimir)
Created attachment 482476 [details] [diff] [review]
part 7: Enable GL shadow layers
Attachment #482476 - Flags: review?(tnikkel)
Attachment #482476 - Flags: review?(tnikkel) → review+
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.
Attachment #482470 - Flags: review?(joe) → review+
Created attachment 482649 [details] [diff] [review]
part 1: Implement HW-decelerating ShadowColorLayerOGL

Ugh, wrong patch.  Apologies.
Attachment #482470 - Attachment is obsolete: true
Attachment #482649 - Flags: review?
Attachment #482649 - Flags: review? → review?(joe)
Attachment #482649 - Flags: review?(joe) → review+
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.
Attachment #482472 - Flags: review?(joe) → review+
(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.)
Attachment #482469 - Flags: review?(vladimir) → review+
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 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...
Attachment #482474 - Flags: review?(joe) → review+
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
Attachment #482471 - Flags: review?(vladimir) → review+
Attachment #482473 - Flags: review?(vladimir) → review+
Attachment #482475 - Flags: review?(vladimir) → review+
(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.
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.
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Blocks: 618261
You need to log in before you can comment on or make changes to this bug.