Closed Bug 621601 Opened 9 years ago Closed 9 years ago

Support empty transactions with D3D layer managers

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: roc, Assigned: roc)

References

Details

(Whiteboard: [hardblocker])

Attachments

(4 files, 2 obsolete files)

Extend bug 615870 to support DoEmptyTransaction() on D3D layer managers. This will speed up video and async plugin rendering on Windows.
Attached patch D3D9 patch (obsolete) — Splinter Review
This is on top of the patches for bug 593604.
Attachment #499993 - Flags: review?(bas.schouten)
Comment on attachment 499993 [details] [diff] [review]
D3D9 patch

So, the part of this that really hurts me is it's like a big ugly hack :(. It doesn't make me very happy at all. If we need something like this I think it should be done with a more 'proper' refactoring of how drawing is coordinated.

I believe if we do want something like this we should design the API as follows:

BeginTransaction
TryEmptyTransaction
EndTransaction

TryEmptyTransaction which passes over all ThebesLayers and returns false if any of them were (partially) invalidated. Otherwise it recomposites.
EndTransaction will behave as usual.

What I'm unclear about is how we currently ever hit the 'mTransactionIncomplete' situation. If we do a DoEmptyTransaction we shouldn't have done anything that transaction, after all if we invalidated a thebes layer the transaction wasn't empty and DoEmptyTransaction should really be TryEmptyTransaction or something like that.

In any case we should then iterate over all our layers, verify they're all valid, if they're not return false without doing any rendering. Otherwise return true for TryEmptyTransaction and consider the transaction finished. In the false case no drawing will be done at all, and the transaction will not finish, and within this same transaction the display list should be prepared and drawing should be done. Considering we already require an immediate BeginTransaction/EndTransaction follow-up in the DoEmptyTransaction = false case the only real for difference for the API user is no extra BeginTransaction call. However this approach has numerous advantages:

1. No 'magical' incomplete state in which a LayerManager doesn't have a validated layer tree outside of a transaction. This is almost a requirement if we want to do asynchronous composition at a later stage. i.e. everything will be contained within a transaction like it should be imho.
2. Prevent the 'double' drawing case for partially transparent layers to a transparent background.
3. We can still build the display list only when we discover our transaction isn't empty.

The only downside would be an extra pass over all the layers. This really shouldn't take any significant amount of work and avoiding it seems like premature optimization to me.

>+  if (mTransactionIncomplete) {
>+    // Last transaction was incomplete. Don't do rendering setup again.
>+    mTransactionIncomplete = false;
>+  } else {
>+    if (!mSwapChain->PrepareForRendering()) {
>+      return;
>+    }
>+    deviceManager()->SetupRenderState();
> 
>-  device()->BeginScene();
>+    SetupPipeline();
>+
>+    device()->Clear(0, NULL, D3DCLEAR_TARGET, 0x00000000, 0, 0);
>+
>+    device()->BeginScene();
>+  }

I realize this is documented in the API but I think it's really odd behavior to require another BeginTransaction/EndTransaction pair to follow a failed transaction. It might even cause invalid rendering if we have a partially transparent layer with no opaque background that gets redrawn behind it in the layer tree. Since this will now be drawn twice without a clearing of the background inbetween.

I'm willing to change this to r+ if I'm convinced this isn't true.

