497225 - implement Core Animation NPAPI drawing model for Mac OS X

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We should implement the Core Animation drawing model for Mac OS X.

WebKit has this working and it has quite a few benefits. A particularly huge benefit is that it will allow for accelerated drawing by out of process plugins.

Spec coming soon. This will require the Cocoa NPAPI event model.

Comment 1

[Josh Aas]

Spec is here:

https://wiki.mozilla.org/Plugins:CoreAnimationDrawingModel

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Adds CoreAnimation NPAPI support

I have a few comments I would like to note about this patch:
- The only #define for LEOPARD we have are within widget/src/cocoa. I think #include-ing those headers from within layout/generic/ source files is excessive. Although repeating the define is bad style.
- Because we are using an NSView instead of CGGraphics, there is the following side effects that occur when a plug-in uses the drawing model:
-- Overlaying elements behind/over the plug-in does not render correctly. The plug-in renders above everything else it covers. Alpha transparency behind the CALayer does not blend with CGGraphics content.
-- When switching tabs the plug-in renders one frame before. The repaint does not happen at the same time. This only shows up during tab switching.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Compiled plugin that can be used to test the patch

Includes a compiled OS X plug-in using the CoreAnimation drawing model, a test html to use and a reference png to compare the results.

Comment 4

[Josh Aas]

Comment on attachment 421900 [details] [diff] [review]
Adds CoreAnimation NPAPI support

