Bas Schouten (:bas.schouten) Assignee
	Description • 13 years ago

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

	Bas Schouten (:bas.schouten) Assignee
	Comment 1 • 13 years ago

This is a very first attempt to add the types and functions to npapi.h, requesting feedback here to see if this looks alright.

	Bas Schouten (:bas.schouten) Assignee
	Comment 2 • 13 years ago

Fix #ifdef inbalance and add stuff to npfunctions.h

	Benjamin Smedberg
	Comment 5 • 13 years ago

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.

	Josh Aas
	Comment 6 • 13 years ago

We are not done working out the spec, Bas is just implementing early as a check on our design decisions.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 13 years ago

Comment on attachment 528418 [details] [diff] [review]
Part 4: Allow setting different drawing models across all platforms

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 13 years ago

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?)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 13 years ago

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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 10 • 13 years ago

Updated to the latest API spec.

	Bas Schouten (:bas.schouten) Assignee
	Comment 13 • 13 years ago

Updated to remove some dead code in the patch.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 14 • 13 years ago

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.)

	Bas Schouten (:bas.schouten) Assignee
	Comment 16 • 13 years ago

Added an updated description of the pixel layout in memory, as requested by roc in previous comments.

	Bas Schouten (:bas.schouten) Assignee
	Comment 18 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 19 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 20 • 13 years ago

Updated again to correct a typo.

	Bas Schouten (:bas.schouten) Assignee
	Comment 22 • 13 years ago

This allows the test plugin to do async bitmap drawing for solid color tests.

	Bas Schouten (:bas.schouten) Assignee
	Comment 23 • 13 years ago

Updated to correct for updated constant.

	Bas Schouten (:bas.schouten) Assignee
	Comment 24 • 13 years ago

This adds reftests that test if Async bitmap drawing is rendering sanely, and if it's updating properly.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 13 years ago

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]:
-----------------------------------------------------------------

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 28 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 29 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 13 years ago

Comment on attachment 531480 [details] [diff] [review]
Part 7: Add reftest for async bitmap drawing

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 31 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 32 • 13 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 33 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 34 • 13 years ago

Updated per IRC discussions and review comments.

	Bas Schouten (:bas.schouten) Assignee
	Comment 35 • 13 years ago

Updated to reflect new function signatures.

	Bas Schouten (:bas.schouten) Assignee
	Comment 36 • 13 years ago

Updated to work with a pref.

	Bas Schouten (:bas.schouten) Assignee
	Comment 37 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 38 • 13 years ago

Comment on attachment 531843 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v2

Requesting cjones to also sanity check my threading code.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 39 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 40 • 13 years ago

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]:
-----------------------------------------------------------------

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 41 • 13 years ago

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;"

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 42 • 13 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 43 • 13 years ago

I was wondering whether we should have one pref per async surface format. I don't feel strongly about it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 44 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 45 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 46 • 13 years ago

I guess I'm looking for a new Part 6 patch?

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 47 • 13 years ago