>+  // If the transaction is incomplete, don't finalize rendering, let the
>+  // next transaction continue rendering.
>+  if (!mTransactionIncomplete) {
>+    device()->EndScene();
> 
>-  if (!mTarget) {
>-    const nsIntRect *r;
>-    for (nsIntRegionRectIterator iter(mClippingRegion);
>-         (r = iter.Next()) != nsnull;) {
>-      mSwapChain->Present(*r);
>+    if (!mTarget) {
>+      const nsIntRect *r;
>+      for (nsIntRegionRectIterator iter(mClippingRegion);
>+           (r = iter.Next()) != nsnull;) {
>+        mSwapChain->Present(*r);
>+      }
>+    } else {
>+      PaintToTarget();

We probably want to do an if (mTarget) { PaintToTarget(); return; } to prevent the nesting here, we assert !mTarget in the case of DoEmptyTransaction and !mTransactionIncomplete in the case of EndTransaction. So the combination mTransactionIncomplete && mTarget should never occur.
Attachment #499993 - Flags: review?(bas.schouten) → review-
(In reply to comment #2)
> What I'm unclear about is how we currently ever hit the
> 'mTransactionIncomplete' situation. If we do a DoEmptyTransaction we shouldn't
> have done anything that transaction, after all if we invalidated a thebes layer
> the transaction wasn't empty and DoEmptyTransaction should really be
> TryEmptyTransaction or something like that.

Yeah, in retrospect I guess for D3D9 we can't hit this. Then we can simplify down DoEmptyTransaction to just check mRoot and then do a normal Begin/EndTransaction. Same for D3D10.

(For BasicLayers we can't just do that because BasicLayers decides to not retain contents in some situations.)

I think you're also right that Begin/TryEmpty/EndTransaction would be a better API. I'll do that too.
Alright, what do you think of this?
Attachment #499993 - Attachment is obsolete: true
Attachment #500263 - Flags: review?
Attachment #500263 - Flags: review?(tnikkel)
Attachment #500263 - Flags: review?(bas.schouten)
Attachment #500263 - Flags: review?
Attached patch Part 2: D3D9 support (obsolete) — Splinter Review
Attachment #500265 - Flags: review?(bas.schouten)
Attachment #500263 - Flags: review?(bas.schouten) → review+
Comment on attachment 500265 [details] [diff] [review]
Part 2: D3D9 support

It is possible for a device reset to cause us to lose VRAM. In the situation of a device reset we'll also receive an invalidation of our window though, so a proper repaint should follow soon after if we ever have an empty transaction between the device being reset and the invalidation arriving.

Considering the rarity of that situation I think this is fine.
Attachment #500265 - Flags: review?(bas.schouten) → review+
Attachment #500266 - Flags: review?(bas.schouten) → review+
Comment on attachment 500263 [details] [diff] [review]
Part 1: EndEmptyTransaction API change

I only reviewed the stuff in layout/.
Attachment #500263 - Flags: review?(tnikkel) → review+
Comment on attachment 500265 [details] [diff] [review]
Part 2: D3D9 support

These patches block a blocker (602080)
Attachment #500265 - Flags: approval2.0?
Comment on attachment 500266 [details] [diff] [review]
Part 3: D3D10 support

These patches block a blocker (602080)
Attachment #500266 - Flags: approval2.0?
(In reply to comment #7)
> Comment on attachment 500265 [details] [diff] [review]
> Part 2: D3D9 support
> 
> It is possible for a device reset to cause us to lose VRAM. In the situation of
> a device reset we'll also receive an invalidation of our window though, so a
> proper repaint should follow soon after if we ever have an empty transaction
> between the device being reset and the invalidation arriving.
> 
> Considering the rarity of that situation I think this is fine.

Doesn't a device reset cause us to get a new layermanager? In which case, mRoot will be null so EndEmptyTransaction will return false.
(In reply to comment #11)
> (In reply to comment #7)
> > Comment on attachment 500265 [details] [diff] [review]
> > Part 2: D3D9 support
> > 
> > It is possible for a device reset to cause us to lose VRAM. In the situation of
> > a device reset we'll also receive an invalidation of our window though, so a
> > proper repaint should follow soon after if we ever have an empty transaction
> > between the device being reset and the invalidation arriving.
> > 
> > Considering the rarity of that situation I think this is fine.
> 
> Doesn't a device reset cause us to get a new layermanager? In which case, mRoot
> will be null so EndEmptyTransaction will return false.

No, it does not, in D3D10 this is true, in D3D9 we make an attempt to 'reset' the device (this doesn't exist in D3D10) since this is more efficient. And happens in D3D9 on Windows XP for example on a screen lock/unlock.
An invalidation of our window won't help at all then, because it will just be another empty transaction. We need to detect this reset and refuse to do an empty transaction after it.

Besides which, that means we can hit the NS_ERROR I added about null callbacks. We need to avoid that.

I guess where we call ResetEx and Reset in DeviceManagerD3D9::VerifyReadyForRendering, we can also do something to indicate the device was reset? Bump a serial number in the device manager and check that serial number from the layer manager, or keep a list of layer managers in the device manager and notify them all explicitly? Which would you prefer?

In VerifyReadyForRendering, why don't we call CleanResources and Reset swap chains after calling ResetEx?
Comment on attachment 500263 [details] [diff] [review]
Part 1: EndEmptyTransaction API change

Blocks a blocker
Attachment #500263 - Flags: approval2.0+ → approval2.0?
Attached patch Part 2 v2Splinter Review
Don't allow empty transactions if device has been reset
Attachment #500506 - Flags: review?(bas.schouten)
Attachment #500506 - Flags: review?(bas.schouten) → review+
Attachment #500265 - Attachment is obsolete: true
Attachment #500265 - Flags: approval2.0?
Attachment #500263 - Flags: approval2.0? → approval2.0+
Attachment #500266 - Flags: approval2.0? → approval2.0+
Attachment #500506 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs landing]
On try I got some test failures where we crash because an ImageLayerD3D10 has a null current image. I guess we used to not hit this before because whenever we had no image, we wouldn't even create a layer, but now we might have an empty transaction so the layer is not removed even though the image has been set to null (e.g. because the plugin is shutting down).

The ImageLayer implementations should simply not render anything when there's no current image. BasicImageLayer already does this.
Attachment #504949 - Flags: review?(bas.schouten) → review+
Depends on: 627383
Depends on: 627438
The patches for a18080aa75d6 caused a few regressions with elements going missing (bug 627383 and bug 627438) and in one instance, I saw the entire browser window become transparent.
No longer blocks: 627399
Depends on: 627399
(In reply to comment #19)
> The patches for a18080aa75d6 caused a few regressions with elements going
> missing (bug 627383 and bug 627438) and in one instance, I saw the entire
> browser window become transparent.

Since they plan to branch b10 tomorrow, if these can't be fixed in time I think it is probably best to back out this.
I'm landing a patch to bug 627399 soon which hopefully fixes all of this.
blocking2.0: --- → betaN+
Whiteboard: [hardblocker]
Attachment #504949 - Flags: approval2.0? → approval2.0+
Depends on: 635928
No longer depends on: 635928
Depends on: 637824
You need to log in before you can comment on or make changes to this bug.