Extend bug 615870 to support DoEmptyTransaction() on D3D layer managers. This will speed up video and async plugin rendering on Windows.

This is on top of the patches for bug 593604.

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.

(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?

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.

Comment on attachment 500263 [details] [diff] [review] Part 1: EndEmptyTransaction API change I only reviewed the stuff in layout/.

Comment on attachment 500265 [details] [diff] [review] Part 2: D3D9 support These patches block a blocker (602080)

Comment on attachment 500266 [details] [diff] [review] Part 3: D3D10 support These patches block a blocker (602080)

(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

Don't allow empty transactions if device has been reset

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.

Trivial patch

http://hg.mozilla.org/mozilla-central/rev/a18080aa75d6 http://hg.mozilla.org/mozilla-central/rev/4fc581f1ff00 http://hg.mozilla.org/mozilla-central/rev/1d293c9ffa95 http://hg.mozilla.org/mozilla-central/rev/710c1a6faf99

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.

(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.