634521 - Let Mac plugins trigger empty transactions

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

If async plugin rendering for mac is going to be too far down the road, we should let mac plugins trigger empty transactions in the meantime.

Currently painting is triggered from within BuildLayer, we need to move this to somewhere that will still run during an empty transaction. nsPluginInstanceOwner::Invalidate should probably schedule this somehow.

Comment 1

[Joe Drew (not getting mail)]

Does this need to block? Does it need to block beta N? Is it a hardblocker?

It's not clear to me whether the patches in bug 591687 are enough to consider Flash performance "good enough", or how much we gain further from plugins triggering empty transactions.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I'd go for yes on blocking, less sure if we need beta coverage.

The performance differences I noticed (from looking at activity monitor) we more substantial from this change than in bug 591687. However I think bug 591687 is hardware dependant, and the readback cost of my onboard 9400M is lower than separate cards.

I can try get figures if we need them.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Does anyone have suggestions for how to implement this?

I have a work in progress patch that manually triggers the paint event inside ImageLayerOGL::RenderLayer (similar to how ThebesLayers are drawn).

We can't really call the draw event from inside Invalidate since it can happen multiple times per frame. We could flag as needing to redraw, but we still need an external trigger that will happen even with an empty transaction.

Comment 4

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

With CoreAnimation the draw event is super-cheap right? Because it doesn't actually draw anything, just grabs an existing IOSurface. Are you sure it hurts to call it from inside Invalidate?

Comment 5

[Matt Woodrow (:mattwoodrow)]

I got 1.4% CPU usage last time I ran it with shark. I'm not sure if this is just the cost of synchronizing across processes, or actual time spent handling the event.

Comment 6

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

OK, but how much of that CPU usage is due to multiple calls with no intervening paint?

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: First attempt

I decided to add the callback onto MacIOSurfaceImage since it makes it very contained and easy to remove once async plugin rendering gets landed.

Adding a generic callback list to LayerManager(OGL?) seemed very hard to get a callback prototype that would be useful for future unknown situations. We also need to track the lifetimes of these callbacks and make sure they don't get added twice for a layer, and removed when the layer is removed from the layer tree.

This seems like extra unnecessary work when we only have a single use case that should have a very short lifetime.

We may still need a Validate() cycle so that these callbacks happen before ComputeEffectiveTransforms(), this seems a lot more flexible and will be useful for syncing when we get content acceleration. I'll add this soon.

Looks like this patch works anyway, and CPU usage drops considerably.

Comment 8

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

Adding the callback on the image is kinda crazy since all the callback will do is replace the image in its imagecontainer with a different image. But OK, maybe this is the best thing to do for now. If this hack lasts long I'll be deeply unhappy.

Can we at least remove nsObjectFrame from ImageLayers.h and use a void* instead? And don't call it a DrawPluginCallback, call it UpdateSurfaceCallback or something like that instead. Document that these things are temporary hacks that should NOT be used.

nsObjectFrame::UpdateImageLayer shouldn't need to do anything other than DoCocoaEventDrawRect and SetCurrentImage. The rest should be unchanged since the last paint. If it's not, we shouldn't be doing an empty transaction.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #8)
 If this hack lasts long I'll be
> deeply unhappy.

You and me both. I'm not sure what exactly is involved in adding async plugin support, but I'm happy to look into this post 4.0.

> 
> Can we at least remove nsObjectFrame from ImageLayers.h and use a void*
> instead? And don't call it a DrawPluginCallback, call it UpdateSurfaceCallback
> or something like that instead. Document that these things are temporary hacks
> that should NOT be used.

Sure.

> 
> nsObjectFrame::UpdateImageLayer shouldn't need to do anything other than
> DoCocoaEventDrawRect and SetCurrentImage. The rest should be unchanged since
> the last paint. If it's not, we shouldn't be doing an empty transaction.


Right, so we won't need the validate loop?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix v2

Comment 11

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

Comment on attachment 513002 [details] [diff] [review]
Fix v2

+MacIOSurfaceImageOGL::Update(ImageContainer* aContainer)
+{
+  mCallback(aContainer, mObjectFrame);

Make this if (mCallback)

+      MacIOSurfaceImage *oglImage = static_cast<MacIOSurfaceImage*>(image.get());
+      oglImage->SetCallback(&DrawPlugin, mObjectFrame);

Wrap this in an image type check for safety.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix v3

Fixed review comments, carrying forward r=roc

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix v4

Forgot to qref the most important change, actually re-enabling empty transactions for plugins.

Carrying forward r,a=roc

Comment 14

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/mozilla-central/rev/0ae820da6e20