524180 - Implement async drawWindow/Element() using shared memory

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Joe Drew (not getting mail)]

Attachment: add support for shared memory canvas

This patch adds support for shared memory canvases to the electrolysis branch. Basically, you set an attribute on your canvas - moz-shmem - and then call asyncDrawXULElement on it as usual. (Regardless of whether your canvas is backed by shared memory, it will do the right thing.)

When in shmem mode, we just tell the content process to draw into the shared memory, and then display it. We have two shared memory segments per canvas: a front and a back buffer. The canvas always has a front buffer, and passes the back buffer to the rendering process when a render is requested. Then, when the rendering is complete, the back buffer and the front buffer are swapped, and the new front buffer's contents are copied into the new back buffer. (Note that right now, we will just drop on the floor any rendering requests that come in when we're waiting on another render. Queueing will need to be a future bug.)

This is almost ready for review. I still need to make it so that we won't create shared memory segments unless the person calling getContext() (or potentially who calls setAttribute()) is privileged.

Comment 2

[Joe Drew (not getting mail)]

Attachment: add MozGetShmemContext, v1

This slightly changes how we go about rendering using Shmem. After some discussion, we decided that adding a new function, rather than an attribute, makes the most sense, so I've added a function, MozGetShmemContext, to the canvas element, that is only callable from privileged contexts. Other than that, this patch is the same as the last, except it includes all the files I added. :)

Comment 3

[Joe Drew (not getting mail)]

Attachment: patch to use shmem context in fennec, v1

In addition to the changes in dependent bugs and bug 525287, this diff is needed to fennec to use shmem-backed canvases.

Comment 4

[Oleg Romashin (:romaxa)]

Something missing or outdated...

mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp: In member function 'virtual nsresult nsCanvasRenderingContext2D::AsyncDrawXULElement(nsIDOMXULElement*, float, float, float, float, const nsAString_internal&, PRUint32)':
mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:3472: error: 'class mozilla::ipc::Shmem' has no member named 'IsWritable'

Comment 5

[Joe Drew (not getting mail)]

As I mentioned in comment 3, you need the changes in the dependent bugs. This includes dependent bug 544607 and dependent bug 544623.

Comment 6

[Joe Drew (not getting mail)]

Attachment: implement MozGetShmemContext, v2

This v2 of the patch does things a little smarter: we actually forward the current transformation of the canvas context, which allows us to ensure we're actually drawing the right part of the canvas, rather than trying to hack around the problem.

Comment 7

[Joe Drew (not getting mail)]

Attachment: fire an event when async canvas draws finish

This patch adds an event to be fired on the canvas element when the draw command returns from the content process and everything is synchronized to the chrome process. This allows consumers of AsyncDrawXULElement (with shared memory only, for now anyways) to know when their draw is complete, and sending another draw command to the canvas will succeed.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 428825 [details] [diff] [review]
fire an event when async canvas draws finish

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>@@ -1081,16 +1081,28 @@ nsCanvasRenderingContext2D::Swap(mozilla
>     mThebes->SetColor(gfxRGBA(1,1,1,1));
> 
>     Redraw(gfxRect(x, y, w, h));
> 
>     // Copy the new contents to the old to keep them in sync. 
>     memcpy(mBackBuffer.get<unsigned char>(), mFrontBuffer.get<unsigned char>(),
>            mWidth * mHeight * 4);
> 
>+    // Notify listeners that we've finished drawing
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(mCanvasElement);
>+    nsIDocument* ownerDoc = nsnull;
>+    if (content)
>+	ownerDoc = content->GetOwnerDoc();
>+
>+    if (ownerDoc && mCanvasElement) {
>+	nsContentUtils::DispatchTrustedEvent(ownerDoc, mCanvasElement, 
>+					     NS_LITERAL_STRING("MozAsyncCanvasRender"),
>+					     /* aCanBubble = */ PR_TRUE, /* aCancelable = */ PR_TRUE);
>+    }
>+

I'd write that
  if (mCanvasElement) {
    nsCOMPtr<nsIContent> content = do_QueryInterface(mCanvasElement);
    nsContentUtils::DispatchTrustedEvent(content->GetOwnerDoc(), content,
                                         NS_LITERAL_STRING("MozAsyncCanvasRender"),
                                         PR_TRUE, PR_TRUE);
  }

And note, seems like there are some tabs in your patch.

But with that all fixed, r=me

Comment 9

[Doug Turner (:dougt)]

joe - please protect uses of mShmem with #ifdef MOZ_IPC.  If you compile without IPC enabled, you will get compilation errors.

Comment 10

[Benjamin Smedberg]

Comment on attachment 428329 [details] [diff] [review]
implement MozGetShmemContext, v2

I am not a good reviewer for this, I don't think. cjones or somebody from graphics/layout should review.

Comment 11

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

Comment on attachment 428329 [details] [diff] [review]
implement MozGetShmemContext, v2

I'll do a "jrmuizel lite" review, which IMHO is enough to get this on trunk.

Comment 12

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

Sorry, by "trunk" I meant "e10s" ;).

