Last Comment Bug 651192 - Implement new AsyncDrawing model for plugins
: Implement new AsyncDrawing model for plugins
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: x86 Windows 7
: -- normal with 3 votes (vote)
: mozilla13
Assigned To: Bas Schouten (:bas.schouten)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
https://wiki.mozilla.org/NPAPI:AsyncD...
Depends on: 710511 724886 725552
Blocks: 657261
  Show dependency treegraph
 
Reported: 2011-04-19 11:06 PDT by Bas Schouten (:bas.schouten)
Modified: 2015-05-08 12:46 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add types and functions to npapi.h (4.87 KB, patch)
2011-04-19 11:08 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h (7.39 KB, patch)
2011-04-19 11:38 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 4: Allow setting different drawing models across all platforms (12.74 KB, patch)
2011-04-26 13:24 PDT, Bas Schouten (:bas.schouten)
roc: feedback+
Details | Diff | Splinter Review
Part 6: Support AsyncBitmapSurface drawing model (20.30 KB, patch)
2011-04-26 13:24 PDT, Bas Schouten (:bas.schouten)
roc: feedback+
Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v2 (7.75 KB, patch)
2011-05-02 13:08 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Update code to know the new functions (10.05 KB, patch)
2011-05-02 13:09 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 3: Add NPRemoteAsyncSurface structure for IPC (3.93 KB, patch)
2011-05-02 13:10 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 3: Add NPRemoteAsyncSurface structure for IPC v2 (2.34 KB, patch)
2011-05-02 13:13 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: feedback+
Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v3 (7.73 KB, patch)
2011-05-10 12:05 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v4 (7.98 KB, patch)
2011-05-10 12:27 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 2: Update the interface and stub implementors which don't have support (9.94 KB, patch)
2011-05-10 12:28 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 3: Add NPRemoteAsyncSurface structure for IPC v3 (3.34 KB, patch)
2011-05-10 12:31 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: review+
Details | Diff | Splinter Review
Part 4: Allow setting different drawing models across all platforms (11.84 KB, patch)
2011-05-10 12:38 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v5 (7.98 KB, patch)
2011-05-10 12:41 PDT, Bas Schouten (:bas.schouten)
roc: superreview+
Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model (19.46 KB, patch)
2011-05-10 13:41 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing (14.00 KB, patch)
2011-05-10 13:45 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v2 (13.99 KB, patch)
2011-05-10 15:48 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 7: Add reftest for async bitmap drawing (4.00 KB, patch)
2011-05-10 15:49 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v6 (7.98 KB, patch)
2011-05-11 20:43 PDT, Bas Schouten (:bas.schouten)
roc: superreview+
Details | Diff | Splinter Review
Part 2: Update the interface and stub implementors which don't have support (10.28 KB, patch)
2011-05-11 20:44 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 4: Allow setting different drawing models across all platforms v2 (20.46 KB, patch)
2011-05-11 20:44 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v2 (25.98 KB, patch)
2011-05-11 20:46 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v3 (26.86 KB, patch)
2011-05-13 07:52 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v4 (27.05 KB, patch)
2011-05-13 14:55 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v5 (27.09 KB, patch)
2011-05-13 15:33 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v3 (17.56 KB, patch)
2011-05-13 15:34 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v6 (22.69 KB, patch)
2011-05-17 12:23 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4 (13.99 KB, patch)
2011-05-17 14:57 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4 (14.05 KB, patch)
2011-05-17 16:51 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v7 (8.07 KB, patch)
2011-11-23 16:42 PST, Bas Schouten (:bas.schouten)
jaas: review+
roc: superreview+
Details | Diff | Splinter Review
Part 2: Update the interface and stub implementors which don't have support v2 (9.96 KB, patch)
2011-11-23 16:43 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 3: Add NPRemoteAsyncSurface structure for IPC v3 (3.34 KB, patch)
2011-11-23 16:45 PST, Bas Schouten (:bas.schouten)
bas: review+
Details | Diff | Splinter Review
Part 4: Allow setting different drawing models across all platforms v3 (20.38 KB, patch)
2011-11-24 06:47 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 5: Support AsyncBitmapSurface drawing model v7 (22.93 KB, patch)
2011-11-27 06:37 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v8 (11.50 KB, patch)
2012-02-08 11:41 PST, Bas Schouten (:bas.schouten)
jaas: review+
roc: superreview+
Details | Diff | Splinter Review
Part 5: Allow accessing image containers from remote process. (30.63 KB, patch)
2012-02-14 09:25 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Allow accessing image containers from remote process. v2 (43.85 KB, patch)
2012-02-16 18:01 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 1: Add types and functions to npapi.h and npfunctions.h v9 (11.63 KB, patch)
2012-02-16 18:03 PST, Bas Schouten (:bas.schouten)
jaas: review+
roc: superreview+
Details | Diff | Splinter Review
Part 5: Allow accessing image containers from remote process. v3 (45.17 KB, patch)
2012-02-16 23:19 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Allow accessing image containers from remote process. v4 (45.47 KB, patch)
2012-02-18 08:07 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 5: Allow accessing image containers from remote process. v5 (45.48 KB, patch)
2012-02-19 08:10 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 6: Implement the sync bitmap drawing model v5 (34.81 KB, patch)
2012-02-21 20:32 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Implement the sync bitmap drawing model v6 (34.96 KB, patch)
2012-02-25 09:58 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Part 6: Implement the sync bitmap drawing model v7 (36.11 KB, patch)
2012-02-28 21:24 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 7: Implement NPP_DidComposite API. (10.06 KB, patch)
2012-02-28 21:25 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review
Part 8: Add test code to test plugin (14.74 KB, patch)
2012-02-28 21:26 PST, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Splinter Review

Description Bas Schouten (:bas.schouten) 2011-04-19 11:06:16 PDT
We need to implement the new Async drawing model for plugins. The specification of the new drawing model is located at:

https://wiki.mozilla.org/NPAPI:AsyncDrawing
Comment 1 Bas Schouten (:bas.schouten) 2011-04-19 11:08:18 PDT
Created attachment 527047 [details] [diff] [review]
Part 1: Add types and functions to npapi.h

This is a very first attempt to add the types and functions to npapi.h, requesting feedback here to see if this looks alright.
Comment 2 Bas Schouten (:bas.schouten) 2011-04-19 11:38:26 PDT
Created attachment 527057 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h

Fix #ifdef inbalance and add stuff to npfunctions.h
Comment 3 Bas Schouten (:bas.schouten) 2011-04-26 13:24:05 PDT
Created attachment 528418 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms
Comment 4 Bas Schouten (:bas.schouten) 2011-04-26 13:24:40 PDT
Created attachment 528420 [details] [diff] [review]
Part 6: Support AsyncBitmapSurface drawing model
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-04-26 13:35:03 PDT
Did this work get coordinated with the android drawing model that Stuart talked with Adobe about? AFAIK that ended up in the android distro sources but never got proposed to plugin-futures, so we might have competing proposals in place.
Comment 6 Josh Aas 2011-04-26 13:52:12 PDT
We are not done working out the spec, Bas is just implementing early as a check on our design decisions.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-26 14:25:10 PDT
Comment on attachment 528418 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms

Review of attachment 528418 [details] [diff] [review]:
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-26 14:32:04 PDT
Comment on attachment 527057 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h

Review of attachment 527057 [details] [diff] [review]:

::: modules/plugin/base/public/npapi.h
@@ +232,5 @@
+typedef enum {
+  NPImageFormatARGB32P    = 0x1,
+  NPImageFormatARGB32     = 0x2,
+  NPImageFormatXRGB32     = 0x4
+} NPImageFormat;

Something needs to specify the exact byte layout (endianness, etc) of these formats.

::: modules/plugin/base/public/npfunctions.h
@@ +125,5 @@
 typedef NPBool       (*NPN_ConvertPointPtr)(NPP instance, double sourceX, double sourceY, NPCoordinateSpace sourceSpace, double *destX, double *destY, NPCoordinateSpace destSpace);
 typedef NPBool       (*NPN_HandleEventPtr)(NPP instance, void *event, NPBool handled);
 typedef NPBool       (*NPN_UnfocusInstancePtr)(NPP instance, NPFocusDirection direction);
 typedef void         (*NPN_URLRedirectResponsePtr)(NPP instance, void* notifyData, NPBool allow);
+typedef NPError      (*NPN_CreateAsyncSurfacePtr)(NPP instance, NPAsyncSurface *surface);

I don't like the way some fields of 'surface' are inputs and some are outputs here. I think the whole surface struct should consistently treated as either an input or an output.

So I would say that NPN_CreateAsyncSurfacePtr should take version, size and format as parameters. (Do we really need version?)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-26 15:13:05 PDT
Comment on attachment 528420 [details] [diff] [review]
Part 6: Support AsyncBitmapSurface drawing model

Review of attachment 528420 [details] [diff] [review]:

::: dom/plugins/ipc/PPluginInstance.ipdl
@@ +217,5 @@
                        NPCoordinateSpace destSpace)
     returns (double destX, double destY, bool result);
 
+  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
+    returns (NPError result);

Need to document that changedRect is in plugin coordinates here --- *unlike* the npapi method of the same name, hence why this needs to be carefully documented.

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +1060,5 @@
+            if (npevent->event == WM_PAINT || npevent->event == DoublePassRenderingEvent()) {
+                // This plugin maintains its own async drawing.
+                return handled;
+            }
+        }

Don't we need this change for other platforms too?
Comment 10 Bas Schouten (:bas.schouten) 2011-05-02 13:08:56 PDT
Created attachment 529550 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v2

Updated to the latest API spec.
Comment 11 Bas Schouten (:bas.schouten) 2011-05-02 13:09:52 PDT
Created attachment 529552 [details] [diff] [review]
Part 2: Update code to know the new functions
Comment 12 Bas Schouten (:bas.schouten) 2011-05-02 13:10:31 PDT
Created attachment 529553 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC
Comment 13 Bas Schouten (:bas.schouten) 2011-05-02 13:13:18 PDT
Created attachment 529555 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC v2

Updated to remove some dead code in the patch.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-03 09:39:38 PDT
Comment on attachment 529555 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC v2

Hm, where's the beef?

