555281 - Implement Core Animation NPAPI drawing model for OOPP

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Benoit Girard (:BenWa)]

I've started working on this. I have it working without the use of IOSurface. For now we plan on waiting for a patch that uses IOSurface before we land it.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Core Animation IOSurface

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 438121 [details] [diff] [review]
Core Animation IOSurface

This patch implement Core Animation for OOPP.

I moved the nsCARenderer code from layout/generic/nsPluginUtilsOSX.mm to modules/plugins/base/src/nsCoreAnimationSupport.mm since the code is getting bigger.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Core Animation IOSurface

Fixed a typo in non OSX code. Fixed code that was wider than 80 columns.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 439058 [details] [diff] [review]
Core Animation IOSurface

In general, s/NULL/nsnull/g

>diff --git a/dom/plugins/PPluginInstance.ipdl b/dom/plugins/PPluginInstance.ipdl
>--- a/dom/plugins/PPluginInstance.ipdl
>+++ b/dom/plugins/PPluginInstance.ipdl
>@@ -83,6 +83,9 @@
>   // special cases where we need to a shared memory buffer
>   rpc NPP_HandleEvent_Shmem(NPRemoteEvent event, Shmem buffer)
>     returns (int16_t handled, Shmem rtnbuffer);
>+  // special cases where we need an iosurface
>+  rpc NPP_HandleEvent_IOSurface(NPRemoteEvent event, uint32_t surfaceid)
>+    returns (int16_t handled);
>   // special cases of HandleEvent to make mediating races simpler
>   rpc Paint(NPRemoteEvent event)
>     returns (int16_t handled);
>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
>--- a/dom/plugins/PluginInstanceChild.cpp
>+++ b/dom/plugins/PluginInstanceChild.cpp
>@@ -80,7 +80,7 @@
> #define NS_OOPP_DOUBLEPASS_MSGID TEXT("MozDoublePassMsg")
> #elif defined(XP_MACOSX)
> #include <ApplicationServices/ApplicationServices.h>
>-#include "nsPluginUtilsOSX.h"
>+#define DEFAULT_REFRESH_RATE 20 // CoreAnimation: 50 FPS

This should be renamed to DEFAULT_REFRESH_TIMEOUT_MS or something - rate, to me, means Hz.

>@@ -632,7 +663,7 @@
>         PLUGIN_LOG_DEBUG(("Invalid event type for AnswerNNP_HandleEvent_Shmem."));
>         *handled = false;
>         *rtnmem = mem;
>-        return true;
>+        return handled;

You're returning handled like this a lot. It is my memory that returning false from a handler means that the child process gets killed. Is this a) still the case; b) what you want?

The GL code all looks totally correct. I would, however, like to see the GL error status checked from time to time with glGetError(), so we don't try to run if the driver's buggy or doesn't like something we've given it.

In general, I cannot r+ this hard enough.

Comment 6

[Benoit Girard (:BenWa)]

We will need to fix bug 555281 before we can land this otherwise when you switch tabs with this patch the plug-in remains white until the the whole plug-in is invalidated (resizing/scrolling window).

Comment 7

[Josh Aas]

(In reply to comment #6)
> We will need to fix bug 555281 before we can land this otherwise when you
> switch tabs with this patch the plug-in remains white until the the whole
> plug-in is invalidated (resizing/scrolling window).

I don't want to block landing on bugs like this. This is limited to trunk Flash 10.1 users, the benefits of having widespread testing (including by Adobe) outweigh bugs like that.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Core Animation IOSurface (Review Comments Applied)

I fixed the review comments. Checking the glError showed that I shouldn't be calling 'glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);' so I removed that call.

I used the windows quirk mechanism to disable the timer under OSX like I had discussed with Josh.

Comment 9

[Josh Aas]

Comment on attachment 439169 [details] [diff] [review]
Core Animation IOSurface (Review Comments Applied)

+static PRInt32 OSXVersion()

You don't need the OS version check, it is redundant. We already don't allow plugins to run out-of-process on 10.5.

+    // Significant speed up by reseting the scaling

"resetting"

+    ::CGContextSetInterpolationQuality(aCGContext, kCGInterpolationNone );

Extra space before ")".