(In reply to comment #44)
> We're doing a blocking RPC while holding our mutex? That sounds bad.

Yeah that's asking for trouble.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 48 • 13 years ago

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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 49 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 50 • 13 years ago

> 
> 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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 51 • 13 years ago

Updated to address the majority of review comments.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 52 • 13 years ago

(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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 53 • 13 years ago

(In reply to comment #52)
>   CancelableTask *mTask;

Nit: "CancelableTask*" ;).  (Copied and pasted that, whups.)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 54 • 13 years ago

(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'.

	Bas Schouten (:bas.schouten) Assignee
	Comment 55 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 56 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 57 • 13 years ago

This fixes a minor issue in the event logic of the last patch that I found when running the multi-threaded testing plugin.

	Bas Schouten (:bas.schouten) Assignee
	Comment 58 • 13 years ago

This patch also allows selecting off-main-thread drawing on supported platforms (windows only in this patch).

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 59 • 13 years ago

(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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 60 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 61 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 62 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 63 • 13 years ago

Updated patch without support for any off-main-thread functionality.

	Bas Schouten (:bas.schouten) Assignee
	Comment 64 • 13 years ago

Fixed up version of plugin test code patch without multithreading support.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 65 • 13 years ago

Comment on attachment 533038 [details] [diff] [review]
Part 5: Support AsyncBitmapSurface drawing model v6

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 66 • 13 years ago

Part 6 v4 is still using PRInt32 to write pixels to the buffer, but we need to use memcpy of a byte array.

	Bas Schouten (:bas.schouten) Assignee
	Comment 67 • 13 years ago

Upload the right v4 patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 68 • 13 years ago

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.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 69 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 70 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 71 • 13 years ago

Updated patch to compile against current m-c. In light of re-emphasizing NPAsync model.

	Bas Schouten (:bas.schouten) Assignee
	Comment 72 • 13 years ago

Updated to compile, nothing major changed, but some stuff moved around so re-requesting review.

	Bas Schouten (:bas.schouten) Assignee
	Comment 73 • 13 years ago

Updated to apply, carrying r+. Nothing really changed.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 74 • 13 years ago

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?

	Josh Aas
	Comment 75 • 13 years ago

Is the spec up to date? It hasn't been modified since May 2011 and I think you had more changes to make.

	Bas Schouten (:bas.schouten) Assignee
	Comment 76 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 77 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 78 • 13 years ago

This didn't apply at all, a lot changed in this code. Merged to apply again, untested at this point, but should work.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 79 • 13 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 80 • 13 years ago

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".

	Bas Schouten (:bas.schouten) Assignee
	Comment 81 • 13 years ago

Updated this patch to m-c as well. Untested still though.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 82 • 13 years ago

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!

	Josh Aas
	Comment 83 • 13 years ago

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.

	Josh Aas
	Comment 84 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 85 • 13 years ago

This addresses review comments and also adds the new DidComposite function.

	Bas Schouten (:bas.schouten) Assignee
	Comment 86 • 13 years ago

This allows accessing ImageContainers from a remote process.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 87 • 13 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 88 • 13 years ago

If we fix the above things, I think the ImageContainer API will be pretty good.

	Bas Schouten (:bas.schouten) Assignee
	Comment 89 • 13 years ago

(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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 90 • 13 years ago

(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();
  };
?

	Bas Schouten (:bas.schouten) Assignee
	Comment 91 • 13 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 92 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 93 • 13 years ago

(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();
  };

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 94 • 13 years ago

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).

	Bas Schouten (:bas.schouten) Assignee
	Comment 96 • 13 years ago

Updated as ISO C++ forbids anonymous structs, generating a compiler warning seems undesirable in C++ files.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 97 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 98 • 13 years ago

Updated.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 99 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 100 • 13 years ago

(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 :).

	Bas Schouten (:bas.schouten) Assignee
	Comment 101 • 13 years ago

Adjusted.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 102 • 13 years ago

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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 103 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 104 • 13 years ago

This should give the desired behavior.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 105 • 13 years ago

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!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 107 • 13 years ago

You don't seem to have addressed the issues in comment #9?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 108 • 13 years ago

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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 109 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 110 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 111 • 13 years ago

Updated for review comments (except the ones replied upon).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 112 • 13 years ago

(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?

	Bas Schouten (:bas.schouten) Assignee
	Comment 113 • 13 years ago

(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.

	Bas Schouten (:bas.schouten) Assignee
	Comment 116 • 13 years ago

Shouldn't have changed much from the old stuff!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 117 • 13 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 118 • 13 years ago

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;

{}

	Girish Sharma [:Optimizer]
	Comment 119 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/b2de8767a482
http://hg.mozilla.org/integration/mozilla-inbound/rev/445cb6ba6d29
http://hg.mozilla.org/integration/mozilla-inbound/rev/add138dc3cb0
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce22cc5a848b
http://hg.mozilla.org/integration/mozilla-inbound/rev/cba4fa01e60f
http://hg.mozilla.org/integration/mozilla-inbound/rev/9e98ff019305
http://hg.mozilla.org/integration/mozilla-inbound/rev/54a134419d1b
http://hg.mozilla.org/integration/mozilla-inbound/rev/7ba25b39e2b3
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb9ce1b15809

Since now it has landed on inbound, Are there any current plugins (like flash player etc) that utilizes this feature ?
Or Firefox is going to force the existing ones to be async ?

	Marco Bonardo [:mak]
	Comment 120 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/b2de8767a482
http://hg.mozilla.org/mozilla-central/rev/445cb6ba6d29
http://hg.mozilla.org/mozilla-central/rev/add138dc3cb0
http://hg.mozilla.org/mozilla-central/rev/ce22cc5a848b
http://hg.mozilla.org/mozilla-central/rev/cba4fa01e60f
http://hg.mozilla.org/mozilla-central/rev/9e98ff019305
http://hg.mozilla.org/mozilla-central/rev/54a134419d1b
http://hg.mozilla.org/mozilla-central/rev/7ba25b39e2b3
http://hg.mozilla.org/mozilla-central/rev/bb9ce1b15809

	Marco Bonardo [:mak]
	Comment 121 • 13 years ago

and follow-ups
https://hg.mozilla.org/mozilla-central/rev/931292ee1a69
https://hg.mozilla.org/mozilla-central/rev/f50254fe8912
https://hg.mozilla.org/mozilla-central/rev/04d22a971056

	Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)
	Comment 122 • 9 years ago

Question:

Are we going to enable it by default, same as bug #1116806?
or we're going to remove it in bug #1072528?

	Bas Schouten (:bas.schouten) Assignee
	Comment 123 • 9 years ago

(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.