Are these new surface types going to become gfxASurface for now in the chrome process?  If so, I'd just add them to the existing SurfaceDescriptor union.  There's going to be boilerplate shared between old and new models.  If not, what kind of surface will they become?  (There's not really such a thing as an "async surface", just surfaces that the async drawing model supports.)
Comment 15 Bas Schouten (:bas.schouten) 2011-05-10 12:05:51 PDT
Created attachment 531401 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v3
Comment 16 Bas Schouten (:bas.schouten) 2011-05-10 12:27:03 PDT
Created attachment 531413 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v4

Added an updated description of the pixel layout in memory, as requested by roc in previous comments.
Comment 17 Bas Schouten (:bas.schouten) 2011-05-10 12:28:28 PDT
Created attachment 531417 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support
Comment 18 Bas Schouten (:bas.schouten) 2011-05-10 12:31:37 PDT
Created attachment 531420 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC v3

So these will not all become gfxASurfaces to add your question (some will just go directly to specialized Images for example). I realize the term 'AsyncSurface' is a bit ambiguous. But it's probably the best description for a Surface which is transferred through the Async drawing model.
Comment 19 Bas Schouten (:bas.schouten) 2011-05-10 12:38:38 PDT
Created attachment 531425 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms

This patch for now only reports support for the drawing model on windows, as we'll need to change the event loop for other platforms as Roc pointed out. This shouldn't be hard and when this is done we can make this return true on the relevant platforms.
Comment 20 Bas Schouten (:bas.schouten) 2011-05-10 12:41:56 PDT
Created attachment 531427 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v5

Updated again to correct a typo.
Comment 21 Bas Schouten (:bas.schouten) 2011-05-10 13:41:32 PDT
Created attachment 531450 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model
Comment 22 Bas Schouten (:bas.schouten) 2011-05-10 13:45:33 PDT
Created attachment 531455 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing

This allows the test plugin to do async bitmap drawing for solid color tests.
Comment 23 Bas Schouten (:bas.schouten) 2011-05-10 15:48:30 PDT
Created attachment 531478 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v2

Updated to correct for updated constant.
Comment 24 Bas Schouten (:bas.schouten) 2011-05-10 15:49:36 PDT
Created attachment 531480 [details] [diff] [review]
Part 7: Add reftest for async bitmap drawing

This adds reftests that test if Async bitmap drawing is rendering sanely, and if it's updating properly.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 17:19:35 PDT
Comment on attachment 531427 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v5

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

::: dom/plugins/base/npapi.h
@@ +229,5 @@
>  } NPFocusDirection;
>  
> +/* These formats describe the format in the memory byte-order. This means if
> + * a 32-bit value of a pixel is viewed on a little-endian system the layout will
> + * be 0xAARRGGBB. Since the Alpha channel will be stored in the most significant

/Since the/The/

@@ +230,5 @@
>  
> +/* These formats describe the format in the memory byte-order. This means if
> + * a 32-bit value of a pixel is viewed on a little-endian system the layout will
> + * be 0xAARRGGBB. Since the Alpha channel will be stored in the most significant
> + * bits */

bits.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 17:20:39 PDT
Comment on attachment 531417 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support

Review of attachment 531417 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 17:26:16 PDT
Comment on attachment 531425 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms

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

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +71,4 @@
>  #ifdef NP_NO_QUICKDRAW
>      mDrawingModel(NPDrawingModelCoreGraphics),
>  #else
> +    mDrawingModel(NPDrawingModelAsyncBitmapSurface),

Shouldn't we be keeping the #ifdef XP_MACOSX and the NPDrawingModelQuickDraw code, and using NPDrawingModelAsyncBitmapSurface as the default drawing model for other platforms?

In fact, why isn't this the same as the PluginInstanceChild/PluginInstanceParent code?

It would be simpler if there was one place that defines the default drawing model using #ifdefs.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 17:57:55 PDT
Comment on attachment 531450 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model

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

As we discussed on IRC, I think we should support off-main-thread SetCurrentSurface calls from the beginning, even if our implementation does something stupid like proxy to the main thread. If we don't support it now at all, then adding support later will require revving interfaces and introducing new feature sniffing. If we support it via proxying now, then we can improve it internally later without affecting compatibility.

I think we don't need to support off-main-thread Init and Finalize calls. But we do need to make sure that SetCurrentSurface calls from multiple threads get ordered correctly, and that we don't trip over any issues with a Finalize that destroys a surface for a pending SetCurrentSurface.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2321,5 @@
> +  {
> +    return true;
> +  }
> +
> +  return false;

just
  return mDrawingModel == NPDrawingModelAsyncBitmapSurface ||
         mDrawingngModel == NPDrawingModelAsyncWindowsDXGISurface ||
         mDrawingModel == NPDrawingModelAsyncWindowsDX9ExSurface;

