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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 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
: 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
: 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: --- → ?
tracking-fennec: ? → 2.0b2+

Comment 1

9 years ago
Making you (cjones) own this for now.  Disown as you deem necessary.
Assignee: nobody → jones.chris.g
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+
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.)
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+
(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.

Updated

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