The test renders fine in the browser, however, ref test failure returns an opaque rectangle when running automated tests. (attached)

Changes: Moved the double pass logic into the child process. the original patch tried to keep this in parent, but it turned out in certain 'rare' cases we must do a double render to two different surfaces. 95% of the time we render directly to the window's hdc, in which case we don't need the special treatment. But in cases such as drawWindow w/a transparent plugin, we need to jump through extra hoops. In these cases we render twice on the first paint event then we copy back the second (cached) render on the second event. Also fixed an issue with ipc wm_paint handle event messages (NPRect's should have been RECTs). Two things I have left to do - optimize the cached buffer which is currently being created/destroyed on every render, and add a test or two to better cover these cases.

The WM_PAINT used a NPRect because that's what the documentation at https://developer.mozilla.org/en/Gecko_Plugin_API_Reference/Drawing_and_Event_Handling says. Should the documentation be fixed too?

(In reply to comment #3) > The WM_PAINT used a NPRect because that's what the documentation at > https://developer.mozilla.org/en/Gecko_Plugin_API_Reference/Drawing_and_Event_Handling > says. Should the documentation be fixed too? Maybe it should be an NPRect? Doing a little searching, I'm finding conflicting docs. My assumption was that if this is supposed to mimic a windows paint event, the parameters should match. I'll see if I can track down the answer.

So nsObjectFrame sends a RECT, so I'd guess this is what plugins expect - http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#4745

Comment on attachment 416957 [details] [diff] [review] ipc plugin alpha extraction patch v.1 >diff --git a/dom/plugins/NPEventWindows.h b/dom/plugins/NPEventWindows.h >--- a/dom/plugins/NPEventWindows.h >+++ b/dom/plugins/NPEventWindows.h >@@ -51,7 +51,7 @@ struct NPRemoteEvent > { > NPEvent event; > union { >- NPRect rect; >+ RECT rect; > WINDOWPOS windowpos; > } lParamData; > }; >@@ -87,9 +87,9 @@ struct ParamTraits<mozilla::plugins::NPR > paramCopy.lParamData.windowpos = *(reinterpret_cast<WINDOWPOS*>(paramCopy.event.lParam)); > break; > case WM_PAINT: >- // The lParam paramter of WM_PAINT holds a pointer to an NPRect >+ // The lParam paramter of WM_PAINT holds a pointer to an RECT > // structure specifying the bounding box of the update area. >- paramCopy.lParamData.rect = *(reinterpret_cast<NPRect*>(paramCopy.event.lParam)); >+ paramCopy.lParamData.rect = *(reinterpret_cast<RECT*>(paramCopy.event.lParam)); > break; > > // the white list of events that we will ipc to the client >@@ -129,6 +129,9 @@ struct ParamTraits<mozilla::plugins::NPR > > // ignore any events we don't expect the 'ignore' comment should move above the 'return' statement so that it still makes sense. > default: >+ // RegisterWindowMessage events should be passed. >+ if (paramCopy.event.event >= 0xC000 && paramCopy.event.event <= 0xFFFF) >+ break; > return; > } >diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp >--- a/dom/plugins/PluginInstanceChild.cpp >+++ b/dom/plugins/PluginInstanceChild.cpp >@@ -59,6 +59,7 @@ using mozilla::gfx::SharedDIB; > > #include <windows.h> > >+#define NS_OOPP_DOUBLEPASS_MSGID TEXT("MozDoublePassMsg") > #endif > > PluginInstanceChild::PluginInstanceChild(const NPPluginFuncs* aPluginIface) : >@@ -67,6 +68,10 @@ PluginInstanceChild::PluginInstanceChild > , mPluginWindowHWND(0) > , mPluginWndProc(0) > , mPluginParentHWND(0) >+ , mDoublePass(RENDER_NATIVE) >+ , mHdc(NULL) >+ , mBmp(NULL) >+ , mOldObj(NULL) > #endif > { > memset(&mWindow, 0, sizeof(mWindow)); >@@ -78,6 +83,9 @@ PluginInstanceChild::PluginInstanceChild > mWsInfo.display = GDK_DISPLAY(); > # endif > #endif >+#if defined(OS_WIN) >+ mDoublePassEvent = ::RegisterWindowMessage(NS_OOPP_DOUBLEPASS_MSGID); >+#endif > } > > PluginInstanceChild::~PluginInstanceChild() >@@ -342,9 +350,21 @@ PluginInstanceChild::AnswerNPP_HandleEve > NPEvent evcopy = event.event; > > #ifdef OS_WIN >- // Setup the shared dib for painting and update evcopy. >- if (NPWindowTypeDrawable == mWindow.type && WM_PAINT == evcopy.event) >- SharedSurfaceBeforePaint(evcopy); >+ // Painting for win32. SharedSurfacePaint handles everything. >+ if (NPWindowTypeDrawable == mWindow.type) { >+ if (WM_PAINT == evcopy.event) { >+ *handled = SharedSurfacePaint(evcopy); >+ return true; >+ } >+ else if (mDoublePassEvent == evcopy.event) { extra space after '==' here. >+ // We'll render to mSharedSurfaceDib first, then render to a cached bitmap >+ // we store locally. The two passes are for alpha extraction, so the second >+ // pass must be to a flat white surface in order for things to work. >+ mDoublePass = RENDER_BACK_ONE; >+ *handled = true; >+ return true; >+ } >+ } I believe the common Mozilla style is to write comparisons forwards instead of backwards. i.e. the way you would say them. Writing them backwards makes them harder to read and I think that's worth optimizing for reading over avoiding accidental assignments. > #endif > > *handled = mPluginIface->event(&mData, reinterpret_cast<void*>(&evcopy)); >@@ -658,6 +678,8 @@ PluginInstanceChild::PluginWindowProc(HW > > /* windowless drawing helpers */ > >+// add a div opacity test. >+ unintentional comment? > bool > PluginInstanceChild::SharedSurfaceSetWindow(const NPRemoteWindow& aWindow, > NPError* rv) >@@ -675,6 +697,9 @@ PluginInstanceChild::SharedSurfaceSetWin > if (NS_FAILED(mSharedSurfaceDib.Attach((SharedDIB::Handle)aWindow.surfaceHandle, > aWindow.width, aWindow.height, 32))) > return false; >+ // Free any alpha extraction resources if needed. This will be reset >+ // the next time it's used. >+ AlphaExtractCacheRelease(); > } > > // NPRemoteWindow's origin is the origin of our shared dib. >@@ -694,20 +719,141 @@ void > PluginInstanceChild::SharedSurfaceRelease() > { > mSharedSurfaceDib.Close(); >+ AlphaExtractCacheRelease(); >+} >+ >+/* double pass cache buffer - (rarely) used in cases where alpha extraction >+ * occurs for windowless plugins. */ >+ >+bool >+PluginInstanceChild::AlphaExtractCacheSetup() >+{ >+ AlphaExtractCacheRelease(); >+ >+ mHdc = ::CreateCompatibleDC(NULL); >+ >+ if (!mHdc) >+ return false; >+ >+ BITMAPINFOHEADER bmih; >+ memset((void*)&bmih, 0, sizeof(BITMAPINFOHEADER)); >+ bmih.biSize = sizeof(BITMAPINFOHEADER); >+ bmih.biWidth = mWindow.width; >+ bmih.biHeight = mWindow.height; >+ bmih.biPlanes = 1; >+ bmih.biBitCount = 32; >+ bmih.biCompression = BI_RGB; >+ >+ void* ppvBits = nsnull; >+ mBmp = ::CreateDIBSection(mHdc, >+ (BITMAPINFO*)&bmih, >+ DIB_RGB_COLORS, >+ (void**)&ppvBits, >+ NULL, >+ (unsigned long)sizeof(BITMAPINFOHEADER)); >+ if (!mBmp) >+ return false; >+ >+ mOldObj = ::SelectObject(mHdc, mBmp); Since we know that the bitmap associated with mHdc isn't going to ever be used we could DeleteObject() it instead of holding on to it and replacing it at the end. I'm not sure which is better. If replacing it at the end is more idiomatic we should do that instead of trying to be clever. >+ return true; > } > > void >-PluginInstanceChild::SharedSurfaceBeforePaint(NPEvent& evcopy) >+PluginInstanceChild::AlphaExtractCacheRelease() > { >- // Update the clip rect on our internal hdc >+ if (mHdc && mOldObj) >+ ::SelectObject(mHdc, mOldObj); >+ >+ if (mBmp) >+ ::DeleteObject(mBmp); >+ >+ if (mHdc) >+ ::DeleteObject(mHdc); >+ >+ mBmp = NULL; >+ mOldObj = NULL; >+ mHdc = NULL; These three variables are only used when we're doing alpha extraction right? If so it might be worth moving them into a struct inside the class to make this clear. e.g. struct { bmp; oldObj; hdc; } mAlphaExtract; or something like that. >diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp >--- a/dom/plugins/PluginInstanceParent.cpp >+++ b/dom/plugins/PluginInstanceParent.cpp >@@ -47,10 +47,6 @@ > #include "npfunctions.h" > #include "nsAutoPtr.h" > >-#if defined(OS_WIN) >-#define NS_OOPP_DOUBLEPASS_MSGID TEXT("MozDoublePassMsg") >-#endif >- > using namespace mozilla::plugins; > > PluginInstanceParent::PluginInstanceParent(PluginModuleParent* parent, >@@ -61,14 +57,6 @@ PluginInstanceParent::PluginInstancePare > mNPNIface(npniface), > mWindowType(NPWindowTypeWindow) > { >-#if defined(OS_WIN) >- // Event sent from nsObjectFrame indicating double pass rendering for >- // windowless plugins. RegisterWindowMessage makes it easy sync event >- // values, and insures we never conflict with windowing events we allow >- // for windowless plugins. >- mDoublePassEvent = ::RegisterWindowMessage(NS_OOPP_DOUBLEPASS_MSGID); >- mLocalCopyRender = false; >-#endif > } > > PluginInstanceParent::~PluginInstanceParent() >@@ -467,16 +455,9 @@ PluginInstanceParent::NPP_HandleEvent(vo > > #if defined(OS_WIN) > RECT rect; >- if (mWindowType == NPWindowTypeDrawable) { >- if (mDoublePassEvent && mDoublePassEvent == npevent->event) { >- // Sent from nsObjectFrame to let us know a double pass render is in progress. >- mLocalCopyRender = PR_TRUE; >- return true; >- } else if (WM_PAINT == npevent->event) { >- // Don't forward on the second pass, otherwise, fall through. >- if (!SharedSurfaceBeforePaint(rect, npremoteevent)) >- return true; >- } >+ if (mWindowType == NPWindowTypeDrawable && >+ WM_PAINT == npevent->event) { >+ SharedSurfaceBeforePaint(rect, npremoteevent); more backwards conditions here. > } > #endif > >@@ -730,26 +711,13 @@ PluginInstanceParent::SharedSurfaceSetWi > return true; > } > >-bool >+void > PluginInstanceParent::SharedSurfaceBeforePaint(RECT& rect, > NPRemoteEvent& npremoteevent) > { > RECT* dr = (RECT*)npremoteevent.event.lParam; > HDC parentHdc = (HDC)npremoteevent.event.wParam; > >- // We render twice per frame for windowless plugins that sit in transparent >- // frames. (See nsObjectFrame and gfxWindowsNativeDrawing for details.) IPC >- // message delays in OOP plugin painting can result in two passes yeilding >- // different animation frames. The second rendering doesn't need to go over >- // the wire (we already have a copy of the frame in mSharedSurfaceDib) so we >- // skip off requesting the second. This also gives us a nice perf boost. >- if (mLocalCopyRender) { >- mLocalCopyRender = false; >- // Reuse the old render. >- SharedSurfaceAfterPaint(&npremoteevent.event); >- return false; >- } >- > nsIntRect dirtyRect(dr->left, dr->top, dr->right-dr->left, dr->bottom-dr->top); > dirtyRect.MoveBy(-mPluginPort.x, -mPluginPort.y); // should always be smaller than dirtyRect > >@@ -766,14 +734,11 @@ PluginInstanceParent::SharedSurfaceBefor > // setup the translated dirty rect we'll send to the child > rect.left = dirtyRect.x; > rect.top = dirtyRect.y; >- rect.right = dirtyRect.width; >- rect.bottom = dirtyRect.height; >+ rect.right = dirtyRect.x + dirtyRect.width; >+ rect.bottom = dirtyRect.y + dirtyRect.height; was this broken before? > > npremoteevent.event.wParam = WPARAM(0); > npremoteevent.event.lParam = LPARAM(&rect); >- >- // Send the event to the plugin >- return true; > } > > void >diff --git a/gfx/thebes/src/gfxWindowsNativeDrawing.cpp b/gfx/thebes/src/gfxWindowsNativeDrawing.cpp >--- a/gfx/thebes/src/gfxWindowsNativeDrawing.cpp >+++ b/gfx/thebes/src/gfxWindowsNativeDrawing.cpp >@@ -206,6 +206,21 @@ gfxWindowsNativeDrawing::BeginNativeDraw > } > > PRBool >+gfxWindowsNativeDrawing::IsDoublePass() >+{ >+ nsRefPtr<gfxASurface> surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y); >+ if (!surf || surf->CairoStatus()) >+ return false; >+ if ((surf->GetType() == gfxASurface::SurfaceTypeWin32 || >+ surf->GetType() == gfxASurface::SurfaceTypeWin32Printing) && >+ (surf->GetContentType() != gfxASurface::CONTENT_COLOR || >+ (surf->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA && >+ !(mNativeDrawFlags & CAN_DRAW_TO_COLOR_ALPHA)))) >+ return PR_TRUE; Is this condition copied from somewhere?

(In reply to comment #6) > (From update of attachment 416957 [details] [diff] [review]) > >+ rect.right = dirtyRect.x + dirtyRect.width; > >+ rect.bottom = dirtyRect.y + dirtyRect.height; > > was this broken before? Yes it was. > > PRBool > >+gfxWindowsNativeDrawing::IsDoublePass() > >+{ > >+ nsRefPtr<gfxASurface> surf = mContext->CurrentSurface(&mDeviceOffset.x, &mDeviceOffset.y); > >+ if (!surf || surf->CairoStatus()) > >+ return false; > >+ if ((surf->GetType() == gfxASurface::SurfaceTypeWin32 || > >+ surf->GetType() == gfxASurface::SurfaceTypeWin32Printing) && > >+ (surf->GetContentType() != gfxASurface::CONTENT_COLOR || > >+ (surf->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA && > >+ !(mNativeDrawFlags & CAN_DRAW_TO_COLOR_ALPHA)))) > >+ return PR_TRUE; > > Is this condition copied from somewhere? Yes, see BeginNativeDrawing.

(In reply to comment #7) > (In reply to comment #6) > > > > Is this condition copied from somewhere? > > Yes, see BeginNativeDrawing. Probably worth mentioning that in a comment.

Updated per comments.

Comment on attachment 417511 [details] [diff] [review] ipc plugin alpha extraction patch v.2 >+/* double pass cache buffer - (rarely) used in cases where alpha extraction >+ * occurs for windowless plugins. */ >+ >+bool >+PluginInstanceChild::AlphaExtractCacheSetup() >+{ >+ AlphaExtractCacheRelease(); >+ >+ mAlphaExtract.hdc = ::CreateCompatibleDC(NULL); >+ >+ if (!mAlphaExtract.hdc) >+ return false; >+ >+ BITMAPINFOHEADER bmih; >+ memset((void*)&bmih, 0, sizeof(BITMAPINFOHEADER)); >+ bmih.biSize = sizeof(BITMAPINFOHEADER); >+ bmih.biWidth = mWindow.width; >+ bmih.biHeight = mWindow.height; >+ bmih.biPlanes = 1; >+ bmih.biBitCount = 32; >+ bmih.biCompression = BI_RGB; >+ >+ void* ppvBits = nsnull; >+ mAlphaExtract.bmp = ::CreateDIBSection(mAlphaExtract.hdc, >+ (BITMAPINFO*)&bmih, >+ DIB_RGB_COLORS, >+ (void**)&ppvBits, >+ NULL, >+ (unsigned long)sizeof(BITMAPINFOHEADER)); >+ if (!mAlphaExtract.bmp) >+ return false; >+ >+ DeleteObject(::SelectObject(mAlphaExtract.hdc, mAlphaExtract.bmp)); This could probably use a comment explaining what's going on.

http://hg.mozilla.org/mozilla-central/rev/10f2bc57c804 merged to mozilla-central at http://hg.mozilla.org/mozilla-central/rev/4eef63c0c6c9