Comment 13

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

Comment on attachment 428329 [details] [diff] [review]
implement MozGetShmemContext, v2

>diff --git a/content/canvas/public/DocumentRendererShmemChild.h b/content/canvas/public/DocumentRendererShmemChild.h
>new file mode 100644
>--- /dev/null
>+++ b/content/canvas/public/DocumentRendererShmemChild.h
>+class DocumentRendererShmemChild : public PDocumentRendererShmemChild
>+{
>+public:
>+    DocumentRendererShmemChild();
>+    virtual ~DocumentRendererShmemChild();
>+
>+    bool RenderDocument(nsIDOMWindow *window, const PRInt32& x,
>+                        const PRInt32& y, const PRInt32& w,
>+                        const PRInt32& h, const nsString& aBGColor,
>+                        const PRUint32& flags, const PRBool& flush,
>+			const gfxMatrix &aMatrix,

Nit: prevailing style is |Foo& foo|.  Also, this line is indented with tab characters.

>diff --git a/content/canvas/public/nsICanvasRenderingContextInternal.h b/content/canvas/public/nsICanvasRenderingContextInternal.h
>--- a/content/canvas/public/nsICanvasRenderingContextInternal.h
>+++ b/content/canvas/public/nsICanvasRenderingContextInternal.h
>@@ -39,23 +39,29 @@
> #define nsICanvasRenderingContextInternal_h___
> 
> #include "nsISupports.h"
> #include "nsICanvasElement.h"

I think you only need to forward-declare nsICanvasElement.

> #include "nsIInputStream.h"
> #include "nsIDocShell.h"
> #include "gfxPattern.h"
> 
>-// {ed741c16-4039-469b-91da-dca742c51a9f}
>+// {3c4632ab-8443-4082-a8a310e7cfba4c74}
> #define NS_ICANVASRENDERINGCONTEXTINTERNAL_IID \
>-  { 0xed741c16, 0x4039, 0x469b, { 0x91, 0xda, 0xdc, 0xa7, 0x42, 0xc5, 0x1a, 0x9f } }
>+  { 0x3c4632ab, 0x8443, 0x4082, { 0xa8, 0xa3, 0x10, 0xe7, 0xcf, 0xba, 0x4c, 0x74 } }
> 
> class gfxContext;
> class gfxASurface;
> 
>+namespace mozilla {
>+namespace ipc {
>+class Shmem;
>+}
>+}
>+
> class nsICanvasRenderingContextInternal : public nsISupports {
> public:
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_ICANVASRENDERINGCONTEXTINTERNAL_IID)
> 
>   // This method should NOT hold a ref to aParentCanvas; it will be called
>   // with nsnull when the element is going away.
>   NS_IMETHOD SetCanvasElement(nsICanvasElement* aParentCanvas) = 0;
> 
>@@ -85,14 +91,25 @@ public:
>   // If this context is opaque, the backing store of the canvas should
>   // be created as opaque; all compositing operators should assume the
>   // dst alpha is always 1.0.  If this is never called, the context
>   // defaults to false (not opaque).
>   NS_IMETHOD SetIsOpaque(PRBool isOpaque) = 0;
> 
>   // Redraw the dirty rectangle of this canvas.
>   NS_IMETHOD Redraw(const gfxRect &dirty) = 0;

Nit: |Foo& foo|.  Is |Foo &foo| gfx style?

>+
>+  // If this context can be set to use Mozilla's Shmem segments as its backing
>+  // store, this will set it to that state. Note that if you have drawn
>+  // anything into this canvas before changing the shmem state, it will be
>+  // lost.
>+  NS_IMETHOD SetIsShmem(PRBool isShmem) = 0;
>+
>+  // Swap this back buffer with the front, and copy its contents to the new
>+  // back. x, y, w, and h specify the area of |back| that is dirty.
>+  NS_IMETHOD Swap(mozilla::ipc::Shmem &back,

Nit: here too.

>+                  PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h) = 0;
> };
> 
> NS_DEFINE_STATIC_IID_ACCESSOR(nsICanvasRenderingContextInternal,
>                               NS_ICANVASRENDERINGCONTEXTINTERNAL_IID)
> 
> #endif /* nsICanvasRenderingContextInternal_h___ */
>diff --git a/content/canvas/src/DocumentRendererShmemChild.cpp b/content/canvas/src/DocumentRendererShmemChild.cpp
>new file mode 100644
>--- /dev/null
>+++ b/content/canvas/src/DocumentRendererShmemChild.cpp
>+bool
>+DocumentRendererShmemChild::RenderDocument(nsIDOMWindow *window, const PRInt32& x,
>+                                      const PRInt32& y, const PRInt32& w,
>+                                      const PRInt32& h, const nsString& aBGColor,
>+                                      const PRUint32& flags, const PRBool& flush,
>+				      const gfxMatrix &aMatrix,

Nit: ref style and tab characters again.

>+                                      const PRInt32& bufw, const PRInt32& bufh,
>+                                      Shmem& data)
>+{
>+    if (flush)
>+        FlushLayoutForTree(window);
>+
>+    nsCOMPtr<nsPresContext> presContext;
>+    nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(window);
>+    if (win) {
>+        nsIDocShell* docshell = win->GetDocShell();
>+        if (docshell) {
>+            docshell->GetPresContext(getter_AddRefs(presContext));
>+        }
>+    }
>+    if (!presContext)
>+        return false;
>+
>+    nscolor bgColor;
>+    nsCOMPtr<nsICSSParser> parser = do_CreateInstance("@mozilla.org/content/css-parser;1");
>+    nsresult rv = parser->ParseColorString(PromiseFlatString(aBGColor),
>+                                           nsnull, 0, &bgColor);
>+    if (NS_FAILED(rv))
>+        return false;
>+
>+    nsIPresShell* presShell = presContext->PresShell();
>+
>+    nsRect r(x, y, w, h);
>+
>+    // Draw directly into the output array.
>+    nsRefPtr<gfxImageSurface> surf = new gfxImageSurface(data.get<PRUint8>(),
>+                                                         gfxIntSize(bufw, bufh),
>+                                                         4 * bufw,

Why 4*bufw?  For a/r/g/b?

>+                                                         gfxASurface::ImageFormatARGB32);
>+    nsRefPtr<gfxContext> ctx = new gfxContext(surf);
>+    ctx->SetMatrix(aMatrix);
>+
>+    PRBool oldDisableValue = nsLayoutUtils::sDisableGetUsedXAssertions;
>+    nsLayoutUtils::sDisableGetUsedXAssertions = oldDisableValue || !flush;
>+    presShell->RenderDocument(r, flags, bgColor, ctx);
>+    nsLayoutUtils::sDisableGetUsedXAssertions = oldDisableValue;
>+

I don't think this disablegetusedxassertions exists anymore, but I'm
sure somebody has this removed in a local patch.

>+    return true;
>+}
>diff --git a/content/canvas/src/WebGLContext.cpp b/content/canvas/src/WebGLContext.cpp
>--- a/content/canvas/src/WebGLContext.cpp
>+++ b/content/canvas/src/WebGLContext.cpp
>@@ -352,17 +352,16 @@ WebGLContext::GetThebesSurface(gfxASurfa
>         return NS_ERROR_NOT_AVAILABLE;
>     }
> 
>     *surface = mGLPbuffer->ThebesSurface();
>     NS_IF_ADDREF(*surface);
>     return NS_OK;
> }
> 
>-

