598425 - Asynchronous layer-based plugin painting on Mac

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

+++ This bug was initially created as a clone of Bug #596451 +++Asynchronous layer-based plugin painting on Mac.

Comment 1

[Benjamin Smedberg]

Josh, should this be yours?

Comment 2

[Josh Aas]

I may be able to take this at some point but I'm working on other blockers at the moment. I'll try to find an owner and take if myself if necessary.

Comment 3

[Benjamin Smedberg]

We're not going to block on this directly, although we still might take it to fix the performance issues which are a blocker.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: wip (SetWindow+SurfaceInit)

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 533425 [details] [diff] [review]
Async Core Animation

Feel free to delegate the review. I'm not sure who to ask.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v2

Added a check in UseAsyncRendering to check for Layers OPENGL

Comment 8

[Oleg Romashin (:romaxa)]

Comment on attachment 533447 [details] [diff] [review]
Async Core Animation v2


>     PRBool useAsyncRendering;
>+#ifndef XP_MACOSX
>     return (mInstance &&
>             NS_SUCCEEDED(mInstance->UseAsyncPainting(&useAsyncRendering)) &&
>             useAsyncRendering &&
>             (!mPluginWindow ||
>              mPluginWindow->type == NPWindowTypeDrawable));
>+#else
>+    nsRefPtr<ImageContainer> container = mObjectFrame->GetImageContainer();
>+    return (mInstance &&
>+            NS_SUCCEEDED(mInstance->UseAsyncPainting(&useAsyncRendering)) &&
>+            useAsyncRendering &&
>+            IsRemoteDrawingCoreAnimation() &&
>+            container && container->GetBackendType() == LayerManager::LAYERS_OPENGL
>+            );
>+#endif

would it be better ifdef only part of this condition and keep, common parts common


>bool
>PluginInstanceChild::ShowPluginFrame()
>{
....
>+        if (!SendShow(r, currSurf, &returnSurf)) {
>+            return false;
>+        }
>+
>+        return true;
>+    } else {
>+        NS_ERROR("Unsupported drawing model for async layer rendering");
>+        return false;
>+    }
>+#endif

I'm not  100% what is IOSurface, is it double buffered magically by default? don't you need cur/back surface, readback, background paint stuff ?

Comment 9

[Benoit Girard (:BenWa)]