+  CFNumberRef cfWidth = CFNumberCreate(NULL, kCFNumberSInt32Type, &aWidth);
+  CFNumberRef cfHeight = CFNumberCreate(NULL, kCFNumberSInt32Type, &aHeight);
+  CFNumberRef cfBytesPerElem = CFNumberCreate(NULL, kCFNumberSInt32Type, &bytesPerElem);
+  ::CFDictionaryAddValue(props, nsIOSurfaceLib::kPropWidth,
+                                cfWidth);
+  ::CFDictionaryAddValue(props, nsIOSurfaceLib::kPropHeight,
+                                cfHeight);
+  ::CFDictionaryAddValue(props, nsIOSurfaceLib::kPropBytesPerElem, 
+                                cfBytesPerElem);
+  ::CFDictionaryAddValue(props, nsIOSurfaceLib::kPropIsGlobal, 
+                                kCFBooleanTrue);
+
+  IOSurfacePtr surfaceRef = nsIOSurfaceLib::IOSurfaceCreate(props);
+  ::CFRelease(cfWidth);
+  ::CFRelease(cfHeight);
+  ::CFRelease(cfBytesPerElem);
+  ::CFRelease(props);

Once you add a CF object to a dictionary the dictionary retains it. You can release right after adding the CFNumbers, then just release the dictionary after nsIOSurfaceLib::IOSurfaceCreate.

Also, prefix CFNumberCreate usage with "::".

+  return ioSurface;
+
+}

Extra newline at the bottom of the method.

Comment 10

[Josh Aas]

Comment on attachment 439169 [details] [diff] [review]
Core Animation IOSurface (Review Comments Applied)

Also, it looks like each instance has its own timer. That means with 4 instances on a page we'll have 200 timer interrupts per second. Lets have one timer that starts when the instance count is > 0 and stops when the instance count gets back to 0.

We have a shared timer setup for the Carbon event model, in-process. You can look at that code.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Core Animation IOSurface (Review Comments Applied)

Moved the timer from PluginInstanceChild to PluginInstanceModule. This way we only have one plug-in timer and they can invalidate without sending IPC messages.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Changes from previous patch

This patch show the changes following josh' comment to make the review easier.

Comment 13

[Josh Aas]

Comment on attachment 439311 [details] [diff] [review]
Core Animation IOSurface (Review Comments Applied)

+    // Significant speed up by reseting the scaling

You didn't fix the spelling error here.

Otherwise looks good!

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 439311 [details] [diff] [review]
Core Animation IOSurface (Review Comments Applied)

>diff --git a/dom/plugins/PPluginInstance.ipdl b/dom/plugins/PPluginInstance.ipdl
>--- a/dom/plugins/PPluginInstance.ipdl
>+++ b/dom/plugins/PPluginInstance.ipdl
>@@ -83,6 +83,9 @@
>   // special cases where we need to a shared memory buffer
>   rpc NPP_HandleEvent_Shmem(NPRemoteEvent event, Shmem buffer)
>     returns (int16_t handled, Shmem rtnbuffer);
>+  // special cases where we need an iosurface
>+  rpc NPP_HandleEvent_IOSurface(NPRemoteEvent event, uint32_t surfaceid)
>+    returns (int16_t handled);

Both HandleEvent_Shmem and HandleEvent_IOSurface are paint events and
so need to win RPC races.  Please add them to
PluginMessageUtils.cpp:MediateRPCRace().

>   // special cases of HandleEvent to make mediating races simpler
>   rpc Paint(NPRemoteEvent event)
>     returns (int16_t handled);
>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
>--- a/dom/plugins/PluginInstanceChild.cpp
>+++ b/dom/plugins/PluginInstanceChild.cpp
>+#ifdef XP_MACOSX
>+bool
>+PluginInstanceChild::AnswerNPP_HandleEvent_IOSurface(const NPRemoteEvent& event,
>+                                                 const uint32_t &surfaceid,
>+                                                 int16_t* handled)

Nit: the indentation is messed up above.