>+    if (mInstanceOwner->GetDrawingModel() == NPDrawingModelCoreAnimations) {
>+      // We are using the CoreAnimations drawing model, we need to attach
>+      // the CoreAnimation layer provided by the plugin to the NSView.
>+      void* caLayer = mInstanceOwner->GetCALayer();
>+      if (caLayer) {
>+        // Use the mac util function to set the layer here.
>+        NS_NPAPI_SetCALayer( mWidget->GetNativeData(NS_NATIVE_WIDGET), caLayer );

All of the comments here are unnecessary, the code is clear enough.

>+#ifdef XP_MACOSX
>+  if( mWidget ) {

"if (mWidget) {" <- style nit

>+    if (mInstanceOwner->GetDrawingModel() == NPDrawingModelCoreAnimations) {
>+      // We are using the CoreAnimations drawing model, we need to release
>+      // the CoreAnimation layer provided by the plugin to the NSView.
>+      NS_NPAPI_ReleaseCALayer( mWidget->GetNativeData(NS_NATIVE_WIDGET) );

I think "ReleaseCALayer" is misleading, sounds like a simple memory mgmt function. How about we just have "NS_NPAPI_SetCALayer" accept a NULL layer and it can perform the actions that NS_NPAPI_ReleaseCALayer currently does.

>+void* nsPluginInstanceOwner::GetCALayer()
>+{
>+#ifdef NS_LEOPARD_AND_LATER
>+  void* caLayer;
>+  mInstance->GetValueFromPlugin(NPPVpluginCoreAnimationLayer, 
>+                               &caLayer);
>+  return caLayer;
>+#endif
>+  return nsnull;
>+}

Lets just skip all of the 10.5 checks for now. We can add that back if we ever re-enable 10.4 support, which we probably won't.

>+#if defined(MAC_OS_X_VERSION_10_5) && (MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5)
>+#define NS_LEOPARD_AND_LATER 1
>+#endif

Kill this off for now.

>+// Util function to attach a CoreAnimation layer to a NSView called from C++ code.
>+NPError NS_NPAPI_SetCALayer(void* inView, void* layer);
>+// Util function to release the backing CoreAnimation layer of a given NSView view from C++ code.
>+NPError NS_NPAPI_ReleaseCALayer(void* inView);

Like I said before, lets just have SetCALayer accept a NULL layer instead of having two functions.

>+NPError NS_NPAPI_ReleaseCALayer(void* inView)
>+{
>+  NSView* cocoaView = (NSView*)inView;
>+  CALayer* caLayer = [cocoaView layer];
>+  [cocoaView setLayer:nil];
>+  [cocoaView setWantsLayer:NO];
>+  // Use release since this is a retained layer
>+  [caLayer release];

I think the plugin should handle the release. I have requested a spec clarification here, proposing that the browser not handle release.

Great work, thanks Ben!

Comment 5

[Josh Aas]

We modified the spec to say explicitly that the browser is not responsible for releasing the CALayer. The plugin will do it during or after NPP_Destroy for the instance.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Applied review comments.

Here is the revised patch.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Applied review comments.

Forgot to remove some extra comments in the previous patch. This is done now.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Applied review comments.

Comment 9

[Josh Aas]

Attachment: updated to current trunk

I landed something that made this patch not apply. This is just an un-bitrotted patch.

Comment 10

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

+  , NPNVsupportsCoreAnimationsBool = 2003

Shouldn't this be NPNVsupportsCoreAnimationBool (no 's')?

It seems to me that this won't work with Web content placed over the plugin. Is that correct?

Comment 11

[Josh Aas]

(In reply to comment #10)
> +  , NPNVsupportsCoreAnimationsBool = 2003
> 
> Shouldn't this be NPNVsupportsCoreAnimationBool (no 's')?

Yes

> It seems to me that this won't work with Web content placed over the plugin. Is
> that correct?

There are some known problems, see comment #2 from Benoit.

Comment 12

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

So what's the plan? It seems to me we should be rendering CoreAnimation plugins in a different way that doesn't just set the layer on the view.

Comment 13

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

In particular, shouldn't we be rendering the plugin using renderInContext or something like that?

Comment 14

[Benoit Girard (:BenWa)]

This is the way I tried to implement it at first. From what I found you need a OpenGL context for the CoreAnimation layer to render correctly. This is not compatible with the CoreGraphics context we are currently using. 

I discussed this with Jeff and apparently in the future the rendering will all be done using an OpenGL context at which point we can have CoreAnimation 'renderInContext' in its proper place. The CARenderer class should do the trick: 
http://developer.apple.com/mac/library/DOCUMENTATION/GraphicsImaging/Reference/CARenderer_class/Introduction/Introduction.html

Comment 15

[Benjamin Smedberg]

For OOPP we will need to use CALayer to remote the drawing across processes: does this mean that we're going to need fundamental changes to our drawing surfaces to accomplish this? How far away is the future (and what bug covers it)?

Comment 16

[Benoit Girard (:BenWa)]

Jeff was referring to Bug 534425.

I'm still not comfortable saying that we can't render Core Animation within 'renderInContext' without having solid evidence. I will give it another shot and see if I can make progress on this.

Comment 17

[Josh Aas]

I'd still like to take this patch as-is for now. We can file other bugs on fixing the known issues.

Comment 18

[Benoit Girard (:BenWa)]

Attachment: Removed extra 's'

Comment 19

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

The problem is that fixing the known issues basically means using an entirely different approach. What is the value of landing this patch now?

Comment 20

[Josh Aas]

Seems like the current level of support is better than nothing but if there is any chance of significantly better support any time soon then I'm fine with holding off for now.

(In reply to comment #14)
> I discussed this with Jeff and apparently in the future the rendering will all
> be done using an OpenGL context at which point we can have CoreAnimation
> 'renderInContext' in its proper place.

This sounds like something that won't happen soon.

Comment 21

[Benjamin Smedberg]

Is the known problem that we won't render transparent or clipped plugins correctly? Is this similar or worse than windowed-mode plugins on Windows/Linux?

Comment 22

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

It will basically have the same effects as windowed-mode plugins on Windows/Linux. Currently I suspect the patch will not clip the plugin properly, you'll need to change SetWindowClipRegion to somehow clip the layer.

Note, Mac currently has no notion of windowless plugins, so I think with this patch, if Flash supported CA rendering then everyone using wmode="transparent" is going to regress. That's not shippable.

One way forward might be to not enable CA rendering for plugins which want windowless mode. I don't know if we can do that ... I don't know if Flash ever requests windowless mode on Mac.

CARenderer can render the layer into an OpenGL context. Presumably there are slow ways to read back from the OpenGL context and render into a Quartz context. Are there fast ways?

Comment 23

[Benoit Girard (:BenWa)]

> I'm still not comfortable saying that we can't render Core Animation within
> 'renderInContext' without having solid evidence. I will give it another shot
> and see if I can make progress on this.

I've managed to get 'drawInContext' (as opposed 'renderInContext') to produce some visual output, however everything is being rendered about the same origin giving incorrect results. This fixes the clipping/transparency problems outlined earlier since we are drawing using Core Graphics. The documentation states the following for renderInContext but my guess is that the same will apply to drawInContext:

Important: The Mac OS X v10.5 implementation of this method does not support the entire Core Animation composition model. QCCompositionLayer, CAOpenGLLayer, and QTMovieLayer layers are not rendered. Additionally, layers that use 3D transforms are not rendered, nor are layers that specify backgroundFilters, filters, compositingFilter, or a mask values. Future versions of Mac OS X may add support for rendering these layers and properties.

I'm also curious if using 'renderInContext/drawInContext' will result in accelerated drawing since it is using a CoreGraphics context.

Comment 24

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

(In reply to comment #23)
> Important: The Mac OS X v10.5 implementation of this method does not support
> the entire Core Animation composition model. QCCompositionLayer, CAOpenGLLayer,
> and QTMovieLayer layers are not rendered. Additionally, layers that use 3D
> transforms are not rendered, nor are layers that specify backgroundFilters,
> filters, compositingFilter, or a mask values. Future versions of Mac OS X may
> add support for rendering these layers and properties.

I think getting the data into a temporary GL context and somehow getting the data from there into Quartz is probably the most foolproof approach.

Comment 25

[Benoit Girard (:BenWa)]

Attachment: WebKit Core Animation Print to PDF

It might be worth noting that the current patch is nearly identical to the current WebKit implementation of Core Animation (i.e. attach the CALayer to the NSView). I created a test case and could not get web content to appear above the Core Animation layer similar to our implementation. The attachment is an example of what happens when you print to PDF using WebKit. The layer is not rendered. The implementations seem to suffer from the same problems with some slight differences.

I'm currently working on the CARenderer method at the moment. I've got a custom OpenGL context rendering working. I just need to debug why the CARenderer isn't trying to render the layer.

Comment 26

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

That sounds nasty!

Comment 27

[Benoit Girard (:BenWa)]

I've got the 'renderInContext' working with the correct transparency behavior working. Given the layer support limitation mentioned above are we interested in using that method?

Otherwise I've also made progress using CARenderer on a temporary openGL context. There are still a color conversion problems I'm still working out.

Comment 28

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

(In reply to comment #27)
> I've got the 'renderInContext' working with the correct transparency behavior
> working. Given the layer support limitation mentioned above are we interested
> in using that method?

Does it work with Flash? Especially for video playback.

Comment 29

[Benoit Girard (:BenWa)]

I've tested flash and quicktime with the 3 implementations we have so far:

-Using NSView both flash and quicktime work.
-Using renderInContext flash crashes and only the playback bar shows up for Quicktime. I'm assuming that is expected since the video is rendered using a QTMovieLayer and that is not supported with renderInContext.
-Using CARenderer flash crashes and the quicktime video does not show up. I expect it should be possible to get the video since NSView/CARenderer both call the same code to render.

I can't find for sure what is making flash crash but right now the pattern is when the CALayer is not attached to an NSView.

Comment 30

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

Can we attach the CALayer to a hidden NSView and then use CARenderer?

Comment 31

[Ashley Bischoff (blog at handcoding.com)]

If it may be of interest, this post from one of Adobe's engineers goes over their work toward adding support for the Core Animation drawing model in Flash 10.1:
http://www.kaourantin.net/2010/02/core-animation.html

(The page also includes some preliminary before/after benchmarks.)

Comment 32

[Benoit Girard (:BenWa)]

Attachment: CARenderer implementation

Here is what I have so far for the Core Animation implementation. I'm flagging this as feedback until we get the performance problem figured out.

I can't get nextFrameTime to give me meaningful results so right now I refresh continuously. This spikes CPU usage and does not give us any A/V sync for interactive plug-ins (Flash). Other then that we are bottle-necking on glReadPixels so presumably this will get significantly faster once 'Layers' is working.

Youtube video run reasonable well at 480p. 1080p video get low FPS while buffering (similarly to the current safari nightly), and run somewhat smoothly once buffered. Flash games I have tried run with a low FPS count but are playable.

And of course transparency/compositing are working.

Comment 33

[Benoit Girard (:BenWa)]

Attachment: 50FPS Implementation

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 433637 [details] [diff] [review]
50FPS Implementation

This patch uses a single static 50FPS timer.

I have noticed a bit of performance problem with this core animation rendering on certain flash beta instances such as 1080p video and flash game that use certain effects. I think this patch is ready to go regardless and those issues should be opened as separate bugs so we can resolve them later.

The Shark samples I have shows that 90%+ CPU was spend within flash so I will try and confirm this on a separate machine. If anyone reproduces the slowdown please send me your shark samples.

Comment 35

[Benoit Girard (:BenWa)]

Attachment: 50FPS Implementation2

I had an issue with the previous patch with QuartzCore not being linked with the tryserver. I think I just was just linking at the wrong place. I also fixed the warnings when the object frame had 0 width/height.

I am still getting leak from the tryserver. I don't understand how come the static nsTArray does not get deconstructed on shutdown. I get the following output:
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsTArray_base with size 4 bytes


I will post a revision to the patch to address that problem. Since it will be a minor fix perhaps we can get the rest reviewed meanwhile.

Comment 36

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

Don't rely on destructors for static objects being run. You might be able to put your shutdown code in nsLayoutStatics::Shutdown.

Comment 37

[Benjamin Smedberg]

FWIW, the destructor is being run, but it runs after we've collected all the leak stats, which occurs at the last NS_LogTerm. As a policy, you should not have static C++ objects with destructors, which includes nsTArray, nsString, etc.

Comment 38

[Benoit Girard (:BenWa)]

I could instead use a nsTArray* and new/delete it when I insert/delete the first/last element. I don't like how that needlessly add another allocation step that could fail (even if it's unlikely).

I'm trying to maintain a list of plug-ins that needs to redraw using the core animation redraw timer instead of having one timer per plug-in.

Comment 39

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

Slightly ugly, but probably the right thing to do.

Comment 40

[Benoit Girard (:BenWa)]

Attachment: CARenderer memfix

This patch addresses the memory leakage. I ran reftest to confirm it.

Comment 41

[Josh Aas]

Comment on attachment 434144 [details] [diff] [review]
CARenderer memfix

+nsCOMPtr<nsITimer>                *nsPluginInstanceOwner::CATimer = NULL;
+nsTArray<nsPluginInstanceOwner*>  *nsPluginInstanceOwner::CARefreshListeners = NULL;

Please prefix static variables like this with "s", as in "sCATimer".

+void nsPluginInstanceOwner::AddToCARefreshTimer(nsPluginInstanceOwner *aPluginInstance) {
+  if (CARefreshListeners == NULL ) {
+    CARefreshListeners = new nsTArray<nsPluginInstanceOwner*>();
+    if (CARefreshListeners == NULL) return;
+  }
+  NS_ASSERTION(!CARefreshListeners->Contains(aPluginInstance), "pluginInstanceOwner already registered as a listener");
+  CARefreshListeners->AppendElement(aPluginInstance);
+  if (CARefreshListeners->Length() == 1 || !CATimer) {
+    CATimer = new nsCOMPtr<nsITimer>();
+    if (CATimer == NULL) {
+      CARefreshListeners->RemoveElement(aPluginInstance);
+      return;
+    }
+    *CATimer = do_CreateInstance("@mozilla.org/timer;1");
+    (*CATimer)->InitWithFuncCallback(CARefresh, NULL, DEFAULT_REFRESH_RATE, nsITimer::TYPE_REPEATING_SLACK);
+  }
+}

Just do this in 3 discrete stages. Create CARefreshListeners if necessary, then create CATimer if necessary, then handle inserting the instance and starting the timer.

+  if (CGLCreatePBuffer(width, height, GL_TEXTURE_RECTANGLE_EXT, GL_RGBA, 0, &mPixelBuffer) != kCGLNoError) {

Calls to OS functions like CGLCreatePBuffer should be prefixed with "::".

+  if (colorSpace) CGColorSpaceRelease(colorSpace);

I generally dislike any form of "if" block contents on the same line as the "if" test. Saves space, sure, but harder to recognize visually.

+  //*nextFrameTime = [caRenderer nextFrameTime] - caTime;

Did you mean to remove this line?

Comment 42

[Benoit Girard (:BenWa)]

Attachment: CARenderer reviewed

Thanks for the suggestions.

I also made the modified SetupCARenderer to safeguard against GetValueFromPlugin not setting caLayer:
+void nsPluginInstanceOwner::SetupCARenderer(int w, int h)
+{
+  void *caLayer = NULL;
+  mInstance->GetValueFromPlugin(NPPVpluginCoreAnimationLayer, &caLayer);
+  if (!caLayer) {
+    return;
+  }
+  mCARenderer.SetupRenderer(caLayer, w, h);
+  AddToCARefreshTimer(this);
+}

Comment 43

[Josh Aas]

Comment on attachment 434301 [details] [diff] [review]
CARenderer reviewed

+void nsPluginInstanceOwner::AddToCARefreshTimer(nsPluginInstanceOwner *aPluginInstance) {
+  if (sCARefreshListeners == NULL ) {
+    sCARefreshListeners = new nsTArray<nsPluginInstanceOwner*>();
+    if (sCARefreshListeners == NULL) {
+      return;
+    }
+  }
+
+  NS_ASSERTION(!sCARefreshListeners->Contains(aPluginInstance), "pluginInstanceOwner already registered as a listener");
+  sCARefreshListeners->AppendElement(aPluginInstance);
+
+  if (!sCATimer) {
+    sCATimer = new nsCOMPtr<nsITimer>();
+    if (sCATimer == NULL) {
+      return;
+    }
+  }

No need to make another patch to do this, but in the future no need to use "== NULL" or "!= NULL" - just consistently use "foo" or "!foo" like you did in one of the tests here. Also there is an extra space between "NULL" and ")" here.

Comment 44

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

+  void RenderCoreAnimation(CGContextRef cgContext, int w, int h);

aCGContext, aWidth, aHeight

+  void SetupCARenderer(int w, int h);

aWidth, aHeight

+void CARendererCallback(nsITimer *aTimer, void *aClosure) {

static

+    if (GetDrawingModel() == NPDrawingModelCoreGraphics || GetDrawingModel() == NPDrawingModelCoreAnimation)
+  else if (drawingModel == NPDrawingModelCoreGraphics || drawingModel == NPDrawingModelCoreAnimation)
+  if (::CGLCreatePBuffer(width, height, GL_TEXTURE_RECTANGLE_EXT, GL_RGBA, 0, &mPixelBuffer) != kCGLNoError) {

80 column warning

+NPError nsCARenderer::Render(CGContextRef cgContext, int width, int height) {

Why does this return NPError instead of nsresult?

Comment 45

[Benoit Girard (:BenWa)]

Attachment: CARenderer superreview

I used NPError because that was what is currently used in nsPluginUtilsOSX.mm, but I have made the switch to nsresult.

Comment 46

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/e687f97bbb6e

Comment 47

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/comm-central/rev/3147aad42dfe
Trunk bustage fix for Mac builds following bug 497225 landing on mozilla-central

Comment 48

[Stephen Augenstein]

If we have visual bugs with the new core-animation builds and flash player 10.1b3, should we create a new bug or comment here? In addition, is there an easy way to see if it's the fault of the new Firefox code or the Flash Player plugin.

Comment 49

[Benoit Girard (:BenWa)]

It wouldn't hurt to open a bug to track that issue.

There is no easy way to track on which side the issue is at the moment. I am in the process of investigating it.