592870 - Make personas caption buttons work with d3d9 layers

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: First try

Joe and Bas tell me that with ded9 layers enabled, the region of the window's caption buttons are plain white (instead of transparent which would make the buttons visible).

Hopefully with the change in approach in this patch this will be solved. At the moment, we remove the caption button's area from our drawing region, but probably the surface is cleared to white first.

With this patch we don't touch the paint region. Instead we let all painting occur normally, and afterwards we paint transparent (using operator_clear) the area on the thebesSurface where the caption buttons are.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Yeah this doesn't work, the accelerated layers manager doesn't follow that code path.

Maybe we still should take this patch though, I don't know. I guess it will depend on what the code to support accelerated layers will need.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Second try

Now this is interesting, if we simply paint the DC with the black brush after all the painting happened, we can see the caption buttons. Probably the way the black brush works is setting the alpha to 0 on those pixels (which is ultimately what we want, but not sure if this is the right way).

This works for GDI, d2d, d3d9.  The only problem is that the buttons flicker a bit specially when moving the mouse over the firefox button area.

Bas: any feedback? Is this working by lucky? If you have ideas on what's the correct way to do this let me know

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 471379 [details] [diff] [review]
Second try

So, this working on D3D9, I believe is mostly because of luck. Same for D2D, GDI this is at least a better approach. What we should do to make this better for both GDI and D2D is do this from the context, i.e.

(pseudo code)

context->NewPath();
foreach (rectangle in region)
  context->Rectangle(rect);
context->SetOperator(OPERATOR_CLEAR);
context->Fill();

I believe this is the best performing approach for D2D and GDI.

For hardware accelerated layers it becomes a bit trickier, since they don't follow this code at all. They shouldn't even be creating a cairo context there (see the LayerManager type switch statement).

Here we should probably just ensure from the users of the layermanager that the region of the caption buttons is ensured to be transparent. But I don't see why it wouldn't just be that, and why it would have white there in the first place.

Comment 4

[Bas Schouten (:bas.schouten)]

Roc might have useful input.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does the D3D9 layer manager need to clear its backbuffer to completely transparent before painting?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #3)
> Comment on attachment 471379 [details] [diff] [review]
> Second try
> 
> So, this working on D3D9, I believe is mostly because of luck. Same for D2D,
> GDI this is at least a better approach. What we should do to make this better
> for both GDI and D2D is do this from the context, i.e.
> 
> (pseudo code)
> 
> context->NewPath();
> foreach (rectangle in region)
>   context->Rectangle(rect);
> context->SetOperator(OPERATOR_CLEAR);
> context->Fill();
> 
> I believe this is the best performing approach for D2D and GDI.

Ok, that's what patch v1 does

FTR I did some quick measurements and AFAICT we very rarely create a complex region by excluding the caption buttons region, because the invalidated region usually does not intersect with that (unless something is forcing full repaints)

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 471379 [details] [diff] [review]
Second try

We should only do the fillrect thing when gfxWindowsPlatform::GetPlatform()->GetRenderMode() == RENDER_GDI. Other than that I've figured out why it works, and it's fine.

What was actually breaking this for D3D9 layers is that the caption button area was never invalidated, and also never drawn to (GDI doesn't work here), the area would therefor keep the white, initial content of the window. Invalidating it with D3D9 layers will make it transparent, and blend properly.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix up comments from Bas

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 471949 [details] [diff] [review]
Fix up comments from Bas

The original patch (patch v2) removed the code to subtract the caption buttons region from the event.region, and just always drew black on the DC, which made the buttons area transparent. If we then do that only for GDI, the caption buttons won't show up anymore with d2d or accelerated layers.

I think that the best to do would be:

if (basic layers) {
  paint transparent on the thebesContext, as suggested on comment #3
} else {
  event.region.Sub(mCaptionButtons);
  Fix why this doesn't work on d3d9 (comment #7). note: this already works for OGL
}


P.S.: As the caption buttons always show white with d3d9 right now (personas or not), if the goal is really to enable d3d9 today, we can make the buttons work correctly without personas (by backing out the personas bug or applying patch v1 here) and break personas again, and then we set this as blocking b6

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: New patch

So for now we can use the technique of clearing the DC which makes it work for d3d9, but not remove the event.region part yet because that makes basic layers + d2d flicker a lot.

After the invalidation issue is fixed I think we'll be able to remove  this and do something like comment #9.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 472097 [details] [diff] [review]
New patch

Good enough for now.

Comment 13

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/79477a6f41b2

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to comment #11)
> Created attachment 472097 [details] [diff] [review]
> New patch
> 
> So for now we can use the technique of clearing the DC which makes it work for
> d3d9, but not remove the event.region part yet because that makes basic layers
> + d2d flicker a lot.
> 
> After the invalidation issue is fixed I think we'll be able to remove  this and
> do something like comment #9.

It's probably better if we do only do this once on the windows first paint. Every subsequent time we do this I believe is useless in the case of hardware accelerated layers. We'd need to verify this though.