>+{
>+    PLUGIN_LOG_DEBUG_FUNCTION;
>+    AssertPluginThread();
>+
>+    NPCocoaEvent evcopy = event.event;
>+    nsIOSurface* surf = nsIOSurface::LookupSurface(surfaceid);
>+    if (!surf) {
>+        NS_ERROR("Invalid IOSurface.\n");
>+        *handled = false;
>+        return true;
>+    }
>+

Is there any understood reason why this lookup could fail?  If not,
you should |return false| here, that's a terminal error.

>+    if (evcopy.type == NPCocoaEventDrawRect) {
>+        mCARenderer.AttachIOSurface(surf);
>+        if (!mCARenderer.isInit()) {
>+            void *caLayer = nsnull;
>+            NPError result = mPluginIface->getvalue(GetNPP(), 
>+                                     NPPVpluginCoreAnimationLayer,
>+                                     &caLayer);
>+            if (result != NPERR_NO_ERROR || !caLayer) {
>+                PLUGIN_LOG_DEBUG(("Plugin requested CoreAnimation but did not "
>+                                  "provide CALayer."));
>+                *handled = false;
>+                return true;
>+            }

I think this is also a terminal error.

>+            mCARenderer.SetupRenderer(caLayer, mWindow.width, mWindow.height);
>+            // Flash needs to have the window set again after this step
>+            if (mPluginIface->setwindow)
>+                (void) mPluginIface->setwindow(&mData, &mWindow);
>+        }
>+    } else {
>+        PLUGIN_LOG_DEBUG(("Invalid event type for "
>+                          "AnswerNNP_HandleEvent_IOSurface."));
>+        *handled = false;
>+        return true;
>+    } 
>+

This is also a terminal error.

>diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp
>--- a/dom/plugins/PluginInstanceParent.cpp
>+++ b/dom/plugins/PluginInstanceParent.cpp
>@@ -61,13 +61,13 @@
> #include <gdk/gdk.h>
> #elif defined(XP_MACOSX)
> #include <ApplicationServices/ApplicationServices.h>
>-#include "nsPluginUtilsOSX.h"
> #endif // defined(XP_MACOSX)
> 
> using namespace mozilla::plugins;
> 
> PluginInstanceParent::PluginInstanceParent(PluginModuleParent* parent,
>                                            NPP npp,
>+                                           const nsCString& aMimeType,
>                                            const NPNetscapeFuncs* npniface)
>   : mParent(parent)
>     , mNPP(npp)
>@@ -78,12 +78,29 @@
>     , mPluginWndProc(NULL)
>     , mNestedEventState(false)
> #endif // defined(XP_WIN)
>+    , mQuirks(0)
> #if defined(XP_MACOSX)
>     , mShWidth(0)
>     , mShHeight(0)
>-    , mShColorSpace(NULL)
>+    , mShColorSpace(nsnull)
>+    , mDrawingModel(NPDrawingModelCoreGraphics)
>+    , mIOSurface(nsnull)
> #endif
> {
>+    InitQuirksModes(aMimeType);
>+}
>+

You should set quirks in PluginModuleParent and read them in
PluginInstanceParent.  Also, it seems to me that the refresh timer
should be the quirk, since it sucks ass.  But that's just a semantic
issue.

>+            // XXX: benwa: OMG MEMORY LEAK! 
>+            //      There is currently no way dealloc the shmem
>+            //      so for now we will leak the memory and will fix this ASAP!

(BTW, I fixed this yesterday.)

>@@ -679,48 +722,82 @@
> 
> #ifdef XP_MACOSX
>     if (npevent->type == NPCocoaEventDrawRect) {
>-        if (mShWidth == 0 && mShHeight == 0) {
>-            PLUGIN_LOG_DEBUG(("NPCocoaEventDrawRect on window of size 0."));
>-            return false;
>-        }
>-        if (!mShSurface.IsReadable()) {
>-            PLUGIN_LOG_DEBUG(("Shmem is not readable."));
>-            return false;
>-        }
>+        if (mDrawingModel == NPDrawingModelCoreAnimation) {
>+            if (!mIOSurface) {
>+                NS_ERROR("No IOSurface allocated.");
>+                return true;

This is a fatal error, return false.

>+            }
>+            if (!CallNPP_HandleEvent_IOSurface(npremoteevent, 
>+                                               mIOSurface->GetIOSurfaceID(), 
>+                                               &handled)) 
>+                return false; // no good way to handle errors here...
> 
>-        if (!CallNPP_HandleEvent_Shmem(npremoteevent, mShSurface, &handled, &mShSurface)) 
>-            return false; // no good way to handle errors here...
>+            CGContextRef cgContext = npevent->data.draw.context;
>+            if (!mShColorSpace) {
>+                mShColorSpace = CreateSystemColorSpace();
>+            }
>+            if (!mShColorSpace) {
>+                PLUGIN_LOG_DEBUG(("Could not allocate ColorSpace."));
>+                return true;
>+            } 

Fatal.

>+            nsCARenderer::DrawSurfaceToCGContext(cgContext, mIOSurface, 
>+                                                 mShColorSpace,
>+                                                 npevent->data.draw.x,
>+                                                 npevent->data.draw.y,
>+                                                 npevent->data.draw.width,
>+                                                 npevent->data.draw.height);
>+            return true;
>+        } else {
>+            if (mShWidth == 0 && mShHeight == 0) {
>+                PLUGIN_LOG_DEBUG(("NPCocoaEventDrawRect on window of size 0."));
>+                return true;
>+            }
>+            if (!mShSurface.IsReadable()) {
>+                PLUGIN_LOG_DEBUG(("Shmem is not readable."));
>+                return true;
>+            }

Fatal.

>+            if (!CallNPP_HandleEvent_Shmem(npremoteevent, mShSurface, 
>+                                           &handled, &mShSurface)) 
>+                return true; // no good way to handle errors here...
> 
>+            if (!mShSurface.IsReadable()) {
>+                PLUGIN_LOG_DEBUG(("Shmem not returned. Either the plugin crashed "
>+                                  "or we have a bug."));
>+                return true;
>+            }

Fatal.

>+            char* shContextByte = mShSurface.get<char>();
>+
>+            if (!mShColorSpace) {
>+                mShColorSpace = CreateSystemColorSpace();
>+            }
>+            if (!mShColorSpace) {
>+                PLUGIN_LOG_DEBUG(("Could not allocate ColorSpace."));
>+                return true;
>+            } 

Fatal.

>+            CGContextRef shContext = ::CGBitmapContextCreate(shContextByte, 
>+                                    mShWidth, mShHeight, 8, 
>+                                    mShWidth*4, mShColorSpace, 
>+                                    kCGImageAlphaPremultipliedFirst | 
>+                                    kCGBitmapByteOrder32Host);
>+            if (!shContext) {
>+                PLUGIN_LOG_DEBUG(("Could not allocate CGBitmapContext."));
>+                return true;
>+            }
>+

Fatal.


>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp

I didn't review from here on.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #14)
> You should set quirks in PluginModuleParent and read them in
> PluginInstanceParent.  Also, it seems to me that the refresh timer
> should be the quirk, since it sucks ass.  But that's just a semantic
> issue.
> 

Also, my understanding is that the refresh timer should be expected to be gone in, say, a year, right?  Then, it will be the quirk, and you won't need to change this code.

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Core Animation IOSurface (Review Comments Applied)

More good review changes applied.

I've also reworked the following logic since IOSurface are width/height sensitive where CGContext backed by a Shmem is not:

-    if (mShWidth * mShHeight != window.width * window.height) {
-        // XXX: benwa: OMG MEMORY LEAK! 
-        //      There is currently no way dealloc the shmem
-        //      so for now we will leak the memory and will fix this ASAP!
-        if (!AllocShmem(window.width * window.height * 4, &mShSurface)) {
-            PLUGIN_LOG_DEBUG(("Shared memory could not be allocated."));
-            return NPERR_GENERIC_ERROR;
+    if (mShWidth != window.width || mShHeight != window.height) {
+        if (mDrawingModel == NPDrawingModelCoreAnimation) {
+            if (mIOSurface) {
+                delete mIOSurface;
+            }
+            mIOSurface = nsIOSurface::CreateIOSurface(window.width, window.height);
+        } else if (mShWidth * mShHeight != window.width * window.height) {
+            // Uncomment me when DeallocShmem lands.
+            //if (mShWidth != 0 && mShHeight != 0) {
+            //    DeallocShmem(&mShSurface);
+            //}
+            if (window.width != 0 && window.height != 0) {
+                if (!AllocShmem(window.width * window.height*4, 
+                                &mShSurface)) {
+                    PLUGIN_LOG_DEBUG(("Shared memory could not be allocated."));
+                    return NPERR_GENERIC_ERROR;
+                } 
+            }

Comment 17

[Benoit Girard (:BenWa)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/44ee6030f4b2

Comment 18

[Benoit Girard (:BenWa)]

I had to back out the change because nsCoreAnimation.mm was not linked correctly with libxul disabled. I moved nsCoreAnimationSupport.mm to gfxutils library that is shared between the components.

I will try to push again today.

Comment 19

[Benoit Girard (:BenWa)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3d763264de13

Comment 20

[Benoit Girard (:BenWa)]

Attachment: Fix Horizontal Line

The code was off by 1 on the Y axis since we reset the Y axis to avoid 'rescaling'. This makes the horizontal lines go away.