Probably want to delete this patch hunk.

> //
> // XPCOM goop
> //
> 
> NS_IMPL_ADDREF(WebGLContext)
> NS_IMPL_RELEASE(WebGLContext)
> 
> NS_INTERFACE_MAP_BEGIN(WebGLContext)
>diff --git a/content/canvas/src/WebGLContext.h b/content/canvas/src/WebGLContext.h
>--- a/content/canvas/src/WebGLContext.h
>+++ b/content/canvas/src/WebGLContext.h
>@@ -237,17 +237,21 @@ public:
>     NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height)
>         { return NS_ERROR_NOT_IMPLEMENTED; }
>     NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter f);
>     NS_IMETHOD GetInputStream(const char* aMimeType,
>                               const PRUnichar* aEncoderOptions,
>                               nsIInputStream **aStream);
>     NS_IMETHOD GetThebesSurface(gfxASurface **surface);
>     NS_IMETHOD SetIsOpaque(PRBool b) { return NS_OK; };
>+    NS_IMETHOD SetIsShmem(PRBool b) { return NS_OK; }

Why isn't this also NS_ERROR_NOT_IMPLEMENTED?

>diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
>+static void
>+CopyContext(gfxContext* dest, gfxContext* src)
>+{
>+    dest->Multiply(src->CurrentMatrix());
>+
>+    nsRefPtr<gfxPath> path = src->CopyPath();
>+    dest->NewPath();
>+    dest->AppendPath(path);
>+
>+    nsRefPtr<gfxPattern> pattern = src->GetPattern();
>+    dest->SetPattern(pattern);
>+
>+    dest->SetLineWidth(src->CurrentLineWidth());
>+    dest->SetLineCap(src->CurrentLineCap());
>+    dest->SetLineJoin(src->CurrentLineJoin());
>+    dest->SetMiterLimit(src->CurrentMiterLimit());
>+    dest->SetFillRule(src->CurrentFillRule());
>+
>+    dest->SetAntialiasMode(src->CurrentAntialiasMode());
>+}

