Implement async drawWindow/Element() using shared memory

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: joe)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

Comment hidden (empty)
Assignee: nobody → joe
Blocks: 505847, 523170
No longer depends on: 505847

Updated

9 years ago
Blocks: 516521
(Assignee)

Comment 1

9 years ago
Created attachment 425580 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Depends on: 544607
(Assignee)

Comment 2

9 years ago
Created attachment 426019 [details] [diff] [review]
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. :)
Attachment #425580 - Attachment is obsolete: true
Attachment #426019 - Flags: review?(jones.chris.g)
Attachment #426019 - Flags: review?(benjamin)
(Assignee)

Comment 3

9 years ago
Created attachment 426020 [details] [diff] [review]
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.
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'
(Assignee)

Comment 5

9 years ago
As I mentioned in comment 3, you need the changes in the dependent bugs. This includes dependent bug 544607 and dependent bug 544623.
(Assignee)

Updated

9 years ago
Depends on: 547881
(Assignee)

Comment 6

9 years ago
Created attachment 428329 [details] [diff] [review]
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.
Attachment #426019 - Attachment is obsolete: true
Attachment #428329 - Flags: review?(benjamin)
Attachment #426019 - Flags: review?(jones.chris.g)
Attachment #426019 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Blocks: 548437
(Assignee)

Comment 7

9 years ago
Created attachment 428825 [details] [diff] [review]
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.
Attachment #428825 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #428825 - Flags: review? → review?(Olli.Pettay)
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
Attachment #428825 - Flags: review?(Olli.Pettay) → review+
joe - please protect uses of mShmem with #ifdef MOZ_IPC.  If you compile without IPC enabled, you will get compilation errors.
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.
Attachment #428329 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Attachment #428329 - Flags: review?(jmuizelaar)
(Assignee)

Updated

9 years ago
Depends on: 550337
Attachment #428329 - Flags: review?(jones.chris.g)
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.
Sorry, by "trunk" I meant "e10s" ;).
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.
Attachment #428329 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 14

9 years ago
(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.
Created attachment 434038 [details] [diff] [review]
implement MozGetShmemContext, v3
Attachment #434038 - Flags: review?
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.
Attachment #434038 - Flags: review?
Created attachment 434039 [details] [diff] [review]
implement MozGetShmemContext, v3
Attachment #434038 - Attachment is obsolete: true
Attachment #434039 - Flags: review?(joe)
>+    async Dummy(Shmem foo);

^^^^^^ is still needed for xshm creation.  why, not sure.
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 :(.
Created attachment 434691 [details] [diff] [review]
Build fix for non-MOZ_IPC

Mac builds keep burning.  Let's clear that up, shall we?
Attachment #434691 - Attachment is patch: true
Attachment #434691 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

9 years ago
Attachment #434691 - Flags: review+
Created attachment 434767 [details] [diff] [review]
More build fixes

The try server tells me that OS X no longer burns with this final build fix.
Attachment #434039 - Flags: review?(joe) → review-
Attachment #428329 - Flags: review?(jmuizelaar)
You need to log in before you can comment on or make changes to this bug.