@@ +2453,5 @@
> +    pluginChanged.right = mWindow.width;
> +    pluginChanged.bottom = mWindow.height;
> +  } else {
> +    float xRatio = float(mWindow.width) / float(surface->size.width);
> +    float yRatio = float(mWindow.height) / float(surface->size.height);

Need to protect against zero width/height

@@ +2457,5 @@
> +    float yRatio = float(mWindow.height) / float(surface->size.height);
> +
> +    pluginChanged.left = (uint16_t)floor(float(changed->left) * xRatio);
> +    pluginChanged.right = (uint16_t)ceil(float(changed->right) * xRatio);
> +    pluginChanged.top = (uint16_t)floor(float(changed->right) * yRatio);

changed->top

@@ +2458,5 @@
> +
> +    pluginChanged.left = (uint16_t)floor(float(changed->left) * xRatio);
> +    pluginChanged.right = (uint16_t)ceil(float(changed->right) * xRatio);
> +    pluginChanged.top = (uint16_t)floor(float(changed->right) * yRatio);
> +    pluginChanged.bottom = (uint16_t)ceil(float(changed->right) * yRatio);

changed->bottom

@@ +2466,5 @@
> +
> +  if (result == NPERR_NO_ERROR && mDrawingModel == NPDrawingModelAsyncBitmapSurface) {
> +    if (mCurrentBitmapSurfaceFinalized) {
> +      DeallocateAsyncBitmapSurface(mCurrentBitmapSurface);
> +      mCurrentBitmapSurfaceFinalized = false;

Personally I'd just disallow Finalizing a surface while it's current!

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +332,5 @@
> +PluginInstanceParent::IsAsyncDrawing()
> +{
> +  if (mDrawingModel == NPDrawingModelAsyncBitmapSurface ||
> +      mDrawingModel == NPDrawingModelAsyncWindowsDXGISurface ||
> +      mDrawingModel == NPDrawingModelAsyncWindowsDX9ExSurface)

Should be able to share this with PluginInstanceChild somehow.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 18:43:20 PDT
Comment on attachment 531478 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v2

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

This will need to be extended to test calling SetCurrent from a non-main-thread.

We should also test passing non-NULL for the "changed" rectangle as well.

::: modules/plugin/test/testplugin/nptest.cpp
@@ +554,5 @@
> +  r = PRUint8(float(alpha * r) / 0xFF);
> +  g = PRUint8(float(alpha * g) / 0xFF);
> +  b = PRUint8(float(alpha * b) / 0xFF);
> +  PRUint32 premultiplied =
> +    (alpha << 24) +	(r << 16) + (g << 8) + b;

This is wrong on big-endian platforms, isn't it?

@@ +1067,5 @@
> +      NPN_FinalizeAsyncSurface(instance, instanceData->frontBuffer);
> +      NPN_MemFree(instanceData->frontBuffer);
> +    }
> +    if (instanceData->backBuffer) {
> +      NPN_FinalizeAsyncSurface(instance, instanceData->backBuffer);

This will have to be changed if we can't finalize a surface that's current; I guess you'll have to call SetCurrent(NULL) first.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 18:46:50 PDT
Comment on attachment 531480 [details] [diff] [review]
Part 7: Add reftest for async bitmap drawing

Review of attachment 531480 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-10 18:48:27 PDT
Somewhere, probably part 4, we need to add a hidden pref to enable/disable the asyncbitmap drawing model, default to false. automation.py.in should set that pref.
Comment 32 Bas Schouten (:bas.schouten) 2011-05-11 17:55:00 PDT
(In reply to comment #28)
> Comment on attachment 531450 [details] [diff] [review] [review]
> ::: dom/plugins/ipc/PluginInstanceParent.cpp
> @@ +332,5 @@
> > +PluginInstanceParent::IsAsyncDrawing()
> > +{
> > +  if (mDrawingModel == NPDrawingModelAsyncBitmapSurface ||
> > +      mDrawingModel == NPDrawingModelAsyncWindowsDXGISurface ||
> > +      mDrawingModel == NPDrawingModelAsyncWindowsDX9ExSurface)
> 
> Should be able to share this with PluginInstanceChild somehow.

I don't think that's very easy. Considering they're completely separate objects and this is a member, we could do IsDrawingModelAsync(NPDrawingModel *aModel) and make these functions call that perhaps? I'm not sure if that's worth it though.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 18:56:47 PDT
I think it's worth doing something since this is going to be extended and we don't want to have to change it in multiple places.

You can put a function in a header file and just #include it from both places.
Comment 34 Bas Schouten (:bas.schouten) 2011-05-11 20:43:04 PDT
Created attachment 531840 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v6

Updated per IRC discussions and review comments.
Comment 35 Bas Schouten (:bas.schouten) 2011-05-11 20:44:02 PDT
Created attachment 531841 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support

Updated to reflect new function signatures.
Comment 36 Bas Schouten (:bas.schouten) 2011-05-11 20:44:57 PDT
Created attachment 531842 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms v2

Updated to work with a pref.
Comment 37 Bas Schouten (:bas.schouten) 2011-05-11 20:46:39 PDT
Created attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

Updated to support calling SetCurrentAsyncSurface off the main thread. I'm still working on tests for this but the new logic seems to work fine when calling SetCurrent from the main thread.
Comment 38 Bas Schouten (:bas.schouten) 2011-05-11 20:47:20 PDT
Comment on attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

Requesting cjones to also sanity check my threading code.
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-11 22:10:33 PDT
Comment on attachment 531420 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC v3

>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    aMsg->WriteInt16(int16(aParam));
>+  }
>+
>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)

Please check that these values are legal.

r=me with that.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:16:46 PDT
Comment on attachment 531840 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v6

Review of attachment 531840 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:17:52 PDT
Comment on attachment 531841 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support

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

::: dom/base/nsGlobalWindow.cpp
@@ +460,5 @@
> +                           void *initData, NPAsyncSurface *surface)
> +  { return NPERR_GENERIC_ERROR; }
> +
> +  NPError FinalizeAsyncSurface(NPAsyncSurface *surface) { return NPERR_GENERIC_ERROR; }
> +  void SetCurrentAsyncSurface(NPAsyncSurface *surface, NPRect *changed) { return; }

don't need the "return;"
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:22:36 PDT
Comment on attachment 531842 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms v2

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

::: dom/plugins/base/nsIPluginInstance.idl
@@ +69,5 @@
> +#else
> +#ifndef NP_NO_QUICKDRAW
> +const NPDrawingModel kDefaultDrawingModel = NPDrawingModelQuickDraw;
> +#else
> +const NPDrawingModel kDefaultDrawingModel = NPDrawingModelCoreGraphics;

Doesn't this cause a problem with duplicate definitions when the file is #included multiple times? Make these static?
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:26:26 PDT
I was wondering whether we should have one pref per async surface format. I don't feel strongly about it.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:47:33 PDT
Comment on attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2403,5 @@
> +
> +void
> +PluginInstanceChild::UpdateCurrentAsyncSurface()
> +{
> +  if (!mNeedAsyncSurfaceUpdate) {

Use AssertCurrentThreadOwns on the mutex here.

@@ +2469,5 @@
> +    mAsyncChangedRect = NULL;
> +  }
> +
> +  bool success;
> +  CallNPN_SetCurrentAsyncSurface(remoteAsync, pluginChanged, &success);

We're doing a blocking RPC while holding our mutex? That sounds bad.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:48:19 PDT
Comment on attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

I didn't actually mean to mark this r+ yet, I'm worried about the rpc with mutex held.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-11 22:49:17 PDT
I guess I'm looking for a new Part 6 patch?
Comment 47 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-12 16:40:45 PDT
(In reply to comment #44)
> We're doing a blocking RPC while holding our mutex? That sounds bad.

Yeah that's asking for trouble.
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-12 17:30:37 PDT
Comment on attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

General nit: in the .cpp, |using namespace mozilla;| and
s/mozilla::MutexAutoLock/MutexAutoLock/.  (The code should have been
defined in namespace mozilla::plugins, historical accident.)  Also,
the conventional name for the RAII lock instance var is |lock|, so
s/autoLock/lock/.

>+  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
>+    returns (bool result);
>+
>+  rpc ClearCurrentAsyncSurface();

Why are these 'rpc'?  I don't see why they need to allow re-entry.
It's not clear to me why they can't be 'async', actually (the bool
return doesn't seem very useful.)

>+    , mAsyncSurfaceMutex("AsyncSurfaceMutex")

Nit: "PluginInstanceChild.mAsyncSurfaceMutex".  (This name is used in
error messages where we don't necessarily have stack traces.)

>+NPError
>+PluginInstanceChild::DeallocateAsyncBitmapSurface(NPAsyncSurface *aSurface)
>+{
>+  for (unsigned int i = 0; i < mAsyncBitmaps.Length(); i++) {

Yuck.  Use a map.

>+  switch (mDrawingModel) {
>+  case NPDrawingModelAsyncBitmapSurface:
>+    {

Nit: brace on same line as case label, "case Foo: {".

>+      for (unsigned int i = 0; i < mAsyncBitmaps.Length(); i++) {
>+        if (mAsyncBitmaps[i].mSurface == surface) {
>+          return NPERR_INVALID_PARAM;

Is this part of the spec?

>+      if (!entry.mSharedMem.IsWritable()) {

You can NS_ABORT_IF_FALSE() for this.

>+NPError
>+PluginInstanceChild::NPN_FinalizeAsyncSurface(NPAsyncSurface *surface)
>+{
>+  AssertPluginThread();
>+
>+  if (!IsAsyncDrawing()) {
>+    return NPERR_GENERIC_ERROR;
>+  }
>+
>+  mozilla::MutexAutoLock autoLock(mAsyncSurfaceMutex);
>+  UpdateCurrentAsyncSurface();
>+
>+  switch (mDrawingModel) {
>+  case NPDrawingModelAsyncBitmapSurface:
>+    {
>+      if (mCurrentAsyncSurface == surface ||
>+          mComingCurrentAsyncSurface == surface)
>+      {

Nit: brace on same line as closing paren of if-condition.

>+        return NPERR_INVALID_PARAM;
>+      }

Non-nit: is this check of the spec?  (I don't recall.)  I don't think
this is the behavior we'd want --- if we return an error to the
plugin, we have to rely on it to check the error and do something
intelligent with |surface|.  I don't like that gamble.  Instead, how
about we clear the current or next surface if this happens?

Why does this call UpdateCurrentAsyncSurface()?  (And so need to hold
the mutex.)

>+void
>+PluginInstanceChild::UpdateCurrentAsyncSurface()
>+{

Assert mutex is held.

>+  if (!mNeedAsyncSurfaceUpdate) {
>+    return;
>+  }
>+
>+  uint32_t version = 0;
>+  AsyncSurfaceDescriptor surfDescriptor;
>+
>+  if (!mComingCurrentAsyncSurface) {
>+    CallClearCurrentAsyncSurface();

I think you'd be better off having a single SetCurrentSurface() that
takes a nullable argument (union with null_t).

>+      if (!entry.mSurface) {
>+        return;
>+      }

When could this happen?

>+
>+      surfDescriptor = entry.mSharedMem;
>+      break;
>+    }
>+  }
>+
>+  NPRect pluginChanged;
>+
>+  NPRemoteAsyncSurface remoteAsync(version,

s/remoteAsync/remoteSurface/?

>+  if (success) {
>+    mCurrentAsyncSurface = mComingCurrentAsyncSurface;
>+    mNeedAsyncSurfaceUpdate = false;
>+  }

I'm not sure what the right way to handle this error is, but I'm not
sure we care, see above.

>+void
>+PluginInstanceChild::UpdateAsyncSurfaceTask::Run()
>+{
>+  if (!mCancelled) {

CancelableTask does this check for you.

>+void
>+PluginInstanceChild::UpdateAsyncSurfaceTask::Cancel()
>+{
>+  mCancelled = true;
>+}

... and this.

>+  UpdateAsyncSurfaceTask *task =
>+    new UpdateAsyncSurfaceTask(this);
>+  
>+  mCurrentUpdateAsyncSurfaceTask = task;
>+
>+  ProcessChild::message_loop()->PostTask(FROM_HERE, task);

Kill the temporary |task| variable.

>+    struct AsyncBitmapEntry {
>+      NPAsyncSurface *mSurface;
>+      Shmem mSharedMem;
>+    };

(This goes away when you use a map.)

>+    // Access guarded by AsyncSurfaceMutex. The way these works is at follows,

"as follows"

>+    class UpdateAsyncSurfaceTask : public Task

You don't need a custom task type for this, just use CancelableTask.

>+    NPAsyncSurface        *mCurrentAsyncSurface;

Nit: "NPAsyncSurface* mCurrentAsyncSurface", don't try to align field
names.

>+    nsTArray<AsyncBitmapEntry> mAsyncBitmaps;

As noted above, use a map here.

In NPP_Destroy(), we should clean up whatever bitmaps are still here,
and probably NS_ASSERTION() for an error message.

>+    NPAsyncSurface        *mComingCurrentAsyncSurface;

I prefer the name "mNextAsyncSurface".  (Don't like "Async", as we
discussed, but there are naming conflicts here.)

>+    nsAutoPtr<NPRect>     mAsyncChangedRect;

Ew.  Can we use nsIntRect for this and check for IsEmpty() instead == null?
Comment 49 Bas Schouten (:bas.schouten) 2011-05-12 19:49:26 PDT
(In reply to comment #48)
> Comment on attachment 531843 [details] [diff] [review] [review]
> Part 5: Support AsyncBitmapSurface drawing model v2
> 
> General nit: in the .cpp, |using namespace mozilla;| and
> s/mozilla::MutexAutoLock/MutexAutoLock/.  (The code should have been
> defined in namespace mozilla::plugins, historical accident.)  Also,
> the conventional name for the RAII lock instance var is |lock|, so
> s/autoLock/lock/.

Ok, we should document these things if that's the style we prefer. There's several styling guides that frown upon using namespace for these things. Our style guide doesn't mention anything about it except that it's 'allowed' in cpp files.


> 
> >+  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
> >+    returns (bool result);
> >+
> >+  rpc ClearCurrentAsyncSurface();
> 
> Why are these 'rpc'?  I don't see why they need to allow re-entry.
> It's not clear to me why they can't be 'async', actually (the bool
> return doesn't seem very useful.)

A finalize will de-allocate the memory if a surface is current. It needs to be sure that the Parent no longer relies on the memory being present. Therefore this function shouldn't return until it has been processed. We can remove it from the Mutex though.

> >+NPError
> >+PluginInstanceChild::DeallocateAsyncBitmapSurface(NPAsyncSurface *aSurface)
> >+{
> >+  for (unsigned int i = 0; i < mAsyncBitmaps.Length(); i++) {
> 
> Yuck.  Use a map.

I assume you mean stl::map? I didn't find a map class in Mozilla code. But since stl::vector is already used maybe I should've realized using map was fine.

> 
> >+  switch (mDrawingModel) {
> >+  case NPDrawingModelAsyncBitmapSurface:
> >+    {
> 
> Nit: brace on same line as case label, "case Foo: {".

Really? That's ew :) It indeed seems to be listed in our new style guide though.

> >+      for (unsigned int i = 0; i < mAsyncBitmaps.Length(); i++) {
> >+        if (mAsyncBitmaps[i].mSurface == surface) {
> >+          return NPERR_INVALID_PARAM;
> 
> Is this part of the spec?

It should be in my opinion. There's a bunch of things in the Spec I still need to update.

> >+NPError
> >+PluginInstanceChild::NPN_FinalizeAsyncSurface(NPAsyncSurface *surface)
> >+{
> >+  AssertPluginThread();
> >+
> >+  if (!IsAsyncDrawing()) {
> >+    return NPERR_GENERIC_ERROR;
> >+  }
> >+
> >+  mozilla::MutexAutoLock autoLock(mAsyncSurfaceMutex);
> >+  UpdateCurrentAsyncSurface();
> >+
> >+  switch (mDrawingModel) {
> >+  case NPDrawingModelAsyncBitmapSurface:
> >+    {
> >+      if (mCurrentAsyncSurface == surface ||
> >+          mComingCurrentAsyncSurface == surface)
> >+      {
> 
> Nit: brace on same line as closing paren of if-condition.

As far as anyone in Mozilla has ever told me, that's a not-done for multi-line conditions. I'm fine with changing this, but can we please make up our minds about these style issues? I realize K&R makes no exception for multi-line conditions but I do agree it looks a bit cleaner with a newline.

> 
> >+        return NPERR_INVALID_PARAM;
> >+      }
> 
> Non-nit: is this check of the spec?  (I don't recall.)  I don't think
> this is the behavior we'd want --- if we return an error to the
> plugin, we have to rely on it to check the error and do something
> intelligent with |surface|.  I don't like that gamble.  Instead, how
> about we clear the current or next surface if this happens?

I firmly believe we shouldn't have a catastrophic failure if someone passes an NPAsyncSurface that was never initialized. Part of solid API design is handling situations where you get invalid input in a graceful way. It's the API user's responsibility to do something sensible with that error.

> 
> Why does this call UpdateCurrentAsyncSurface()?  (And so need to hold
> the mutex.)

Because a SetCurrentCall coming from another thread may not have been processed when this Finalize call comes in. If it hasn't the finalize call may kill a current surface if the setcurrent call that made this surface un-current wasn't processed.

> >+  if (!mNeedAsyncSurfaceUpdate) {
> >+    return;
> >+  }
> >+
> >+  uint32_t version = 0;
> >+  AsyncSurfaceDescriptor surfDescriptor;
> >+
> >+  if (!mComingCurrentAsyncSurface) {
> >+    CallClearCurrentAsyncSurface();
> 
> I think you'd be better off having a single SetCurrentSurface() that
> takes a nullable argument (union with null_t).

Could do that. This seemed a little bit more readable/cleaner (i.e. in the other case we'd still be passing a changed rect for no reason and all). But I don't feel strongly about it.

> 
> >+      if (!entry.mSurface) {
> >+        return;
> >+      }
> 
> When could this happen?

When someone specifies an Async surface that doesn't exist, I don't think crashing it the right thing to do.

> 
> >+    class UpdateAsyncSurfaceTask : public Task
> 
> You don't need a custom task type for this, just use CancelableTask.

I'll still need a custom class to grab the mutex right? I can just avoid the cancel part.

> 
> >+    NPAsyncSurface        *mCurrentAsyncSurface;
> 
> Nit: "NPAsyncSurface* mCurrentAsyncSurface", don't try to align field
> names.

That's what most of the file seems to do ... When in Rome and all :)

> 
> In NPP_Destroy(), we should clean up whatever bitmaps are still here,
> and probably NS_ASSERTION() for an error message.
> 
> >+    NPAsyncSurface        *mComingCurrentAsyncSurface;
> 
> I prefer the name "mNextAsyncSurface".  (Don't like "Async", as we
> discussed, but there are naming conflicts here.)

Sure.

> 
> >+    nsAutoPtr<NPRect>     mAsyncChangedRect;
> 
> Ew.  Can we use nsIntRect for this and check for IsEmpty() instead == null?

I think so :) I'll have a look.
Comment 50 Bas Schouten (:bas.schouten) 2011-05-13 06:18:58 PDT
> 
> Non-nit: is this check of the spec?  (I don't recall.)  I don't think
> this is the behavior we'd want --- if we return an error to the
> plugin, we have to rely on it to check the error and do something
> intelligent with |surface|.  I don't like that gamble.  Instead, how
> about we clear the current or next surface if this happens?

Ugh, I misread this comment, sorry. I'm actually okay with this idea. I'll discuss it with roc and josh.

> 
> >+void
> >+PluginInstanceChild::UpdateAsyncSurfaceTask::Run()
> >+{
> >+  if (!mCancelled) {
> 
> CancelableTask does this check for you.

As far as I can see CancelableTask is just an abstract base class that does nothing for me except put the Cancel call in the vftable?
Comment 51 Bas Schouten (:bas.schouten) 2011-05-13 07:52:28 PDT
Created attachment 532231 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v3

Updated to address the majority of review comments.
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-13 12:22:12 PDT
(In reply to comment #49)
> A finalize will de-allocate the memory if a surface is current. It needs to
> be sure that the Parent no longer relies on the memory being present.

Is there a reason that the parent can't destroy the surface?

> I assume you mean stl::map? I didn't find a map class in Mozilla code. But
> since stl::vector is already used maybe I should've realized using map was
> fine.

Yes, std::map.  We use map in gecko.

> > Nit: brace on same line as closing paren of if-condition.
> 
> As far as anyone in Mozilla has ever told me, that's a not-done for
> multi-line conditions.

The sad fact of the matter is, style is enforced by module owners, who don't necessarily refer to the style guide.  I'm not the module owner of dom/plugins/ipc, but I have to read this code, and I haven't seen this line-break style anywhere else for 2-space indent.  So, it's just a distraction.  The larger issue of consistent style across gecko is a giant can of worms, but if you want to open it, feel free :).

> > 
> > Why does this call UpdateCurrentAsyncSurface()?  (And so need to hold
> > the mutex.)
> 
> Because a SetCurrentCall coming from another thread may not have been
> processed when this Finalize call comes in. If it hasn't the finalize call
> may kill a current surface if the setcurrent call that made this surface
> un-current wasn't processed.

Please document that.

> > I think you'd be better off having a single SetCurrentSurface() that
> > takes a nullable argument (union with null_t).
> 
> Could do that. This seemed a little bit more readable/cleaner (i.e. in the
> other case we'd still be passing a changed rect for no reason and all). But
> I don't feel strongly about it.

Neither do I.

> 
> > 
> > >+    class UpdateAsyncSurfaceTask : public Task
> > 
> > You don't need a custom task type for this, just use CancelableTask.
> 
> I'll still need a custom class to grab the mutex right? I can just avoid the
> cancel part.

No, you're already having the task run a method that grabs the mutex; the task itself has nothing to do with it.

You just need

  CancelableTask *mTask;
  //...
  mTask = NewRunnableMethod(this, &Child::LockAndCalUpdate)

> 
> > 
> > >+    NPAsyncSurface        *mCurrentAsyncSurface;
> > 
> > Nit: "NPAsyncSurface* mCurrentAsyncSurface", don't try to align field
> > names.
> 
> That's what most of the file seems to do ... When in Rome and all :)

This code is a mess.  The majority seem to not-align and do "Foo*".

(In reply to comment #50)
> > >+void
> > >+PluginInstanceChild::UpdateAsyncSurfaceTask::Run()
> > >+{
> > >+  if (!mCancelled) {
> > 
> > CancelableTask does this check for you.
> 
> As far as I can see CancelableTask is just an abstract base class that does
> nothing for me except put the Cancel call in the vftable?

Yes sorry, I misspoke: The concrete impl of NewRunnableMethod(), which is a CancelableTask, does the check for you.  See code snippet above.
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-13 12:24:07 PDT
(In reply to comment #52)
>   CancelableTask *mTask;

Nit: "CancelableTask*" ;).  (Copied and pasted that, whups.)
Comment 54 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-13 12:49:52 PDT
(In reply to comment #48)
> >+  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
> >+    returns (bool result);
> >+
> >+  rpc ClearCurrentAsyncSurface();
> 
> Why are these 'rpc'?  I don't see why they need to allow re-entry.
> It's not clear to me why they can't be 'async'

I think the answer is that SetCurrent/ClearCurrent affect *all* surfaces in that they make the specified one current, and make all others not-current.  This means the plugin process has to rendezvous with the browser process in order to guarantee the SetCurrent/ClearCurrent has been processed before returning.  This is a bit unfortunate in that it means plugins that try to do e.g. triple-buffering will always have to pay the sync cost, but I doubt it will be a problem in practice.  (Hasn't been yet.)

So I think you want to use 'sync' here, but not 'rpc'.
Comment 55 Bas Schouten (:bas.schouten) 2011-05-13 13:30:17 PDT
(In reply to comment #52)
> (In reply to comment #49)
> > 
> > As far as anyone in Mozilla has ever told me, that's a not-done for
> > multi-line conditions.
> 
> The sad fact of the matter is, style is enforced by module owners, who don't
> necessarily refer to the style guide.  I'm not the module owner of
> dom/plugins/ipc, but I have to read this code, and I haven't seen this
> line-break style anywhere else for 2-space indent.  So, it's just a
> distraction.  The larger issue of consistent style across gecko is a giant
> can of worms, but if you want to open it, feel free :).

One problem here is I need to make this 4-space indent, it seems to be what the rest of the file is :(. But I'll still put the braces on the same line :).

(In reply to comment #54)
> (In reply to comment #48)
> > >+  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
> > >+    returns (bool result);
> > >+
> > >+  rpc ClearCurrentAsyncSurface();
> > 
> > Why are these 'rpc'?  I don't see why they need to allow re-entry.
> > It's not clear to me why they can't be 'async'
> 
> I think the answer is that SetCurrent/ClearCurrent affect *all* surfaces in
> that they make the specified one current, and make all others not-current. 
> This means the plugin process has to rendezvous with the browser process in
> order to guarantee the SetCurrent/ClearCurrent has been processed before
> returning.  This is a bit unfortunate in that it means plugins that try to
> do e.g. triple-buffering will always have to pay the sync cost, but I doubt
> it will be a problem in practice.  (Hasn't been yet.)
> 
> So I think you want to use 'sync' here, but not 'rpc'.

That's true, but that setting current could fail. So ClearCurrentAsyncSurface should be sync I suppose. We could make SetCurrent also be sync and upon failure clear the current surface altogether, but I'm not sure if that's best or not.
Comment 56 Bas Schouten (:bas.schouten) 2011-05-13 14:55:04 PDT
Created attachment 532355 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v4

Updated to process all bits of review comments. Except not changing SetCurrentAsync to 'sync' yet, since we need to discuss how to handle failure.

Also still using NPERR for trying to finalize a surface that is/will be current.
Comment 57 Bas Schouten (:bas.schouten) 2011-05-13 15:33:54 PDT
Created attachment 532363 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v5

This fixes a minor issue in the event logic of the last patch that I found when running the multi-threaded testing plugin.
Comment 58 Bas Schouten (:bas.schouten) 2011-05-13 15:34:42 PDT
Created attachment 532365 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v3

This patch also allows selecting off-main-thread drawing on supported platforms (windows only in this patch).
Comment 59 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-15 15:23:05 PDT
(In reply to comment #55)
> That's true, but that setting current could fail. So
> ClearCurrentAsyncSurface should be sync I suppose. We could make SetCurrent
> also be sync and upon failure clear the current surface altogether, but I'm
> not sure if that's best or not.

What does it mean to fail to set the current surface?
Comment 60 Bas Schouten (:bas.schouten) 2011-05-15 18:30:09 PDT
(In reply to comment #59)
> (In reply to comment #55)
> > That's true, but that setting current could fail. So
> > ClearCurrentAsyncSurface should be sync I suppose. We could make SetCurrent
> > also be sync and upon failure clear the current surface altogether, but I'm
> > not sure if that's best or not.
> 
> What does it mean to fail to set the current surface?

The parent could be out of address space (admittedly this is bad :)), or the NPAsyncSurface could be invalid somehow. In other drawing models it could also have different meanings depending on the drawing model, and in each of these cases we'd want to have an architecture in which we can deal with this elegantly.
Comment 61 Bas Schouten (:bas.schouten) 2011-05-17 11:28:31 PDT
Comment on attachment 532365 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v3

Obsolete now that we've decided we're not going to do multithreading for now.
Comment 62 Bas Schouten (:bas.schouten) 2011-05-17 11:29:04 PDT
Comment on attachment 532363 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v5

Obsolete now that we've decided we're not going to do multithreading for now.
Comment 63 Bas Schouten (:bas.schouten) 2011-05-17 12:23:07 PDT
Created attachment 533038 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v6

Updated patch without support for any off-main-thread functionality.
Comment 64 Bas Schouten (:bas.schouten) 2011-05-17 14:57:15 PDT
Created attachment 533084 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4

Fixed up version of plugin test code patch without multithreading support.
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 16:35:24 PDT
Comment on attachment 533038 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v6

Review of attachment 533038 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 16:38:37 PDT
Part 6 v4 is still using PRInt32 to write pixels to the buffer, but we need to use memcpy of a byte array.
Comment 67 Bas Schouten (:bas.schouten) 2011-05-17 16:51:16 PDT
Created attachment 533117 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4

Upload the right v4 patch.
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 17:06:57 PDT
What I expected to see was something like
  unsigned char pixel[4] = { drawColor, drawColor >> 8, drawColor >> 16, drawColor >> 24 };
  for (unsigned char* buf = instanceData->backBuffer->data; buf < bufEnd; buf += 4) {
    memcpy(buf, pixel, 4);
  }

(I'm pretty sure modern compilers optimize that memcpy to a move, and performance doesn't matter here anyway.)
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-17 17:13:07 PDT
Comment on attachment 533117 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4

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

BTW somewhere we should test that when we're in saync drawing mode, WM_PAINT etc is not called. We can probably do that from script by calling getPaintCount, though.

::: modules/plugin/test/testplugin/nptest.cpp
@@ +1083,5 @@
> +    NPN_InitAsyncSurface(instance, &size, NPImageFormatBGRA32, NULL, instanceData->frontBuffer);
> +    NPN_InitAsyncSurface(instance, &size, NPImageFormatBGRA32, NULL, instanceData->backBuffer);
> +
> +    drawAsyncBitmapColor(instanceData);
> +  }

Split the code for updating async bitmaps out to its own function.

::: modules/plugin/test/testplugin/nptest_platform.h
@@ +50,5 @@
>  
>  /**
> + * Returns true if the plugin supports async bitmap drawing.
> + */
> +bool    pluginSupportsAsyncBitmapDrawing();

Why don't we just support the test plugin using async bitmap drawing for all platforms? The plugin-side async bitmap code is crossplatform after all.
Comment 70 Bas Schouten (:bas.schouten) 2011-10-28 08:34:40 PDT
Comment on attachment 533117 [details] [diff] [review]
Part 6: Add code to the test plugin to allow selecting Async bitmap drawing v4

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

We won't be doing this, at least not in this form.
Comment 71 Bas Schouten (:bas.schouten) 2011-11-23 16:42:23 PST
Created attachment 576651 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v7

Updated patch to compile against current m-c. In light of re-emphasizing NPAsync model.
Comment 72 Bas Schouten (:bas.schouten) 2011-11-23 16:43:49 PST
Created attachment 576652 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support v2

Updated to compile, nothing major changed, but some stuff moved around so re-requesting review.
Comment 73 Bas Schouten (:bas.schouten) 2011-11-23 16:45:56 PST
Created attachment 576653 [details] [diff] [review]
Part 3: Add NPRemoteAsyncSurface structure for IPC v3

Updated to apply, carrying r+. Nothing really changed.
Comment 74 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-23 17:01:16 PST
Comment on attachment 576652 [details] [diff] [review]
Part 2: Update the interface and stub implementors which don't have support v2

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

::: dom/base/nsGlobalWindow.cpp
@@ +466,5 @@
> +                           void *initData, NPAsyncSurface *surface)
> +  { return NPERR_GENERIC_ERROR; }
> +
> +  NPError FinalizeAsyncSurface(NPAsyncSurface *surface) { return NPERR_GENERIC_ERROR; }
> +  void SetCurrentAsyncSurface(NPAsyncSurface *surface, NPRect *changed) { return; }

Why are these needed in nsGlobalWindow?
Comment 75 Josh Aas 2011-11-23 19:27:25 PST
Is the spec up to date? It hasn't been modified since May 2011 and I think you had more changes to make.
Comment 76 Bas Schouten (:bas.schouten) 2011-11-24 05:52:23 PST
(In reply to Josh Aas (Mozilla Corporation) from comment #75)
> Is the spec up to date? It hasn't been modified since May 2011 and I think
> you had more changes to make.

Essentially the only change will be a message when the plugin has been composited so that plugins can limit their framerate, as well as an example of this behaviour. I need to ask roc whether he prefers to send this message via an NPP function or as an event.
Comment 77 Bas Schouten (:bas.schouten) 2011-11-24 05:54:01 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #74)
> Comment on attachment 576652 [details] [diff] [review] [diff] [details] [review]
> Part 2: Update the interface and stub implementors which don't have support
> v2
> 
> Review of attachment 576652 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +466,5 @@
> > +                           void *initData, NPAsyncSurface *surface)
> > +  { return NPERR_GENERIC_ERROR; }
> > +
> > +  NPError FinalizeAsyncSurface(NPAsyncSurface *surface) { return NPERR_GENERIC_ERROR; }
> > +  void SetCurrentAsyncSurface(NPAsyncSurface *surface, NPRect *changed) { return; }
> 
> Why are these needed in nsGlobalWindow?

Because nsDummyJavaPluginOwner implements nsIPluginInstanceOwner, which implements these, I believe, just like ConvertPoint and friends are there.

I think.
Comment 78 Bas Schouten (:bas.schouten) 2011-11-24 06:47:18 PST
Created attachment 576758 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms v3

This didn't apply at all, a lot changed in this code. Merged to apply again, untested at this point, but should work.
Comment 79 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-24 12:18:11 PST
(In reply to Bas Schouten (:bas) from comment #76)
> Essentially the only change will be a message when the plugin has been
> composited so that plugins can limit their framerate, as well as an example
> of this behaviour. I need to ask roc whether he prefers to send this message
> via an NPP function or as an event.

I don't mind.

The spec also needs to be changed to note that SetCurrentSurface does not deliver messages to the plugin while it's blocking. Maybe there should be some informational text explaining that the browser is expected to execute some kind of atomic swap operation.
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-24 12:40:02 PST
Comment on attachment 576758 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms v3

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +418,5 @@
> +#ifdef XP_WIN
> +        *((NPBool*)aValue) = PluginModuleChild::current()->AsyncDrawingAllowed();
> +#else
> +        // Event handling needs to be updated to prevent synchronous paint
> +        // events from being handled on non-windows platforms.

We also won't have the cross-process mutexes we need on non-Windows, initially. I suggest you just say "not supported on Windows yet".
Comment 81 Bas Schouten (:bas.schouten) 2011-11-27 06:37:36 PST
Created attachment 577135 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v7

Updated this patch to m-c as well. Untested still though.
Comment 82 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-27 12:59:27 PST
Comment on attachment 577135 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v7

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

::: dom/plugins/ipc/PPluginInstance.ipdl
@@ +253,5 @@
>                         NPCoordinateSpace destSpace)
>      returns (double destX, double destY, bool result);
>  
> +  rpc NPN_SetCurrentAsyncSurface(NPRemoteAsyncSurface surface, NPRect changedRect)
> +    returns (bool result);

SetCurrentAsyncSurface needs to use cross-process mutexes now!
Comment 83 Josh Aas 2011-11-28 07:11:21 PST
Comment on attachment 576651 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v7

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

::: dom/plugins/base/npapi.h
@@ +464,5 @@
> +#endif
> +  , NPNVsupportsAsyncBitmapSurfaceBool = 2005
> +#if defined(XP_WIN)
> +  , NPNVsupportsAsyncWindowsDXGISurfaceBool = 2006
> +  , NPNVsupportsAsyncWindowsDX9ExSurfaceBool = 2007

We should add a support query variable for the existing win sync model.
Comment 84 Josh Aas 2011-11-28 07:13:00 PST
(In reply to Bas Schouten (:bas) from comment #81)
> Created attachment 577135 [details] [diff] [review] [diff] [details] [review]
> Part 5: Support AsyncBitmapSurface drawing model v7
> 
> Updated this patch to m-c as well. Untested still though.

I don't think anything should land until there is an implementation in the test plugin that can at least be turned on manually, for which most (if not all) tests pass.
Comment 85 Bas Schouten (:bas.schouten) 2012-02-08 11:41:29 PST
Created attachment 595473 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v8

This addresses review comments and also adds the new DidComposite function.
Comment 86 Bas Schouten (:bas.schouten) 2012-02-14 09:25:13 PST
Created attachment 597051 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process.

This allows accessing ImageContainers from a remote process.
Comment 87 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-14 15:17:51 PST
Comment on attachment 597051 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process.

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

::: gfx/layers/ImageLayers.cpp
@@ +133,5 @@
> +    NS_ASSERTION(mRemoteDataMutex, "Should have remote data mutex when having remote data!");
> +    mRemoteDataMutex->Lock();
> +
> +    if (mRemoteData->wasUpdated) {
> +      mActiveImage = NULL;

nsnull instead of NULL please

@@ +145,5 @@
> +      newImg->mSize = mRemoteData->size;
> +      newImg->mStride = mRemoteData->stride;
> +      mRemoteData->wasUpdated = false;
> +              
> +      mActiveImage = newImg;

Nothing stops someone calling GetCurrentImage to get this image, and using that image indefinitely. That's really bad.

I think GetCurrentImage needs to make a copy of the current image, if it's a remote image, and return that.

::: gfx/layers/ImageLayers.h
@@ +128,5 @@
>       */
> +    MAC_IO_SURFACE,
> +
> +    /**
> +     * An bitmap image that comes from a remote process.

Change to "that can be shared with a remote process"? It doesn't *have* to be a remote process, I assume, and other image types can come from remote processes via IPC.

@@ +247,5 @@
> +     * This is a format that uses raw bitmap data.
> +     */
> +    RAW_BITMAP
> +  };
> +  enum Format {

Document these formats!

@@ +252,5 @@
> +    BGRA32,
> +    BGRX32
> +  };
> +
> +  bool wasUpdated;

What does this mean exactly?

@@ +254,5 @@
> +  };
> +
> +  bool wasUpdated;
> +  Type type;
> +  Format format;

Follow m* naming convention please

@@ +330,5 @@
>      nsRefPtr<Image> retval = mActiveImage;
>      return retval.forget();
>    }
> +  
> +  already_AddRefed<Image> LockCurrentImage();

Why is this up here? It should go next to UnlockCurrentImage. It definitely needs a comment to explain how these should be used. It needs to be *very* clear that the Image is only valid until Unlock.

Can this just return a raw Image* pointer since presumably the current image can't change?

@@ +347,5 @@
>     * modify it.
>     * Can be called on any thread. This method takes mReentrantMonitor
>     * when accessing thread-shared state.
> +   * If the current image is a remote image, calling this function will make
> +   * a copy of the image data while holding the mRemoteDataMutex.

Somewhere you need to say what a remote image is.

You should probably say here to use one of the Lock methods if you can. But you need to explain under what circumstances it's OK to use Lock/Unlock.

@@ +354,5 @@
> +  /**
> +   * This is similar to GetCurrentAsSurface, however this does not make a copy
> +   * of the image data and requires the user to call UnlockCurrentImage when
> +   * done with the image data. Once UnlockCurrentImage has been called the
> +   * surface returned by this function is no longer valid!

Need to make clear that this works for any kind of image.

@@ +426,5 @@
>      }
>    }
>  
> +  void SetRemoteImageData(RemoteImageData *aRemoteData,
> +                          CrossProcessMutex *aRemoteDataMutex);

How do remote callers update the image data? Do they call this?

@@ +427,5 @@
>    }
>  
> +  void SetRemoteImageData(RemoteImageData *aRemoteData,
> +                          CrossProcessMutex *aRemoteDataMutex);
> +  RemoteImageData *GetRemoteImageData() { return mRemoteData; }

These need comments, especially SetRemoteImageData.

@@ +474,5 @@
> +  // image.
> +  RemoteImageData *mRemoteData;
> +
> +  // This cross-process mutex is used to synchronise access to mRemoteData.
> +  CrossProcessMutex *mRemoteDataMutex;

Need to explain how use of this mutex interacts with use of the mutex on the container.

Is it possible to have race conditions where different threads disagree about which mutex to use?

@@ +485,5 @@
> +  ~AutoUnlockImage() { if (mContainer) { mContainer->UnlockCurrentImage(); } }
> +
> +  void Unlock() { if (mContainer) { mContainer->UnlockCurrentImage(); mContainer = nsnull; } }
> +private:
> +  ImageContainer *mContainer;

How do you use this? Normally the class would handle the "enter" step as well as the "leave" step. Should it have LockCurrentAsSurface/LockCurrentImage methods?

::: gfx/layers/basic/BasicLayers.cpp
@@ +931,2 @@
>    GetContainer()->NotifyPaintedImage(image);
> +  mContainer->UnlockCurrentImage();

There's a race condition here where we notify the wrong image.

Do we need to extend the API so that we can lock the current image and get an Image and a surface in one operation? I think probably the thing to do would be to have LockCurrentAsSurface return the image as well as the surface.

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +45,5 @@
>  namespace mozilla {
>  namespace layers {
> +  
> +  static already_AddRefed<ID3D10Texture2D>
> +DataToTexture(ID3D10Device *aDevice,

Fix indent
Comment 88 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-14 15:19:24 PST
If we fix the above things, I think the ImageContainer API will be pretty good.
Comment 89 Bas Schouten (:bas.schouten) 2012-02-16 13:52:55 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #87)
> Comment on attachment 597051 [details] [diff] [review]
> Part 5: Allow accessing image containers from remote process.
> 
> Review of attachment 597051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +145,5 @@
> > +      newImg->mSize = mRemoteData->size;
> > +      newImg->mStride = mRemoteData->stride;
> > +      mRemoteData->wasUpdated = false;
> > +              
> > +      mActiveImage = newImg;
> 
> Nothing stops someone calling GetCurrentImage to get this image, and using
> that image indefinitely. That's really bad.

I want to kill GetCurrentImage if possible, how would you feel about that?

> 
> I think GetCurrentImage needs to make a copy of the current image, if it's a
> remote image, and return that.

I agree with that if we cannot kill the API.

> @@ +252,5 @@
> > +    BGRA32,
> > +    BGRX32
> > +  };
> > +
> > +  bool wasUpdated;
> 
> What does this mean exactly?

This indicates that the remote process has changed the data that is pointed to, that allows us to re-use RemoteBitmapImage (and associated cached textures) for as long as the image didn't change. I'll document this.

> Can this just return a raw Image* pointer since presumably the current image
> can't change?

Hrm, right now it can I suppose, in theory, I should hold the reentrant monitor while the lock is active as well, probably.

> @@ +426,5 @@
> >      }
> >    }
> >  
> > +  void SetRemoteImageData(RemoteImageData *aRemoteData,
> > +                          CrossProcessMutex *aRemoteDataMutex);
> 
> How do remote callers update the image data? Do they call this?

No, this is called locally by the creator of the container. Because we can only create Shmem from within our IPC protocol code. This simply tells the container where it can find the data and how to synchronize. Right now the remote process will manually manipulate this structure, if you prefer I could create a wrapper class like
class RemoteContainerInterface {
  RemoteContainerInterface(RemoteImageData *aData);
etc.
};

and use that to set it, but I didn't see much of an advantage at this point.

> @@ +474,5 @@
> > +  // image.
> > +  RemoteImageData *mRemoteData;
> > +
> > +  // This cross-process mutex is used to synchronise access to mRemoteData.
> > +  CrossProcessMutex *mRemoteDataMutex;
> 
> Need to explain how use of this mutex interacts with use of the mutex on the
> container.
> 
> Is it possible to have race conditions where different threads disagree
> about which mutex to use?

So right now I'm thinking about this, and although there's no real deadlock risk, I think we should hold the re-entrant monitor when the current image is locked to prevent any confusion.

> @@ +485,5 @@
> > +  ~AutoUnlockImage() { if (mContainer) { mContainer->UnlockCurrentImage(); } }
> > +
> > +  void Unlock() { if (mContainer) { mContainer->UnlockCurrentImage(); mContainer = nsnull; } }
> > +private:
> > +  ImageContainer *mContainer;
> 
> How do you use this? Normally the class would handle the "enter" step as
> well as the "leave" step. Should it have
> LockCurrentAsSurface/LockCurrentImage methods?

Currently the way we use it is like this.

nsRefPtr<Image> image = container->LockCurrentImage();

AutoUnlockImage autoUnlock(container);

<do stuff>
if (error) {
  return;
}
<do more stuff, finish uploading>
autoUnlock.Unlock();

I could remove unlock and just add a scope around AutoUnlockImage but I hate indentations :P

> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +931,2 @@
> >    GetContainer()->NotifyPaintedImage(image);
> > +  mContainer->UnlockCurrentImage();
> 
> There's a race condition here where we notify the wrong image.
> 
> Do we need to extend the API so that we can lock the current image and get
> an Image and a surface in one operation? I think probably the thing to do
> would be to have LockCurrentAsSurface return the image as well as the
> surface.

This could also be done by having LockCurrentImage grab the re-entrant monitor?
Comment 90 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-16 14:55:42 PST
(In reply to Bas Schouten (:bas) from comment #89)
> I want to kill GetCurrentImage if possible, how would you feel about that?

Sure.

> > How do remote callers update the image data? Do they call this?
> 
> No, this is called locally by the creator of the container. Because we can
> only create Shmem from within our IPC protocol code. This simply tells the
> container where it can find the data and how to synchronize. Right now the
> remote process will manually manipulate this structure, if you prefer I
> could create a wrapper class like
> class RemoteContainerInterface {
>   RemoteContainerInterface(RemoteImageData *aData);
> etc.
> };
> 
> and use that to set it, but I didn't see much of an advantage at this point.

OK. Just document this in the code.

> So right now I'm thinking about this, and although there's no real deadlock
> risk, I think we should hold the re-entrant monitor when the current image
> is locked to prevent any confusion.

Good.

If SetRemoteImageData should only be called (once) while setting up the container, then we shouldn't have any problems, as long as that's documented. You should probably add an assertion that there's no current image, and SetRemoteImageData hasn't already been called.

> Currently the way we use it is like this.
> 
> nsRefPtr<Image> image = container->LockCurrentImage();
> 
> AutoUnlockImage autoUnlock(container);
> 
> <do stuff>
> if (error) {
>   return;
> }
> <do more stuff, finish uploading>
> autoUnlock.Unlock();
> 
> I could remove unlock and just add a scope around AutoUnlockImage but I hate
> indentations :P

I actually think you should remove Unlock, and make AutoUnlockImage be responsible for doing the Lock as well. Maybe call it AutoLockImage.

How about
  class AutoLockImage {
  public:
    AutoLockImage(ImageContainer* aContainer);
    AutoLockImage(ImageContainer* aContainer, gfxASurface** aSurface);
    Image* GetImage();
    ~AutoLockImage();
  };
?
Comment 91 Bas Schouten (:bas.schouten) 2012-02-16 15:21:11 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #90)
> (In reply to Bas Schouten (:bas) from comment #89)
> If SetRemoteImageData should only be called (once) while setting up the
> container, then we shouldn't have any problems, as long as that's
> documented. You should probably add an assertion that there's no current
> image, and SetRemoteImageData hasn't already been called.

It's actually okay for there to be a current image. As any changes to the remote image data make 'mWasUpdated' true, and cause the ActiveImage to change, so it's ok' for SetRemoteImageData to be called later (although it's not the usage pattern I'd expect).

With my latest patch, SetCurrentImage can also still be used and behave as expected from the same process. Although I'm not 100% sure if there's value in that, it seemed to ensure the container always would behave as expected.
Comment 92 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-16 15:29:08 PST
(In reply to Bas Schouten (:bas) from comment #91)
> It's actually okay for there to be a current image. As any changes to the
> remote image data make 'mWasUpdated' true, and cause the ActiveImage to
> change, so it's ok' for SetRemoteImageData to be called later (although it's
> not the usage pattern I'd expect).

Then let's rule it out for now. It's simpler to reason about the code if SetRemoteImageData is called before anyone else gets their hands on the ImageContainer.

> With my latest patch, SetCurrentImage can also still be used and behave as
> expected from the same process. Although I'm not 100% sure if there's value
> in that, it seemed to ensure the container always would behave as expected.

Having SetCurrentImage work after SetRemoteImageData has been called seems OK to me but I don't really mind either way.
Comment 93 Bas Schouten (:bas.schouten) 2012-02-16 15:42:49 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #90)
> (In reply to Bas Schouten (:bas) from comment #89)
> How about
>   class AutoLockImage {
>   public:
>     AutoLockImage(ImageContainer* aContainer);
>     AutoLockImage(ImageContainer* aContainer, gfxASurface** aSurface);
>     Image* GetImage();
>     ~AutoLockImage();
>   };
> ?

Hrm, so I've been playing with this, and removing unlock is hard, since I want to unlock in a nested scope depth because I want to unlock after uploading the textures (before issuing actual draw calls that can take time).

Also as LockCurrentAsSurface might return size, Image and Surface, it would become something like this:

  class AutoLockImage {
  public:
    AutoLockImage(ImageContainer* aContainer);
    AutoLockImage(ImageContainer* aContainer, gfxASurface** aSurface, gfxIntSize &size);
    void Unlock();
    Image* GetImage();
    ~AutoLockImage();
  };

Which isn't too pretty, but if you prefer that's fine? An alternative would be:

  class AutoLockImage {
  public:
    AutoLockImage(ImageContainer* aContainer, Image** aImage);
    AutoLockImage(ImageContainer* aContainer, gfxASurface** aSurface, gfxIntSize &size, Image** aImage);
    void Unlock();
    ~AutoLockImage();
  };
Comment 94 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-16 15:47:39 PST
Multiple getter methods are preferable to out params, in my opinion.

The only reason I made aSurface an out-param is to give you a way to overload the constructor so the right Lock method is called (since locking to get a surface is more expensive).
Comment 95 Bas Schouten (:bas.schouten) 2012-02-16 18:01:19 PST
Created attachment 598086 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v2
Comment 96 Bas Schouten (:bas.schouten) 2012-02-16 18:03:24 PST
Created attachment 598087 [details] [diff] [review]
Part 1: Add types and functions to npapi.h and npfunctions.h v9

Updated as ISO C++ forbids anonymous structs, generating a compiler warning seems undesirable in C++ files.
Comment 97 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-16 18:19:21 PST
Comment on attachment 598086 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v2

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

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +210,5 @@
>      // a new image.  See nsIPluginInstance.idl.
>      mInstance->GetImageContainer(getter_AddRefs(container));
>      if (container) {
>  #ifdef XP_MACOSX
> +      nsRefPtr<Image> image = container->LockCurrentImage();

Use AutoLockImage

@@ +3669,5 @@
>      // Drop image reference because the child may destroy the surface after we return.
>      nsRefPtr<ImageContainer> container = mObjectFrame->GetImageContainer();
>      if (container) {
>  #ifdef XP_MACOSX
> +      nsRefPtr<Image> image = container->LockCurrentImage();

Use AutoLockImage

::: gfx/layers/ImageLayers.cpp
@@ +152,5 @@
> +}
> +
> +already_AddRefed<Image>
> +ImageContainer::LockCurrentImage()
> +{

Why do we need to return already_AddRefed instead of just a raw pointer? The current image shouldn't change while we're locked.

@@ +213,5 @@
> +  } else if (mActiveImage->GetFormat() == Image::CAIRO_SURFACE) {
> +    CairoImage *cairoImage =
> +      static_cast<CairoImage*>(mActiveImage.get());
> +    *aSize = cairoImage->mSize;
> +    return cairoImage->GetAsSurface();

Why don't you just call Image::GetSize() and GetAsSurface directly?

@@ +230,5 @@
>  }
>  
>  already_AddRefed<gfxASurface>
>  ImageContainer::GetCurrentAsSurface(gfxIntSize *aSize)
>  {

Why not just reimplement this to call LockCurrentAsSurface/Unlock internally? Much simpler.

::: gfx/layers/ImageLayers.h
@@ +239,5 @@
> + 
> +/**
> + * This struct is used to store RemoteImages, it is meant to be able to live in
> + * shared memory. Therefor it should not contain a vtable pointer. Remote
> + * users can manipulate the datain this structure to specify what image is to

data in

@@ +452,5 @@
> +  /**
> +   * This function is called to tell the ImageContainer where the
> +   * (cross-process) segment lives where the shared data about possible
> +   * remote images are stored. In addition to this a CrossProcessMutex object
> +   * is passed telling the container how to synchronize access to this data. */

Document that this should be called when setting up the container.

@@ +526,5 @@
> +  }
> +  ~AutoLockImage() { if (mContainer) { mContainer->UnlockCurrentImage(); } }
> +
> +  Image* GetImage() { return mImage; }
> +  gfxIntSize &GetSize() { return mSize; }

const gfxIntSize&

@@ +528,5 @@
> +
> +  Image* GetImage() { return mImage; }
> +  gfxIntSize &GetSize() { return mSize; }
> +
> +  void Unlock() { if (mContainer) { mImage = nsnull; mContainer->UnlockCurrentImage(); mContainer = nsnull; } }

Split across multiple lines

@@ +806,5 @@
> +class RemoteBitmapImage : public Image {
> +public:
> +  RemoteBitmapImage() : Image(NULL, REMOTE_IMAGE_BITMAP) {}
> +
> +  already_AddRefed<gfxASurface> GetAsSurface() { MOZ_ASSERT(false); return nsnull; }

Why can't we return a surface here? Seems like we should.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +212,5 @@
>  
>  void
>  ImageLayerOGL::RenderLayer(int,
>                             const nsIntPoint& aOffset)
>  {

Add an assertion somewhere that RemoteBitmap images aren't handled yet.
Comment 98 Bas Schouten (:bas.schouten) 2012-02-16 23:19:49 PST
Created attachment 598156 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v3

Updated.
Comment 99 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-18 02:58:34 PST
Comment on attachment 598156 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v3

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

::: gfx/layers/ImageLayers.cpp
@@ +192,5 @@
> +      *aSize = newSurf->GetSize();
> +    
> +      return newSurf.forget();
> +    } else {
> +      return nsnull;

So if we have mRemoteData, but someone did SetCurrentImage, we'll always return a surface for the remote data if there is some otherwise we'll ignore the current image? This seems wrong.

Seems like we should check if mActiveImage is the remote image; if not, drop out to the !mRemoteData path.

::: gfx/layers/ImageLayers.h
@@ +453,5 @@
> +  /**
> +   * This function is called to tell the ImageContainer where the
> +   * (cross-process) segment lives where the shared data about possible
> +   * remote images are stored. In addition to this a CrossProcessMutex object
> +   * is passed telling the container how to synchronize access to this data. */

Document that this should only be called during ImageContainer setup and not after we actually start using it.
Comment 100 Bas Schouten (:bas.schouten) 2012-02-18 08:04:49 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #99)
> Comment on attachment 598156 [details] [diff] [review]
> Part 5: Allow accessing image containers from remote process. v3
> 
> Review of attachment 598156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageLayers.cpp
> @@ +192,5 @@
> > +      *aSize = newSurf->GetSize();
> > +    
> > +      return newSurf.forget();
> > +    } else {
> > +      return nsnull;
> 
> So if we have mRemoteData, but someone did SetCurrentImage, we'll always
> return a surface for the remote data if there is some otherwise we'll ignore
> the current image? This seems wrong.
> 
> Seems like we should check if mActiveImage is the remote image; if not, drop
> out to the !mRemoteData path.

You're absolutely right.

> ::: gfx/layers/ImageLayers.h
> @@ +453,5 @@
> > +  /**
> > +   * This function is called to tell the ImageContainer where the
> > +   * (cross-process) segment lives where the shared data about possible
> > +   * remote images are stored. In addition to this a CrossProcessMutex object
> > +   * is passed telling the container how to synchronize access to this data. */
> 
> Document that this should only be called during ImageContainer setup and not
> after we actually start using it.

Hrm, right now that's not actually what happens in my code, it can be cleared and re-instated later. I can have the plugin recreate its container though when it switches drawing models so I guess it isn't a big issue :).
Comment 101 Bas Schouten (:bas.schouten) 2012-02-18 08:07:33 PST
Created attachment 598538 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v4

Adjusted.
Comment 102 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-18 11:44:40 PST
Comment on attachment 598538 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v4

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

::: gfx/layers/ImageLayers.cpp
@@ +181,5 @@
> +      NS_IF_ADDREF(mActiveImage);
> +      *aCurrentImage = mActiveImage.get();
> +    }
> +
> +    if (mRemoteData->mType == RemoteImageData::RAW_BITMAP) {

Seems like nothing has changed here? If mActiveImage is not the remote image, because someone called SetCurrentImage, then this check will still pass and we'll still return a different surface.
Comment 103 Bas Schouten (:bas.schouten) 2012-02-18 21:28:18 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #102)
> Comment on attachment 598538 [details] [diff] [review]
> Part 5: Allow accessing image containers from remote process. v4
> 
> Review of attachment 598538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageLayers.cpp
> @@ +181,5 @@
> > +      NS_IF_ADDREF(mActiveImage);
> > +      *aCurrentImage = mActiveImage.get();
> > +    }
> > +
> > +    if (mRemoteData->mType == RemoteImageData::RAW_BITMAP) {
> 
> Seems like nothing has changed here? If mActiveImage is not the remote
> image, because someone called SetCurrentImage, then this check will still
> pass and we'll still return a different surface.

Hrm, yeah, fair point. I removed the else clause but didn't adjust this.
Comment 104 Bas Schouten (:bas.schouten) 2012-02-19 08:10:43 PST
Created attachment 598663 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v5

This should give the desired behavior.
Comment 105 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-19 13:04:05 PST
Comment on attachment 598663 [details] [diff] [review]
Part 5: Allow accessing image containers from remote process. v5

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

Great, thanks!
Comment 106 Bas Schouten (:bas.schouten) 2012-02-21 20:32:36 PST
Created attachment 599464 [details] [diff] [review]
Part 6: Implement the sync bitmap drawing model v5
Comment 107 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-25 03:42:55 PST
You don't seem to have addressed the issues in comment #9?
Comment 108 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-25 05:19:56 PST
Comment on attachment 599464 [details] [diff] [review]
Part 6: Implement the sync bitmap drawing model v5

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +538,5 @@
>          NPError rv;
>          int drawingModel = (int16) (intptr_t) aValue;
>  
>          if (!PluginModuleChild::current()->AsyncDrawingAllowed()) {
> +            if (IsDrawingModelAsync(drawingModel)) {

Use && instead of a second if

@@ +2404,5 @@
> +
> +            NS_ABORT_IF_FALSE(remote.data().get_Shmem().IsWritable(),
> +                "Failed to create writable shared memory.");
> +            
> +            // XXX - Check stuff.

What are you planning to do here?

@@ +2471,5 @@
> +    if (!surface) {
> +      CrossProcessMutexAutoLock autoLock(*mRemoteImageDataMutex);
> +      data->mBitmap.mData = NULL;
> +      data->mSize = gfxIntSize(0, 0);
> +      data->mWasUpdated = true;

Indentation is all messed up in this function

@@ +2513,5 @@
> +{
> +  {
> +    MutexAutoLock autoLock(mAsyncInvalidateMutex);
> +    mAsyncInvalidateTask = NULL;
> +  }

Fix indent

::: dom/plugins/ipc/PluginInstanceChild.h
@@ +385,5 @@
>      int16_t               mDrawingModel;
> +    NPAsyncSurface* mCurrentAsyncSurface;
> +    struct AsyncBitmapData {
> +      void *remotePtr;
> +      Shmem shmem;

mRemotePtr, mShMem

@@ +388,5 @@
> +      void *remotePtr;
> +      Shmem shmem;
> +    };
> +
> +    std::map<NPAsyncSurface*, AsyncBitmapData> mAsyncBitmaps;

Can we use nsBaseHashtable here instead of std::map?

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +126,5 @@
> +        ImageContainer *container =
> +            GetImageContainer();
> +
> +        if (container) {
> +            container->SetRemoteImageData(NULL, NULL);

nsnull

@@ +441,5 @@
> +
> +        if (mRemoteImageDataShmem.IsWritable()) {
> +            container->SetRemoteImageData(NULL, NULL);
> +            DeallocShmem(mRemoteImageDataShmem);
> +            mRemoteImageDataMutex = NULL;

nsnull

Is changing the drawing model away from an async model actually supposed to be supported? Is it tested?

::: dom/plugins/ipc/PluginMessageUtils.h
@@ +42,5 @@
>  #include "IPC/IPCMessageUtils.h"
>  #include "base/message_loop.h"
>  
>  #include "mozilla/ipc/RPCChannel.h"
> +#include "mozilla/ipc/CrossProcessMutex.h"

Why is this #include needed here?
Comment 109 Bas Schouten (:bas.schouten) 2012-02-25 09:39:45 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #108)
> Comment on attachment 599464 [details] [diff] [review]
> Part 6: Implement the sync bitmap drawing model v5
> 
> @@ +388,5 @@
> > +      void *remotePtr;
> > +      Shmem shmem;
> > +    };
> > +
> > +    std::map<NPAsyncSurface*, AsyncBitmapData> mAsyncBitmaps;
> 
> Can we use nsBaseHashtable here instead of std::map?

I'd rather use a btree here like stl::map. And I'd rather not use ns<stllike> stuff as in my tests the STL stuff has better perf characteristics, but we could, if we really wanted to.

> @@ +441,5 @@
> > +
> > +        if (mRemoteImageDataShmem.IsWritable()) {
> > +            container->SetRemoteImageData(NULL, NULL);
> > +            DeallocShmem(mRemoteImageDataShmem);
> > +            mRemoteImageDataMutex = NULL;
> 
> nsnull
> 
> Is changing the drawing model away from an async model actually supposed to
> be supported? Is it tested?

I thought it was, it seems to be working I didn't do 'extensive' testing though. I could ask the folks at Adobe how they feel about this.

> 
> ::: dom/plugins/ipc/PluginMessageUtils.h
> @@ +42,5 @@
> >  #include "IPC/IPCMessageUtils.h"
> >  #include "base/message_loop.h"
> >  
> >  #include "mozilla/ipc/RPCChannel.h"
> > +#include "mozilla/ipc/CrossProcessMutex.h"
> 
> Why is this #include needed here?

To make sure IPDL knows CrossProcessMutexHandle when it compiles the autogenerated PPluginInstanceParent.cpp and PPluginInstanceChild.

I'll address comment #9 as well.
Comment 110 Bas Schouten (:bas.schouten) 2012-02-25 09:57:03 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #9)
> Comment on attachment 528420 [details] [diff] [review]
> Part 6: Support AsyncBitmapSurface drawing model
> 
> Review of attachment 528420 [details] [diff] [review]:
> ::: dom/plugins/ipc/PluginInstanceParent.cpp
> @@ +1060,5 @@
> +            if (npevent->event == WM_PAINT || npevent->event ==
> DoublePassRenderingEvent()) {
> +                // This plugin maintains its own async drawing.
> +                return handled;
> +            }
> +        }
> 
> Don't we need this change for other platforms too?

This is correct, however I'm not sure how exactly to do this there, and since this model doesn't work on other platforms yet, I suggest we cover this there when we implement/test the model there. Right now I wouldn't know if it behaved correctly anyway.
Comment 111 Bas Schouten (:bas.schouten) 2012-02-25 09:58:32 PST
Created attachment 600685 [details] [diff] [review]
Part 6: Implement the sync bitmap drawing model v6

Updated for review comments (except the ones replied upon).
Comment 112 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-25 10:21:14 PST
(In reply to Bas Schouten (:bas) from comment #109)
> I'd rather use a btree here like stl::map. And I'd rather not use
> ns<stllike> stuff as in my tests the STL stuff has better perf
> characteristics, but we could, if we really wanted to.

Surely a hashtable has better performance here?

We're not using STL in this code yet. I don't think we should start here.

> > Don't we need this change for other platforms too?
> 
> This is correct, however I'm not sure how exactly to do this there, and
> since this model doesn't work on other platforms yet, I suggest we cover
> this there when we implement/test the model there. Right now I wouldn't know
> if it behaved correctly anyway.

What do we have to ensure that people don't try to use the async bitmap model on non-Windows?
Comment 113 Bas Schouten (:bas.schouten) 2012-02-28 10:15:57 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #112)
> (In reply to Bas Schouten (:bas) from comment #109)
> > > Don't we need this change for other platforms too?
> > 
> > This is correct, however I'm not sure how exactly to do this there, and
> > since this model doesn't work on other platforms yet, I suggest we cover
> > this there when we implement/test the model there. Right now I wouldn't know
> > if it behaved correctly anyway.
> 
> What do we have to ensure that people don't try to use the async bitmap
> model on non-Windows?

NPNVsupportsAsyncBitmapSurfaceBool will report false and trying to set the model will fail (as it won't be preffed on on those platforms).

This is also important as we don't have an implementation of cross-process mutices there.
Comment 114 Bas Schouten (:bas.schouten) 2012-02-28 21:24:34 PST
Created attachment 601511 [details] [diff] [review]
Part 6: Implement the sync bitmap drawing model v7
Comment 115 Bas Schouten (:bas.schouten) 2012-02-28 21:25:29 PST
Created attachment 601514 [details] [diff] [review]
Part 7: Implement NPP_DidComposite API.
Comment 116 Bas Schouten (:bas.schouten) 2012-02-28 21:26:27 PST
Created attachment 601515 [details] [diff] [review]
Part 8: Add test code to test plugin

Shouldn't have changed much from the old stuff!
Comment 117 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-29 00:55:02 PST
Comment on attachment 601511 [details] [diff] [review]
Part 6: Implement the sync bitmap drawing model v7

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2357,5 @@
> +        return NPERR_INVALID_PARAM;
> +    }
> +
> +    DeallocShmem(data->mShmem);
> +    aSurface->bitmap.data = NULL;

nsnull
Comment 118 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-29 01:01:32 PST
Comment on attachment 601515 [details] [diff] [review]
Part 8: Add test code to test plugin

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

r+ with that fixed.

::: dom/plugins/test/testplugin/nptest.cpp
@@ +553,5 @@
> +  memcpy(subpixels, &rgba, sizeof(subpixels));
> +
> +  subpixels[0] = PRUint8(float(subpixels[3] * subpixels[0]) / 0xFF);
> +  subpixels[1] = PRUint8(float(subpixels[3] * subpixels[1]) / 0xFF);
> +  subpixels[2] = PRUint8(float(subpixels[3] * subpixels[2]) / 0xFF);

This assumes little-endian. You can rewrite this to not use byte indices so it'll be endian-independent.

@@ +560,5 @@
> +
> +  for (PRUint32* lastPixel = pixelData + instanceData->backBuffer->size.width * instanceData->backBuffer->size.height;
> +	pixelData < lastPixel;
> +	++pixelData)
> +    *pixelData = premultiplied;

{}
Comment 122 Virtual_ManPL [:Virtual] - (ni? me) 2015-05-08 07:12:46 PDT
Question:

Are we going to enable it by default, same as bug #1116806?
or we're going to remove it in bug #1072528?
Comment 123 Bas Schouten (:bas.schouten) 2015-05-08 12:46:51 PDT
(In reply to Virtual_ManPL [:Virtual] from comment #122)
> Question:
> 
> Are we going to enable it by default, same as bug #1116806?
> or we're going to remove it in bug #1072528?

We're removing it.

Note You need to log in before you can comment on or make changes to this bug.