Seems cleaner to me to make a gfxContext::Clone() method or something
to do this (dunno if that's possible).

>+
>+
> /**
>  ** nsCanvasGradient
>  **/
> #define NS_CANVASGRADIENT_PRIVATE_IID \
>     { 0x491d39d8, 0x4058, 0x42bd, { 0xac, 0x76, 0x70, 0xd5, 0x62, 0x7f, 0x02, 0x10 } }
> class nsCanvasGradient : public nsIDOMCanvasGradient
> {
> public:
>@@ -333,18 +358,23 @@ public:
>     NS_IMETHOD SetDimensions(PRInt32 width, PRInt32 height);
>     NS_IMETHOD InitializeWithSurface(nsIDocShell *shell, gfxASurface *surface, PRInt32 width, PRInt32 height);
>     NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter aFilter);
>     NS_IMETHOD GetInputStream(const char* aMimeType,
>                               const PRUnichar* aEncoderOptions,
>                               nsIInputStream **aStream);
>     NS_IMETHOD GetThebesSurface(gfxASurface **surface);
>     NS_IMETHOD SetIsOpaque(PRBool isOpaque);
>+    NS_IMETHOD SetIsShmem(PRBool isShmem);
>     // this rect is in CSS pixels
>     NS_IMETHOD Redraw(const gfxRect &r);
>+    // Swap this back buffer with the front, and copy its contents to the new back.
>+    // x, y, w, and h specify the area of |back| that is dirty.
>+    NS_IMETHOD Swap(mozilla::ipc::Shmem &back, PRInt32 x, PRInt32 y, 

Nit: ref style.  Also this should be #ifdef MOZ_IPC.

>+#ifdef MOZ_IPC
>+bool
>+nsCanvasRenderingContext2D::CreateShmemSegments(PRInt32 width, PRInt32 height,
>+                                                gfxASurface::gfxImageFormat format)
>+{
>+    if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>+                AllocShmem(width * height * 4, &mFrontBuffer))
>+        return false;
>+    if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>+                AllocShmem(width * height * 4, &mBackBuffer))
>+        return false;
>+

Nit: |using mozilla::dom::ContentProcessParent;| please.

> NS_IMETHODIMP
>+nsCanvasRenderingContext2D::SetIsShmem(PRBool isShmem)
>+{
>+    if (isShmem == mShmem)
>+        return NS_OK;
>+
>+    mShmem = isShmem;
>+
>+    if (mValid) {
>+        /* If we've already been created, let SetDimensions take care of
>+         * recreating our surface
>+         */
>+        return SetDimensions(mWidth, mHeight);
>+    }
>+
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::Swap(mozilla::ipc::Shmem &aBack, 

Nit: ref style.  Also, |using mozilla::ipc::Shmem;| please.

>+                                 PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h)
>+{
>+#ifndef MOZ_IPC

I think this whole method needs to be #ifdef MOZ_IPC since you're using Shmem.

>+    return NS_ERROR_NOT_IMPLEMENTED;
>+#else
>+    // Our front buffer is always the correct size. If this back buffer doesn't
>+    // match the front buffer's size, it's out of date (we've resized since
>+    // this message was sent) and we should just ignore it.
>+    if (mFrontBuffer.Size<unsigned char>() != aBack.Size<unsigned char>())
>+        return NS_OK;
>+
>+    // Swap back and front.
>+    // mBackBuffer should be null here, since we've previously sent it to the
>+    // child process.
>+    mBackBuffer = mFrontBuffer;
>+    mFrontBuffer = aBack;
>+
>+    // do want mozilla::Swap
>+    nsRefPtr<gfxASurface> tmp = mSurface;
>+    mSurface = mBackSurface;
>+    mBackSurface = tmp;
>+

Oops, I have a patch for this already that never landed.  But it's
deprecated in favor of std::swap().

> static const gfxFloat SIGMA_MAX = 25;
> 
> gfxContext*
> nsCanvasRenderingContext2D::ShadowInitialize(const gfxRect& extents, gfxAlphaBoxBlur& blur)
> {
>     gfxIntSize blurRadius;
> 
>     gfxFloat sigma = CurrentState().shadowBlur > 8 ? sqrt(CurrentState().shadowBlur) : CurrentState().shadowBlur / 2;
>@@ -3357,21 +3463,47 @@ nsCanvasRenderingContext2D::AsyncDrawXUL
>         renderDocFlags &= ~nsIPresShell::RENDER_IGNORE_VIEWPORT_SCROLLING;
>     }
> 
>     PRInt32 x = nsPresContext::CSSPixelsToAppUnits(aX),
>             y = nsPresContext::CSSPixelsToAppUnits(aY),
>             w = nsPresContext::CSSPixelsToAppUnits(aW),
>             h = nsPresContext::CSSPixelsToAppUnits(aH);
> 
>-    mozilla::ipc::PDocumentRendererParent *pdocrender =
>-        child->SendPDocumentRendererConstructor(x, y, w, h, nsString(aBGColor), renderDocFlags, flush);
>-    mozilla::ipc::DocumentRendererParent *docrender = static_cast<mozilla::ipc::DocumentRendererParent *>(pdocrender);

Can't tell from the patch but this code needs to be #ifdef MOZ_IPC if it's not already.

>+    if (mShmem) {
>+        if (!mBackBuffer.IsWritable())
>+            return NS_ERROR_FAILURE;
> 
>-    docrender->SetCanvasContext(this, mThebes);
>+        mozilla::ipc::PDocumentRendererShmemParent *pdocrender =

Nit: |using mozilla::ipc::PDocumentRendererShmemParent;| please.

>+            child->SendPDocumentRendererShmemConstructor(x, y, w, h,
>+                                                         nsString(aBGColor),
>+                                                         renderDocFlags, flush,
>+							 mThebes->CurrentMatrix(),

Nit: tab characters.

>diff --git a/content/html/content/src/nsHTMLCanvasElement.cpp b/content/html/content/src/nsHTMLCanvasElement.cpp
>--- a/content/html/content/src/nsHTMLCanvasElement.cpp
>+++ b/content/html/content/src/nsHTMLCanvasElement.cpp
>@@ -121,16 +121,18 @@ public:
>   nsresult CopyInnerTo(nsGenericElement* aDest) const;
> protected:
>   nsIntSize GetWidthHeight();
> 
>   nsresult UpdateContext();
>   nsresult ToDataURLImpl(const nsAString& aMimeType,
>                          const nsAString& aEncoderOptions,
>                          nsAString& aDataURL);
>+  nsresult GetContextHelper(const nsAString &aContextId,

Nit: ref style.

>+NS_IMETHODIMP
>+nsHTMLCanvasElement::MozGetShmemContext(const nsAString& aContextId,
>+                                        nsISupports **aContext)
>+{

Might want #ifdef MOZ_IPC return NS_ERROR_NOT_IMPLEMENTED.

>diff --git a/dom/ipc/ContentProcessChild.h b/dom/ipc/ContentProcessChild.h
>--- a/dom/ipc/ContentProcessChild.h
>+++ b/dom/ipc/ContentProcessChild.h
>@@ -66,16 +66,18 @@ public:
>     virtual bool DeallocPIFrameEmbedding(PIFrameEmbeddingChild*);
> 
>     virtual PTestShellChild* AllocPTestShell();
>     virtual bool DeallocPTestShell(PTestShellChild*);
> 
>     virtual PNeckoChild* AllocPNecko();
>     virtual bool DeallocPNecko(PNeckoChild*);
> 
>+    virtual bool RecvDummy(Shmem& foo) { return true; }
>+

We don't need this hack anymore, right?

>diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl
>--- a/dom/ipc/PContentProcess.ipdl
>+++ b/dom/ipc/PContentProcess.ipdl
>@@ -51,14 +51,17 @@ rpc protocol PContentProcess
>     manages PTestShell;
>     manages PNecko;
> 
> child:
>     PIFrameEmbedding();
> 
>     PTestShell();
> 
>+    // A dummy message to make sure PContentProcess contains methods to create
>+    // Shmem segments.
>+    async Dummy(Shmem foo);

Same here.

>diff --git a/dom/ipc/PDocumentRendererShmem.ipdl b/dom/ipc/PDocumentRendererShmem.ipdl
>new file mode 100644
>--- /dev/null
>+++ b/dom/ipc/PDocumentRendererShmem.ipdl
>@@ -0,0 +1,54 @@
>+include protocol "PIFrameEmbedding.ipdl";
>+
>+namespace mozilla {
>+namespace ipc {
>+
>+protocol PDocumentRendererShmem
>+{
>+  manager PIFrameEmbedding;
>+
>+parent:
>+    // Returns the offset, width and height, in pixels, of the area in the
>+    // buffer that was drawn.
>+    __delete__(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, Shmem data);

Seems like it would be cleaner to serialize nsRect or gfxRect and use that here.

>diff --git a/dom/ipc/PIFrameEmbedding.ipdl b/dom/ipc/PIFrameEmbedding.ipdl
>--- a/dom/ipc/PIFrameEmbedding.ipdl
>+++ b/dom/ipc/PIFrameEmbedding.ipdl
>@@ -34,30 +34,33 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> include protocol "PContentProcess.ipdl";
> include protocol "PDocumentRenderer.ipdl";
>+include protocol "PDocumentRendererShmem.ipdl";
> 
> include "mozilla/TabTypes.h";
> include "TabMessageUtils.h";
> 
> using MagicWindowHandle;
> using RemoteDOMEvent;
>+using gfxMatrix;
> 
> namespace mozilla {
> namespace dom {
> 
> rpc protocol PIFrameEmbedding
> {
>     manager PContentProcess;
>     manages PDocumentRenderer;
>+    manages PDocumentRendererShmem;
> 
> child:
>     __delete__();
> 
> parent:
>     /**
>      * When child sends this message, parent should move focus to
>      * the next or previous focusable element.
>@@ -102,14 +105,20 @@ child:
>     activateFrameEvent(nsString aType, bool capture);
> 
>     loadRemoteScript(nsString aURL);
> 
>     sendAsyncMessageToChild(nsString aMessage, nsString aJSON);
> 
>     PDocumentRenderer(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, nsString bgcolor, PRUint32 flags, bool flush);
> 

Maybe nsRect/gfxRect?

>+    // @param matrix the transformation matrix the context we're going to draw into should have.
>+    // @param bufw the width of @buf, in pixels
>+    // @param bufh the height of @buf, in pixels
>+    PDocumentRendererShmem(PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, nsString bgcolor, PRUint32 flags, bool flush,
>+                           gfxMatrix matrix, PRInt32 bufw, PRInt32 bufh, Shmem buf);
>+

Maybe nsRect/gfxRect?

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp
>+++ b/dom/ipc/TabChild.cpp
>+mozilla::ipc::PDocumentRendererShmemChild*
>+TabChild::AllocPDocumentRendererShmem(
>+        const PRInt32& x,
>+        const PRInt32& y,
>+        const PRInt32& w,
>+        const PRInt32& h,
>+        const nsString& bgcolor,
>+        const PRUint32& flags,
>+        const bool& flush,
>+	const gfxMatrix& aMatrix,

Tab character.

>+        const PRInt32& bufw,
>+        const PRInt32& bufh,
>+        Shmem& buf)
>+{
>+    return new mozilla::ipc::DocumentRendererShmemChild();
>+}
>+
>+bool
>+TabChild::DeallocPDocumentRendererShmem(PDocumentRendererShmemChild* actor)
>+{
>+    delete actor;
>+    return true;
>+}
>+
>+bool
>+TabChild::RecvPDocumentRendererShmemConstructor(
>+        mozilla::ipc::PDocumentRendererShmemChild *__a,

You don't need to fully qualify this identifier, IPDL-generated code
adds a |using| automatically.

>+        const PRInt32& aX,
>+        const PRInt32& aY,
>+        const PRInt32& aW,
>+        const PRInt32& aH,
>+        const nsString& bgcolor,
>+        const PRUint32& flags,
>+        const bool& flush,
>+	const gfxMatrix& aMatrix,

Tab character.

>+        const PRInt32& aBufW,
>+        const PRInt32& aBufH,
>+        Shmem& aBuf)
>+{
>+    mozilla::ipc::DocumentRendererShmemChild *render = 
>+        static_cast<mozilla::ipc::DocumentRendererShmemChild *>(__a);
>+

Don't need full qualification here either.

>+    nsCOMPtr<nsIWebBrowser> browser = do_QueryInterface(mWebNav);
>+    if (!browser)
>+        return true; // silently ignore
>+    nsCOMPtr<nsIDOMWindow> window;
>+    if (NS_FAILED(browser->GetContentDOMWindow(getter_AddRefs(window))) ||
>+        !window)
>+    {

Nit: brace on previous line, or drop them.

>diff --git a/dom/ipc/TabChild.h b/dom/ipc/TabChild.h
>--- a/dom/ipc/TabChild.h
>+++ b/dom/ipc/TabChild.h
>@@ -60,16 +60,18 @@
> #include "nsNetUtil.h"
> #include "nsFrameMessageManager.h"
> #include "nsIScriptContext.h"
> #include "nsDOMEventTargetHelper.h"
> #include "nsIPrincipal.h"
> #include "nsIScriptObjectPrincipal.h"
> #include "nsIScriptContext.h"
> 
>+class gfxMatrix;
>+
> namespace mozilla {
> namespace dom {
> 
> class TabChild;
> 
> class TabChildGlobal : public nsDOMEventTargetHelper,
>                        public nsIContentFrameMessageManager,
>                        public nsIScriptObjectPrincipal,
>@@ -160,16 +162,17 @@ public:
>     virtual bool RecvsendMouseEvent(const nsString& aType,
>                                     const PRInt32&  aX,
>                                     const PRInt32&  aY,
>                                     const PRInt32&  aButton,
>                                     const PRInt32&  aClickCount,
>                                     const PRInt32&  aModifiers,
>                                     const bool&     aIgnoreRootScrollFrame);
>     virtual bool RecvactivateFrameEvent(const nsString& aType, const bool& capture);
>+
>     virtual bool RecvloadRemoteScript(const nsString& aURL);
>     virtual bool RecvsendAsyncMessageToChild(const nsString& aMessage,
>                                              const nsString& aJSON);
>     virtual mozilla::ipc::PDocumentRendererChild* AllocPDocumentRenderer(
>             const PRInt32& x,
>             const PRInt32& y,
>             const PRInt32& w,
>             const PRInt32& h,
>@@ -182,16 +185,43 @@ public:
>             const PRInt32& x,
>             const PRInt32& y,
>             const PRInt32& w,
>             const PRInt32& h,
>             const nsString& bgcolor,
>             const PRUint32& flags,
>             const bool& flush);
> 
>+    virtual mozilla::ipc::PDocumentRendererShmemChild* AllocPDocumentRendererShmem(

Don't need full qualification.

>+            const PRInt32& x,
>+            const PRInt32& y,
>+            const PRInt32& w,
>+            const PRInt32& h,
>+            const nsString& bgcolor,
>+            const PRUint32& flags,
>+            const bool& flush,
>+	    const gfxMatrix& aMatrix,

Tab character.

>+            const PRInt32& bufw,
>+            const PRInt32& bufh,
>+            Shmem& buf);
>+    virtual bool DeallocPDocumentRendererShmem(PDocumentRendererShmemChild* actor);
>+    virtual bool RecvPDocumentRendererShmemConstructor(
>+            mozilla::ipc::PDocumentRendererShmemChild *__a,
>+            const PRInt32& aX,
>+            const PRInt32& aY,
>+            const PRInt32& aW,
>+            const PRInt32& aH,
>+            const nsString& bgcolor,
>+            const PRUint32& flags,
>+            const bool& flush,
>+	    const gfxMatrix& aMatrix,

Tab character.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -34,29 +34,31 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "TabParent.h"
> 
> #include "mozilla/ipc/DocumentRendererParent.h"
>+#include "mozilla/ipc/DocumentRendererShmemParent.h"
> 
> #include "nsIURI.h"
> #include "nsFocusManager.h"
> #include "nsCOMPtr.h"
> #include "nsServiceManagerUtils.h"
> #include "nsIDOMElement.h"
> #include "nsEventDispatcher.h"
> #include "nsIDOMEventTarget.h"
> #include "nsIDOMEvent.h"
> #include "nsIPrivateDOMEvent.h"
> #include "nsFrameLoader.h"
> 
> using mozilla::ipc::DocumentRendererParent;
>+using mozilla::ipc::DocumentRendererShmemParent;
> 
> namespace mozilla {
> namespace dom {
> 
> TabParent::TabParent()
> {
> }
> 
>@@ -150,16 +152,32 @@ TabParent::AllocPDocumentRenderer(const 
> 
> bool
> TabParent::DeallocPDocumentRenderer(PDocumentRendererParent* actor)
> {
>     delete actor;
>     return true;
> }
> 
>+mozilla::ipc::PDocumentRendererShmemParent*

Don't need full qualification here.

>diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h
>--- a/dom/ipc/TabParent.h
>+++ b/dom/ipc/TabParent.h
>@@ -43,16 +43,17 @@
> 
> #include "mozilla/ipc/GeckoChildProcessHost.h"
> 
> #include "nsCOMPtr.h"
> #include "nsIBrowserDOMWindow.h"
> 
> class nsIURI;
> class nsIDOMElement;
>+class gfxMatrix;
> 
> namespace mozilla {
> namespace dom {
> 
> class TabParent : public PIFrameEmbeddingParent
> {
> public:
>     TabParent();
>@@ -82,16 +83,31 @@ public:
>             const PRInt32& x,
>             const PRInt32& y,
>             const PRInt32& w,
>             const PRInt32& h,
>             const nsString& bgcolor,
>             const PRUint32& flags,
>             const bool& flush);
>     virtual bool DeallocPDocumentRenderer(PDocumentRendererParent* actor);
>+
>+    virtual mozilla::ipc::PDocumentRendererShmemParent* AllocPDocumentRendererShmem(
>+            const PRInt32& x,
>+            const PRInt32& y,
>+            const PRInt32& w,
>+            const PRInt32& h,
>+            const nsString& bgcolor,
>+            const PRUint32& flags,
>+            const bool& flush,
>+	    const gfxMatrix& aMatrix,

Tab character.

>+            const PRInt32& bufw,
>+            const PRInt32& bufh,
>+            Shmem &buf);

Ref style.

>+    virtual bool DeallocPDocumentRendererShmem(PDocumentRendererShmemParent* actor);
>+
> protected:
>     nsIDOMElement* mFrameElement;
>     nsCOMPtr<nsIBrowserDOMWindow> mBrowserDOMWindow;
> };
> 
> } // namespace dom
> } // namespace mozilla
> 

Looks OK to me as far as I understand what's going on.  Pretty cool actually.

Comment 14

[Joe Drew (not getting mail)]

(In reply to comment #13)

> >diff --git a/content/canvas/src/WebGLContext.h b/content/canvas/src/WebGLContext.h
> >--- a/content/canvas/src/WebGLContext.h
> >+++ b/content/canvas/src/WebGLContext.h
> >@@ -237,17 +237,21 @@ public:
> >     NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height)
> >         { return NS_ERROR_NOT_IMPLEMENTED; }
> >     NS_IMETHOD Render(gfxContext *ctx, gfxPattern::GraphicsFilter f);
> >     NS_IMETHOD GetInputStream(const char* aMimeType,
> >                               const PRUnichar* aEncoderOptions,
> >                               nsIInputStream **aStream);
> >     NS_IMETHOD GetThebesSurface(gfxASurface **surface);
> >     NS_IMETHOD SetIsOpaque(PRBool b) { return NS_OK; };
> >+    NS_IMETHOD SetIsShmem(PRBool b) { return NS_OK; }
> 
> Why isn't this also NS_ERROR_NOT_IMPLEMENTED?

I just followed SetIsOpaque as returning NS_OK, but I think you're right; SetIsOpaque is strictly an optimization, whereas SetIsShmem is a correctness thing. This should return NOT_IMPLEMENTED.

> >diff --git a/content/canvas/src/nsCanvasRenderingContext2D.cpp b/content/canvas/src/nsCanvasRenderingContext2D.cpp
> >--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp
> >+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp
> >+static void
> >+CopyContext(gfxContext* dest, gfxContext* src)
> >+{
> >+    dest->Multiply(src->CurrentMatrix());
> >+
> >+    nsRefPtr<gfxPath> path = src->CopyPath();
> >+    dest->NewPath();
> >+    dest->AppendPath(path);
> >+
> >+    nsRefPtr<gfxPattern> pattern = src->GetPattern();
> >+    dest->SetPattern(pattern);
> >+
> >+    dest->SetLineWidth(src->CurrentLineWidth());
> >+    dest->SetLineCap(src->CurrentLineCap());
> >+    dest->SetLineJoin(src->CurrentLineJoin());
> >+    dest->SetMiterLimit(src->CurrentMiterLimit());
> >+    dest->SetFillRule(src->CurrentFillRule());
> >+
> >+    dest->SetAntialiasMode(src->CurrentAntialiasMode());
> >+}
> 
> Seems cleaner to me to make a gfxContext::Clone() method or something
> to do this (dunno if that's possible).

This was just moved from another place, but after some discussion with Jeff we've decided that, since all I need is the matrix, I should just use the matrix, and move this code back to where it was.

Comment 15

[Doug Turner (:dougt)]

Attachment: implement MozGetShmemContext, v3

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 434038 [details] [diff] [review]
implement MozGetShmemContext, v3

this includes cjones comments.  I didn't change things over to use rects or add a gfxContext::Clone.  i'll let Joe think about that.

Comment 17

[Doug Turner (:dougt)]

Attachment: implement MozGetShmemContext, v3

Comment 18

[Doug Turner (:dougt)]

>+    async Dummy(Shmem foo);

^^^^^^ is still needed for xshm creation.  why, not sure.

Comment 19

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

Otherwise the IPDL compiler doesn't think that the protocol "uses shmem", and so doesn't create a handler for the SHMEM_CREATED_MESSAGE_TYPE.

joe's idea was to write |using Shmem;| instead, which is better, but obviously we never implemented that :(.

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/projects/electrolysis/rev/ee2709f390d6
http://hg.mozilla.org/projects/electrolysis/rev/7d65393c8c73
http://hg.mozilla.org/projects/electrolysis/rev/d40a32fe925e

Comment 21

[Josh Matthews [:jdm]]

Attachment: Build fix for non-MOZ_IPC

Mac builds keep burning.  Let's clear that up, shall we?

Comment 22

[Doug Turner (:dougt)]

http://hg.mozilla.org/projects/electrolysis/rev/87c518f10414

Thanks jdm for the followup!

Comment 23

[Josh Matthews [:jdm]]

Attachment: More build fixes

The try server tells me that OS X no longer burns with this final build fix.