895369 - Add asserts to ThebesLayerBuffer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

ThebesLayerBuffer is an absolute mess. It handles buffer rotation and non-rotated buffers, Azure or Thebes drawing, component alpha and regular buffers; it interacts with single and double buffered content clients in odd ways; optionally works with texture clients (I guess that is only for OMTC, but seems to be not always). Many of these paths do not work with each and there is little to ensure internal consistency. For example, using Azure with a single buffered content client crashes. Azure and component alpha don't work together. There are overloaded methods and weird implicit dependencies. BeginPaint is one of the hairiest methods I've ever seen, and there is some pretty stiff competition around.

I believe buffer rotation might be going away at some point? Presumably once we have tiled thebes layers everywhere. That might take a while. It is probably worth deciding whether we should refactor ThebeLayerBuffer before or after that. But we definitely should improve it.

In the mean time I'm going to insert as many assertions as I can.

Comment 1

[Nick Cameron [:nrc]]

Attachment: tlb_asserts.patch

I'm not going to start refactoring, but here are some asserts to make it a little tiny bit more obvious when we go wrong sometimes.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for the asserts, that will help.  Long term, we're getting rid of Thebes, would this refactoring be a first step to make that easier, or can we skip this bug if Thebes disappears?

Comment 3

[Matt Woodrow (:mattwoodrow)]

I think we'll still need this class, or at least something that provides the equivalent functionality. A new name at least would be good.

We currently have lots of branches for azure vs thebes. All of those can go away once we have azure everywhere.

Comment 4

[Nick Cameron [:nrc]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Thanks for the asserts, that will help.  Long term, we're getting rid of
> Thebes, would this refactoring be a first step to make that easier, or can
> we skip this bug if Thebes disappears?

Yeah, what Matt said. This is another class called ThebesSomething even though it is not part of Thebes and doesn't even rely on Thebes specifically.

Comment 5

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c3d36fa76e

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c3c3d36fa76e

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Nick, does it make sense to keep this bug still open?

Comment 8

[Nick Cameron [:nrc]]

I have no idea sorry. I haven't worked in the area for more than five years.

Perhaps Matt Woodrow or Nical know?

Comment 9

[Nicolas Silva [:nical]]

Thebes/Content layers is not the nicest area of the code but Ryan improved it quite a bit.