(In reply to comment #8)
> 
> I'm not  100% what is IOSurface, is it double buffered magically by default?
> don't you need cur/back surface, readback, background paint stuff ?

We don't need readback, background paint since the surface has proper alpha channel support (unless I am missing something).

I'm not 100% sure if double buffering is required since this is shared video memory, rather then main memory like a Shmem. I haven't observed any issue yet. I discussed this with Joe and we were of the opinion that we could leave it like this and fix it later if we notice any issues.

Comment 10

[Benjamin Smedberg]

We'd obviously like to avoid readback. But I don't understand how you can have asynchronous painting without double buffering: aren't you either setting yourself up for shearing (which will be very noticeable on video sites), or having something in the OS start locking the surface? Unlike the current synchronous setup, we are going to be painting a surface to the screen at the same time we're asking Flash to draw to it (in a different process).

Unless I'm totally missing something, double buffering is the key ingredient which makes asynchronous painting possible.

Comment 11

[Oleg Romashin (:romaxa)]

Without double buffering you will get artifacts (partially painted layer), which will be visible if you open some high FPS flash example, and start scrolling firefox at the same time, so you will have a big chance that layer is painted while flash rendering into the same layer...

Comment 12

[Benoit Girard (:BenWa)]

I haven't seen any tearing or artifacts when testing this patch (1080p video with a non optimized build) but proof by example isn't convincing to prove that it will never happen. I will speak more with the graphics teams tomorrow to understand this.

I don't know enough about graphic hardware implementation but if rasterization is an atomic operation then no locking would be required?

If we can avoid double buffering that would save us from having to reinitialize the PBO/CARenderer and video memory giving us significant performance benefits.

Comment 13

[Karl Tomlinson (:karlt)]

The biggest risk is if this surface is passed to the plugin, the plugin may paint in stages (e.g. background before foreground).  If that is the same surface that is read asynchronously by the browser, then the browser may read only the background of a future paint.

I don't know about the limitations of the APIs involved but it should be possible to keep two video memory buffers and swap between them.

Comment 14

[Benoit Girard (:BenWa)]

(In reply to comment #13)
> The biggest risk is if this surface is passed to the plugin, the plugin may
> paint in stages (e.g. background before foreground).  If that is the same
> surface that is read asynchronously by the browser, then the browser may
> read only the background of a future paint.
> 

The plug-in does not draw directly to our surface, the plug-in draws to a CALayer. This provides double buffering in the plug-in process. I creates a test plug-in that draws 1,000,000 quads taking over 0.5 second per frame. The scene is always completed. (Browser is much more responsive with this patch BTW)

Sequence:
Plug-in draws to CALayer -> CALayer draws to IOSurface -> IOSurface is drawn

The only legitimate concern for artifacts is when drawing the CALayer into the IOSurface.

Here are the relevant section from the API of the function we use to draw the MacIOSurfaceImageOGL in the content process:
Snippet from:
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/OpenGL.framework/Versions/A/Headers/CGLIOSurface.h

> It is the rough equivalent to glTexImage2D(), except that
> the underlying source data comes from an IOSurface rather than from an explicit pointer.
> Note that unlike glTexImage2D(), the binding is "live", in that if the contents of
> the IOSurface change, the contents become visible to OpenGL without making another
> call to CGLTexImageIOSurface2D().   That being said, there are a few synchronization
> things to worry about if you are using IOSurface to pass data between contexts and/or
> different processes, or between the CPU and OpenGL.
>            
> In general IOSurface follows Apple's cross-context synchronization rules for OpenGL. Put
> simply, in order for changes done to an IOSurface on context A to become visible to context
> B, you must flush context A's command stream (via an explicit call to glFlush, glFlushRenderAPPLE,
> etc.), and then perform a 'bind' (in this case, glBindTexture()) on context B.  Note that
> in the case of an IOSurface backed texture used as a color buffer attachment for an FBO,
> you are only required to call glBindFramebuffer() again.  You do not have to call
> glFramebufferTexture2D().

Thus we should be able to rely on glFlush to synchronize the change. If we bind before the flush we will be drawing the previous frame until the flush has been made. SendShow will notify that a glFlush has occurred but the changes will be visible immediately after the flush (unless the window changed, in which case a new surface is allocated).

Comment 15

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v3

Forgot to release the previous mIOSurface in RecvShow.

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 533769 [details] [diff] [review]
Async Core Animation v3

Generic part seems ok for me, but I think some other mac-GFX people should take a look at this patch and prove that double buffering and sync is not needed here, and possibly this wiki rules does not apply on mac...
https://wiki.mozilla.org/Gecko:AsyncPluginPainting

who can do that? roc?

Comment 17

[Benoit Girard (:BenWa)]

This patch only provides async Core Animation. I'll post a patch next week for async Core Graphics that will follow the plug-in strategy more closely.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v4

Review comments for #ifdef, delete IOSurface changes did not get 'qrefresh' into previous patch.

Comment 19

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

Comment on attachment 533952 [details] [diff] [review]
Async Core Animation v4

Review of attachment 533952 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2975,5 @@
> +#ifdef XP_MACOSX
> +    // We can't use the thebes code with CoreAnimation so we will
> +    // take a different code path.
> +    if (mDrawingModel == NPDrawingModelCoreAnimation ||
> +          mDrawingModel == NPDrawingModelInvalidatingCoreAnimation) {

fix indent

::: dom/plugins/ipc/PluginInstanceChild.h
@@ +513,5 @@
>  
> +#ifdef XP_MACOSX
> +    // Current IOSurface available for rendering
> +    // We can't use thebes gfxASurface like other platforms.
> +    nsIOSurface          *mCurrentIOSurface; 

nsAutoPtr (and get rid of the explicit "delete mCurrentIOSurface").

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +533,5 @@
> +            return false;
> +        }
> +      
> +        if (mIOSurface) {
> +            delete mIOSurface;

PluginInstanceParent::mIOSurface should be nsAutoPtr as well

Comment 20

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

I don't really follow the explanation in comment #14. I don't think we can rely on glFlush for synchronization; that will ensure any drawing done before the glFlush shows up when we use the IOSurface after the glFlush, but the problem to worry about is what happens when the parent process uses the IOSurface to draw the previous frame while CARenderer draws the CALayer for the new frame to the IOSurface. glFlush won't prevent that, because there is no guarantee that nothing is drawn until the glFlush happens. (It would be perfectly valid for a GL implementation to draw everything synchronously and make glFlush a no-op.)

But without knowing what goes on under the covers with IOSurface and CARenderers, I don't know if there's a real synchronization hazard here or not.

On the other hand, if we never see any visual problems in practice, then the overhead of synchronization or copying is probably not worth it even if we need it in theory.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Delay Invalidate Task

Adds a 16ms (60FPS) delay to let dirty rects accumulate. This solve performance problems on OS X.

Comment 22

[Oleg Romashin (:romaxa)]

Comment on attachment 535149 [details] [diff] [review]
Delay Invalidate Task

I think this should be fine, be check it carefully on try server.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v5

Applied review comments

Comment 24

[Benoit Girard (:BenWa)]

Attachment: Async CG via Async CA v1

This patch creates a CALayer that we use to draw Core Graphics within. This let's us use the same code path as CALayer for async and allows us to use IOSurface cross process rather then bitmap shared memory. This moves some of the work from the content process to the plugin process since we don't have to convert a CGBitmapContext to a layer.

Once/If OS X gives us accelerated Core Graphics context within Core Animation we should see additional speed improvements.

Comment 25

[Benoit Girard (:BenWa)]

Attachment: Async CG via Async CA v2

Comment 26

[Benoit Girard (:BenWa)]

Investigating some test failure. Patches appear to be delaying the first paint.

Comment 27

[Benoit Girard (:BenWa)]

Will wait until bug 660721 lands to lands these fixes. Got a green try run for the Async CA/CG. Updating attachments that yield green try run.

Comment 28

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v6

Fixed bit-rot and applied review comments. Carrying forward r=roc

Comment 29

[Benoit Girard (:BenWa)]

Attachment: Async CG via Async CA v3

Fixed bit-rot, fragile import ordering, add logging for event/drawing model on os x. Fixed mochitest failure (UseAsyncRendering condition was missing 'mInstanceOwner->UseAsyncRendering()').

Comment 30

[Benjamin Smedberg]

I'm not sure why this got marked tracking firefox 6, but I'm pretty damn certain I don't want it to land on Aurora. This is the kind of thing that needs lots of bake time.

Comment 31

[JP Rosevear [:jpr]]

Agreed with Benjamin, long standing issue, landing for 7 is fine.

Comment 32

[Asa Dotzler [:asa]]

This got marked tracking? because it blocks two security sensitive bugs that were both marked tracking-firefox6+  I'm totally cool with minusing this but it means re-visiting bug 552002 and bug 591409 and presumably minusing those as well. (My nomination here was about housekeeping, not advocacy)

Comment 33

[Benoit Girard (:BenWa)]

Attachment: Async Core Animation v7

Fix bitrot caused by nsObjectFrame refactor.

Comment 34

[Benoit Girard (:BenWa)]

Attachment: Async CG via Async CA v4

Fix bitrot. This patch passed tryserver before the nsObjectFrame refactor. Feel free to pass the review along if you can't get to it.

Comment 35

[Josh Aas]

Comment on attachment 536660 [details] [diff] [review]
Async CG via Async CA v4

Review of attachment 536660 [details] [diff] [review]:
-----------------------------------------------------------------

I would combine the CA and CG patches. It's more confusing not to do that since the CA patch is first but the CG patch contains a critical fix for CA.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2317,5 @@
>          mCurrentAsyncSetWindowTask = nsnull;
>      }
>  
>      mWindow.window = NULL;
> +#ifdef OS_MACOSX

We should just use XP_MACOSX going forward, drop the Chromium macros. No need to clean anything else up, just don't add them.

::: dom/plugins/ipc/PluginUtilsOSX.h
@@ +50,5 @@
>  NPError ShowCocoaContextMenu(void* aMenu, int aX, int aY, void* pluginModule, RemoteProcessEvents remoteEvent);
>  
>  void InvokeNativeEventLoop();
>  
> +// Need to call back into the browser's to process event.

This comment doesn't really make any sense. The browser's what?

Comment 36

[Benoit Girard (:BenWa)]

http://hg.mozilla.org/mozilla-central/rev/24e35780bd8f

Will fill a spin off bug for throttling.

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 536660 [details] [diff] [review]
Async CG via Async CA v4

>--- a/dom/plugins/base/nsPluginInstanceOwner.cpp
>+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
>@@ -258,18 +258,19 @@ nsPluginInstanceOwner::UseAsyncRendering
>   nsRefPtr<ImageContainer> container = mObjectFrame->GetImageContainer();
> #endif
> 
>   PRBool useAsyncRendering;
>   return (mInstance &&
>           NS_SUCCEEDED(mInstance->UseAsyncPainting(&useAsyncRendering)) &&
>           useAsyncRendering &&
> #ifdef XP_MACOSX
>-          IsRemoteDrawingCoreAnimation() &&
>-          container && container->GetBackendType() == LayerManager::LAYERS_OPENGL
>+          mObjectFrame && mObjectFrame->GetImageContainer().get() &&
>+          mObjectFrame->GetImageContainer().get()->GetBackendType() == 
>+                  LayerManager::LAYERS_OPENGL

This leaks.

Comment 38

[Benoit Girard (:BenWa)]

You're right. This isn't picked up since this code isn't being hit because of bug 663259. I'm working on a patch that will fix these issue as well as some test failures.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This bug is pretty big, patches have landed. I'd vote for a new followup bug rather than reopening this one.

Comment 40

[Benoit Girard (:BenWa)]

Agreed, going to do follow up work on bug 663259