548437 - Implement a SysV shared memory backend for Shmem and allow them to be used with Xshm

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Joe Drew (not getting mail)]

Right now, we get a big panning interactivity deficit from using image surfaces rather than X surfaces in the canvas. We need to use image surfaces because we're just wrapping the shared memory, but constantly reuploading the surfaces to the X server slows us down.

I have a hack that just copies our data over to an offscreen X surface when we draw to a canvas, but that's not optimal, since drawing to X can still take a while. Ideally, we'd just use Xshm surfaces, aka gfxSharedImageSurface. Unfortunately the mmap or posix shared memory that Chromium uses isn't compatible with the MIT shared memory that X uses.

Therefore, this bug is two-pronged: we need to be able to use MIT shared memory for IPDL Shmem, and we need to use those shared memory segments with Xshm.

Comment 1

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

I think we'll want the new IPDL interface:

  AllocShmem(Type t \in { Default, SysV } = Default);

and implement "SysV" in ipc/glue/SharedMemorySysv.cpp.

I presume we'll also need a SharedMemorySysv-specific method |key_t Key()| to interoperate with Xshm, but I'm not yet sure how to expose this through Shmem.

Comment 2

[Joe Drew (not getting mail)]

Chris, is your plan to implement this soon? I may look into it otherwise.

Comment 3

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

Wasn't high on my list, but if it's blocking something big it can change.  Please feel free to take.

The SysV shmem backend should be really easy, but there's some IPDL compiler work that's probably most efficiently done by me.

Comment 4

[Doug Turner (:dougt)]

Attachment: hack joe was talking about - should be backed out when we land a real fix

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Who reviewed the original SHM/IPC stuff? It seems like they would be a better reviewer for this.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 429767 [details] [diff] [review]
hack joe was talking about - should be backed out when we land a real fix

That'd be Benjamin Smedberg - though he hasn't yet reviewed it.

Comment 7

[Doug Turner (:dougt)]

In that case, can we land this and have it reviewed at the same time when the original SHM/IPC code is

Comment 8

[Joe Drew (not getting mail)]

This can't land without bug 524180, so no, it can't land yet :)

Comment 9

[Joe Drew (not getting mail)]

Attachment: Part 1: Support SysV shared memory as a type of SharedMemory

This patch makes mozilla::ipc::SharedMemory an abstract class, and creates two implementations of it - SharedMemorySysV and SharedMemoryChromium. This basically compiles, but I have not yet run it or done any testing of it, as I need ipdl compiler support.

There is no more SharedMemoryHandle. There are now two separate types: Shmem::SharedMemoryHandleSysV and Shmem::SharedMemoryHandleChromium. This requires a tiny bit of duplication of code, but it is easier-to-read code because of that.

There is a type, SharedMemory::Type, which enumerates the different types of shared memory.

We will need two separate messages in ipdl - "Here is a Chromium shared memory segment" and "Here is a SysV shared memory segment." The code to instantiate one is subtly different from the other; specifically, Chromium requires you call ShareToProcess, but SysV doesn't (creating a segment automatically shares it in the kernel).

There might be some things missing here - I don't have a handle() call on either of these classes, though I suspect for ipdl support one will be necessary.

I also have not accounted for deletion of shared memory, which is very important - if we do not delete SysV shared memory segments, they remain around forever. The right solution for this is probably to mark them as IPC_RMID after we shmat() it in the parent process. (This still leaves a small window where we could leak the segment if we are killed after shmget() but before shmctl(IPC_RMID). I don't know of a way around this.) Unfortunately, marking a shared memory segment as to be deleted means that on non-Linux hosts, you can't attach to it any more. This doesn't really matter to us right now, but might in the future.

Comment 10

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

I won't do a full review of this until I get it hooked into IPDL, but a quick first edit needs to be s/SharedMemoryChromium/SharedMemoryBasic/ or something.  That we're using code in chromium/ is just a stopgap.  We'll be dropping that soon-ish.

Re Sys V shmem on non-linux: we'll never care about that, so no worries there.  (Some kooks might want to utilize this on one of the obscure *NIXes that use X11, but that market is so small that it's a dontcare.)

Comment 11

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

Hmm, this patch also has the downside of not compiling on e10s latest.

Comment 12

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

Comment on attachment 434663 [details] [diff] [review]
Part 1: Support SysV shared memory as a type of SharedMemory

IPDL integration aside, this patch had some problems.  v2 coming up in a bit.

Comment 13

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

Attachment: Add a SysV Shmem backend and use that for <canvas>

Comment 14

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

FWIW this code wfm in (modified) ipc/ipdl/test/cxx/TestShmem and fennec browser.

Comment 15

[Joe Drew (not getting mail)]

Attachment: Fix shmem a little

This is my updated version of the patch. It contains a few changes, including adding an accessor for the SysV handle to Shmem, and automatically marking ipc segments as to be removed after we've attached to them.

The hunk in nsCanvasRenderingContext2D.cpp should probably be removed in favour of the next patch, but I'll do that at a later date.

Comment 16

[Joe Drew (not getting mail)]

Attachment: draw in async canvases using Xshm

This patch adds Xshm drawing support to canvas. It should (untested) also fall back on the previous hack on platforms that can't support Xshm, or if Xshm creation fails.

Comment 17

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

(In reply to comment #15)
> Created an attachment (id=436590) [details]
> and automatically marking ipc segments as to be removed after we've attached to them.
> 

Hmm ... I had

+  NS_OVERRIDE
+  virtual bool Create(size_t aNbytes)
+  {
[snip]
+    // NB: in this window, we're vulnerable to crashing and
+    // perma-leaking the segment
+    if (-1 == shmctl(id, IPC_RMID, NULL))
+      return false;

in the original patch.  I'm pretty sure that doing IPC_RMID in Map() is just going to fail in both the creating and receiving processes.

Comment 18

[Joe Drew (not getting mail)]

Once you shmat() to the segment, you can safely mark it as removed. Map() calls shmat() then shmctl(IPC_RMID).

Comment 19

[Doug Turner (:dougt)]

Fennec crashes with these patches:

###!!! ABORT: Alloc() segment size disagrees with OpenExisting()'s: file /home/dougt/builds/e10s/electrolysis/ipc/glue/Shmem.cpp, line 540

Comment 20

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

(In reply to comment #18)
> Once you shmat() to the segment, you can safely mark it as removed. Map() calls
> shmat() then shmctl(IPC_RMID).

Well, then something is odd here.  The version IPC_RMID'ing in Create() passed my tests locally (and no race conditions with IPC_RMID).  What bothers me about IPC_RMID in Map() is that that function is called in the receiving process, and my |man shctl| says for IPC_RMID

  The caller must  be  the  owner  or creator, or be privileged.

Also, what I meant in comment 17 is that if the IPC_RMID is succeeding in Create() in the creating process, then I would guess that it's failing in Map().  Any thoughts?

Comment 21

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

(In reply to comment #20)
>   The caller must  be  the  owner  or creator, or be privileged.
> 

Nm, re-reading the man page, I think "owner" in this context means uid/guid.  Still not sure what happens if one IPC_RMID's more than once.

Comment 22

[Doug Turner (:dougt)]

from IRC:

joe: yeah, please post a comment in the bug saying that we don't check to see if the offscreen surface we create is xlib, because if that's not the case we need to create the front and back surfaces


The default render type on Qt right now is image surfaces.  A build using that will reproduce the problem.

Comment 23

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

If it's OK with all involved parties, I'd like to move the nsCanvasContext2d hunks into the second patch so that the first can land on m-c.

Comment 24

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

Comment on attachment 436590 [details] [diff] [review]
Fix shmem a little

Found two minor nits, fixed locally.  I also added a test that I'm going to push to m-c along with this.

Comment 25

[Joe Drew (not getting mail)]

Definitely move that hunk.

Comment 26

[Joe Drew (not getting mail)]

As for IPC_RMID - I don't know precisely. You can definitely rmid more than once (at worst the second will fail). I can see your point with rmid-ing in map, but we can't rmid until we've shmat-Ed, so I don't know what alternative we have. I suppose we could rmid only if we create, but that introduces extra complexity.

Comment 27

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

Attachment: Updated version of joe's patch

Comment 28

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

Attachment: Test for SysV shmem [Checkin: Comment 53]

Comment 29

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

(In reply to comment #26)
> As for IPC_RMID - I don't know precisely. You can definitely rmid more than
> once (at worst the second will fail). I can see your point with rmid-ing in
> map, but we can't rmid until we've shmat-Ed, so I don't know what alternative
> we have. I suppose we could rmid only if we create, but that introduces extra
> complexity.

Yeah, I'll assume that the fact that rmid in Create() was working locally is a fluke we shouldn't rely on.  The latest version of this patch removes IPC_RMID in Create().

Comment 30

[Doug Turner (:dougt)]

If shmat fails, do you need to call before return false?:

shmctl(mHandle, IPC_RMID, 0);

It seems that we do that here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#5176



Drop the fprint (or create an assertion)
+      fprintf(stderr, "shmat(): %s (%d)\n", strerror(errno), errno);



if mData == nsnull, i think your destructor will crash here:
+    shmdt(mData);

Probably want to protect with a nullcheck?

+  if (SharedMemorySysV::IsHandleValid(aHandle)) {
+    segment = new SharedMemorySysV(aHandle);
+  }
+  else {
+    segment = new SharedMemorySysV();

Test for null?  Or do we not do that anymore?



As i mentioned before:

   // this is the only validity check done OPT builds
-  if (aNBytes != *PtrToSize(segment))
+  if (size != *PtrToSize(segment))
     NS_RUNTIMEABORT("Alloc() segment size disagrees with OpenExisting()'s");

this assert is failing on device.  not sure why.

Comment 31

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

(In reply to comment #30)
> If shmat fails, do you need to call before return false?:
> 
> shmctl(mHandle, IPC_RMID, 0);
> 

Oh, good catch.

> Drop the fprint (or create an assertion)
> +      fprintf(stderr, "shmat(): %s (%d)\n", strerror(errno), errno);
> 

We can't assert here because shmat() might fail if there's not enough contiguous address space (sort of like needing to null-check a large memory allocation).  I think what we probably want to do is take the following actions if Map() fails in

 - creating process: return NULL from AllocShmem()
 - receiving child process: NS_RUNTIMEABORT()
 - receiving parent process: terminate child

I think this logic is best implemented outside of SharedMemorySysV (pretty sure we already do the first thing above, second two I'm not sure of.)

> if mData == nsnull, i think your destructor will crash here:
> +    shmdt(mData);
> 
> Probably want to protect with a nullcheck?
> 

By the letter of the man page, this should return -1/EINVAL.  But I don't mind null checking here.

> +  if (SharedMemorySysV::IsHandleValid(aHandle)) {
> +    segment = new SharedMemorySysV(aHandle);
> +  }
> +  else {
> +    segment = new SharedMemorySysV();
> 
> Test for null?  Or do we not do that anymore?
> 

No need, ::operator new() never returns NULL.

> As i mentioned before:
> 
>    // this is the only validity check done OPT builds
> -  if (aNBytes != *PtrToSize(segment))
> +  if (size != *PtrToSize(segment))
>      NS_RUNTIMEABORT("Alloc() segment size disagrees with OpenExisting()'s");
> 
> this assert is failing on device.  not sure why.

Very annoying :(.  Will try to look into this tonight.

Comment 32

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

Thought: I wonder if X is zeroing out this segment behind our backs?  Having a little trouble with my local build, but I'll test this assuming I can get it straightened out.

Comment 33

[Joe Drew (not getting mail)]

It probably comes out of shmat() zeroed, just because that's faster for the OS to do, but X doesn't appear to zero anything.

http://cgit.freedesktop.org/xorg/xserver/tree/Xext/shm.c#n431
http://cgit.freedesktop.org/xorg/xserver/tree/Xext/shm.c#n848

(The actual X extension, available at http://cgit.freedesktop.org/xorg/lib/libXext/tree/src/XShm.c , is mostly uninteresting, as it just calls the implementation in the X server.

Comment 34

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

(In reply to comment #33)
> It probably comes out of shmat() zeroed, just because that's faster for the OS
> to do, but X doesn't appear to zero anything.
> 

That's fine, we write the "user size" at the back of the buffer after shmat().  (Maybe "size" and "capacity" would be better terms?  "Size" being what the user requested and "capacity" being the page-aligned size.)

Comment 35

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

After a few hours of wading through XRE bullshit on the n900 (long story), I'm exiting from some apparent failure to load a library in glibc.  Will resume later.

Comment 36

[Oleg Romashin (:romaxa)]

> > -  if (aNBytes != *PtrToSize(segment))
> > +  if (size != *PtrToSize(segment))
> >      NS_RUNTIMEABORT("Alloc() segment size disagrees with OpenExisting()'s");
> > 
> > this assert is failing on device.  not sure why.
> 
> Very annoying :(.  Will try to look into this tonight.

This assert is also failing on desktop:
mozilla::ipc::Shmem::PtrToSize(mozilla::ipc::SharedMemory*)::259
sz:1048576, stS:0xb00ff000, enS:0xb01ff000, res:0xb01feffc, res:0
0xb01ff000 - 0xb00ff000 = 1048576, 
reinterpret_cast<size_t*>(enS - sizeof(size_t)) = 0xb01feffc;
*0xb01feffc = 0

Comment 37

[Oleg Romashin (:romaxa)]

I tried to remove that check, because it seems useless for me, and then parent process crashes here:

#0  0xb73bddec in gfxPattern (this=0xaf7152b0, surface=0x0)
    at gfx/thebes/src/gfxPattern.cpp:58
#1  0xb6bc72c3 in nsCanvasRenderingContext2D::Swap (this=0xb12ba4e0, aBack=..., x=0, y=0, w=516, h=516)
    at content/canvas/src/nsCanvasRenderingContext2D.cpp:1186
#2  0xb6bc920e in mozilla::ipc::DocumentRendererShmemParent::Recv__delete__ (this=0xaf7500e0, x=@0xbfc416e8, y=@0xbfc416e4, 
    w=@0xbfc416e0, h=@0xbfc416dc, data=...)
    at content/canvas/src/DocumentRendererShmemParent.cpp:58
#3  0xb7241a77 in mozilla::ipc::PDocumentRendererShmemParent::OnMessageReceived (this=0xaf7500e0, msg=...)
    at PDocumentRendererShmemParent.cpp:107
#4  0xb723b1c6 in mozilla::dom::PContentProcessParent::OnMessageReceived (this=0xb307c030, msg=...)
    at PContentProcessParent.cpp:267

Comment 38

[Oleg Romashin (:romaxa)]

I think it happen because I'm not using X-surface for rendering, but using QT build, same as dougt, and in Qt we are using gfxImageSurface for rendering.

Comment 39

[Joe Drew (not getting mail)]

Attachment: fix non-debug build [Checkin: Comment 53]

Doug and Oleg noticed that we had a couple of problems with the SysV support patch. We weren't taking into account the extra size_t that we store to note the "real" size, so we were reading the wrong part of the shared memory segment to check its size. This patch fixes that.

Comment 40

[Joe Drew (not getting mail)]

Attachment: Fix shmem with non-x11 offscreen surface

This patch makes non-X11 offscreen surfaces not crash. Note that you won't get the speed benefit if you run image or QPainter-based offscreen surfaces.

Comment 41

[Oleg Romashin (:romaxa)]

Comment on attachment 438271 [details] [diff] [review]
Fix shmem with non-x11 offscreen surface


>+        nsRefPtr<gfxPattern> pat = new gfxPattern(mFrontSurface);
> 
>-    nsRefPtr<gfxContext> ctx = new gfxContext(mSurface);
>-    CopyContext(ctx, mThebes);
>-    mThebes = ctx;

instead
****************
>+        mThebes->NewPath();
>+        mThebes->PixelSnappedRectangleAndSetPattern(gfxRect(x, y, w, h), pat);
>+        mThebes->Fill();
****************
I would prefer to use this
----------------
          mThebes->NewPath();
          gfxContext::GraphicsOperator op = mThebes->CurrentOperator();
          if (mOpaque)
              mThebes->SetOperator(gfxContext::OPERATOR_SOURCE);
          mThebes->SetSource(mFrontSurface);
          mThebes->Rectangle(gfxRect(x, y, w, h), PR_TRUE);
          mThebes->Clip();
          mThebes->Paint();
          if (mOpaque)
              mThebes->SetOperator(op);
----------------
By some reason it is choosing better pixman path, with more optimized rendering


> ifdef MOZ_DFB
> CPPSRCS += gfxDirectFBSurface.cpp
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>-CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp gfxSharedImageSurface.cpp
>+CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp
This is bad change, it breaks qt compilation


But in general it seems to work fine.

Comment 42

[Joe Drew (not getting mail)]

(In reply to comment #41)
> (From update of attachment 438271 [details] [diff] [review])

> instead
> ****************
> >+        mThebes->NewPath();
> >+        mThebes->PixelSnappedRectangleAndSetPattern(gfxRect(x, y, w, h), pat);
> >+        mThebes->Fill();
> ****************
> I would prefer to use this
> ----------------
>           mThebes->NewPath();
>           gfxContext::GraphicsOperator op = mThebes->CurrentOperator();
>           if (mOpaque)
>               mThebes->SetOperator(gfxContext::OPERATOR_SOURCE);
>           mThebes->SetSource(mFrontSurface);
>           mThebes->Rectangle(gfxRect(x, y, w, h), PR_TRUE);
>           mThebes->Clip();
>           mThebes->Paint();
>           if (mOpaque)
>               mThebes->SetOperator(op);
> ----------------
> By some reason it is choosing better pixman path, with more optimized rendering

What's better about it? What was it choosing before, and what does it choose with this change?

> > ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
> >-CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp gfxSharedImageSurface.cpp
> >+CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp
> This is bad change, it breaks qt compilation

We need gfxSharedImageSurface always now, not just in Qt. So find a place to put it so that works. :)

Comment 43

[Oleg Romashin (:romaxa)]

> What's better about it? What was it choosing before, and what does it choose
> with this change?
OPERATOR_SOURCE will avoid over operation when it is not needed.

PixelSnappedRectangleAndSetPattern(gfxRect(x, y, w, h), pat); - has problem with scaling and pixel snapping... see bug 558087
Also when we are using simple pattern it is choosing pixman general_composite (slow path)
Also I don't know yet why, see and 558308

> 
> > > ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
> > >-CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp gfxSharedImageSurface.cpp
> > >+CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp
> > This is bad change, it breaks qt compilation
> 
> We need gfxSharedImageSurface always now, not just in Qt. So find a place to
> put it so that works. :)

It is already under Qt ifeq, just don't remove it... it is ok to add it for gtk ifeq build.

Comment 44

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

Comment on attachment 438270 [details] [diff] [review]
fix non-debug build [Checkin: Comment 53]

>+#endif // ifndef mozilla_ipc_SharedMemoryBasic_h
>diff --git a/ipc/glue/SharedMemorySysV.h b/ipc/glue/SharedMemorySysV.h
>new file mode 100644
>--- /dev/null
>+++ b/ipc/glue/SharedMemorySysV.h
>+  NS_OVERRIDE
>+  virtual bool Create(size_t aNbytes)
>+  {
>+    int id = shmget(IPC_PRIVATE, aNbytes, IPC_CREAT | 0666);

That should be 0600.

Comment 45

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

Comment on attachment 438271 [details] [diff] [review]
Fix shmem with non-x11 offscreen surface

>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
>@@ -455,18 +465,24 @@ protected:
> 
> #ifdef MOZ_IPC
>     PRPackedBool mShmem;
> 
>     // We always have a front buffer. We hand the back buffer to the other
>     // process to render to, and then swap our two buffers when it finishes.

It'd be really nice to have a more detailed comment giving an overview
of the new scheme.

>     mozilla::ipc::Shmem mFrontBuffer;
>     mozilla::ipc::Shmem mBackBuffer;
>+    nsRefPtr<gfxASurface> mFrontSurface;
>     nsRefPtr<gfxASurface> mBackSurface;
> 
>+#ifdef MOZ_X11
>+    gfxSharedImageSurface* mFrontSharedImage;
>+    gfxSharedImageSurface* mBackSharedImage;
>+#endif
>+

It's not clear to me in what circumstances we would fall back on
mFrontSurface/mBackSurface on X11.  Are you thinking of non-linux
kernels that lack reasonable sysv shmem support?

It seems like this ought to be

#if defined(MOZ_X11) && defined(MOZ_HAVE_SHAREDMEMORYSYSV)
#  define USE_XSHM
#endif
//...
#ifdef USE_XSHM
    gfxSharedImageSurface* mFrontSharedImage;
    gfxSharedImageSurface* mBackSharedImage;
#else
    nsRefPtr<gfxASurface> mFrontSurface;
    nsRefPtr<gfxASurface> mBackSurface;
#endif

And then guard on USE_XSHM in the implementation below.  That should
simplify the implementation logic somewhat.

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

This will fail to compile without MOZ_HAVE_SHAREDMEMORYSYSV.

>+#ifdef MOZ_IPC
>+        if (mShmem && CreateShmemSegments(width, height, format)) {
>+            bool ok = false;
>+
>+#ifdef MOZ_X11
>+            if (surface->GetType() == gfxASurface::SurfaceTypeXlib) {
>+                mFrontSharedImage = new gfxSharedImageSurface();
>+                ok = mFrontSharedImage->Init(gfxIntSize(width, height), format,
>+                                             mFrontBuffer.GetSysVID());
>+
>+                mBackSharedImage = new gfxSharedImageSurface();
>+                ok = ok && mBackSharedImage->Init(gfxIntSize(width, height), format,
>+                                                  mBackBuffer.GetSysVID());
>+                if (!ok) {
>+                    delete mFrontSharedImage;
>+                    mFrontSharedImage = nsnull;
>+                    delete mBackSharedImage;
>+                    mBackSharedImage = nsnull;
>+                }
>+            }
>+#endif
>+            if (!ok) {
>+                mBackSurface = new gfxImageSurface(mBackBuffer.get<unsigned char>(),
>+                                                   gfxIntSize(width, height),
>+                                                   width * 4, format);
>+                mFrontSurface = new gfxImageSurface(mFrontBuffer.get<unsigned char>(),
>+                                                    gfxIntSize(width, height),
>+                                                    width * 4, format);
>+            }
>+        }
>+#endif
>+

Per above, I think this would be simpler if switched on USE_XSHM.

>+#ifdef MOZ_X11
>+    else {
>+        // We should never be able to get into this situation; mFrontSurface
>+        // and mBackSurface are null iff we aren't on X or we couldn't allocate
>+        // or init gfxSharedImageSurfaces.
>+        NS_ABORT_IF_FALSE(mSurface->GetType() == gfxASurface::SurfaceTypeXlib,
>+                          "Trying to use a non-X surface with X functions!");
>+        NS_ABORT_IF_FALSE(mFrontSharedImage && mBackSharedImage,
>+                          "Don't have shared images to draw from and to!");
>+
>+        gfxXlibSurface *xsurf = static_cast<gfxXlibSurface *>(mSurface.get());
>+
>+        GC gc = XCreateGC(xsurf->XDisplay(), xsurf->XDrawable(), 0, nsnull);
>+        XShmPutImage(xsurf->XDisplay(), xsurf->XDrawable(), gc,
>+                     mBackSharedImage->image(), x, y, x, y, mWidth, mHeight, False);
>+        XFreeGC(xsurf->XDisplay(), gc);
>+
>+        gfxSharedImageSurface* tmp = mBackSharedImage;
>+        mBackSharedImage = mFrontSharedImage;
>+        mFrontSharedImage = tmp;

(Psst: I'll r+ a patch that uses <algorithm> and std::swap in MOZ_IPC
code ;) .  Don't tell anyone.)

Would like to glance at v2.

Comment 46

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

Comment on attachment 438271 [details] [diff] [review]
Fix shmem with non-x11 offscreen surface

Nm, Karl points out that remote displays are a case where XShm will fail, doy.

r=me with the more detailed comment and compilation problem fixed.

Comment 47

[Doug Turner (:dougt)]

Comment on attachment 438270 [details] [diff] [review]
fix non-debug build [Checkin: Comment 53]

+  virtual ~SharedMemorySysV()
+  {
+    shmdt(mData);

Without a null check, this will blow up if mData is null.



>- * Portions created by the Initial Developer are Copyright (C) 2009
>+ * Portions created by the Initial Developer are Copyright (C) 2010

not both years?


>+  NS_OVERRIDE
>+  virtual bool Create(size_t aNbytes)
>+  {
>+    int id = shmget(IPC_PRIVATE, aNbytes, IPC_CREAT | 0666);

0600

(
and tell me why this is the way it is:
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/x11_util.cc#49
)
>+    if (mem == (void*) -1) {
>+      fprintf(stderr, "shmat(): %s (%d)\n", strerror(errno), errno);
>+      return false;

don't use fprintf.  Use NS_WARNING()


>+
>+  if (SharedMemory::TYPE_BASIC == type) {
>+    SharedMemoryBasic::Handle handle;
>+    if (!ShmemCreated::ReadHandle(&aDescriptor, &iter, &handle))
>+      return 0;
>+
>+    if (!SharedMemoryBasic::IsHandleValid(handle))
>+      NS_RUNTIMEABORT("trying to open invalid handle");
>+    segment = CreateSegment(segmentSize, handle);
>+  }
>+#ifdef MOZ_HAVE_SHAREDMEMORYSYSV
>+  else if (SharedMemory::TYPE_SYSV == type) {
>+    SharedMemorySysV::Handle handle;
>+    if (!ShmemCreated::ReadHandle(&aDescriptor, &iter, &handle))
>+      return 0;
>+
>+    if (!SharedMemorySysV::IsHandleValid(handle))
>+      NS_RUNTIMEABORT("trying to open invalid handle");
>+    segment = CreateSegment(segmentSize, handle);
>+  }
>+#endif


Did you consider making |IsHandleValid| be part of the |SharedMemory| base class so that you can move that check outside of each test?



>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py

I really do not understand these changes.


>diff --git a/ipc/ipdl/test/cxx/TestShmem.cpp b/ipc/ipdl/test/cxx/TestShmem.cpp
>--- a/ipc/ipdl/test/cxx/TestShmem.cpp
>+++ b/ipc/ipdl/test/cxx/TestShmem.cpp
>@@ -9,17 +9,17 @@ namespace _ipdltest {
> //-----------------------------------------------------------------------------
> // Parent
> 
> void
> TestShmemParent::Main()
> {
>     Shmem mem;
>     size_t size = 12345;
>-    if (!AllocShmem(size, &mem))
>+    if (!AllocShmem(size, SharedMemory::TYPE_BASIC, &mem)


Tests for the other type of memory?

looks + works great.

Comment 48

[Joe Drew (not getting mail)]

(In reply to comment #47)
> Did you consider making |IsHandleValid| be part of the |SharedMemory| base
> class so that you can move that check outside of each test?

Yes, yes, a thousand times yes, but it's impossible. (I spent probably 3 full work days trying different ways of getting this to work.) They're different, incompatible types, so you can't actually define a virtual function for IsHandleValid.

(Chris - one thought I just had was creating a templated base class function that then calls the undefined function IsHandleValidImpl(aHandle). I don't know if that's actually better, but I think it'd work.)

> >diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
> 
> I really do not understand these changes.

Me neither. Probably Ben Turner should review?

Comment 49

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

(In reply to comment #48)
> (In reply to comment #47)
> (Chris - one thought I just had was creating a templated base class function
> that then calls the undefined function IsHandleValidImpl(aHandle). I don't know
> if that's actually better, but I think it'd work.)
> 

Templated on what?

> > >diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
> > 
> > I really do not understand these changes.
> 
> Me neither. Probably Ben Turner should review?

Meh, these changes are pretty trivial, and I have decent tests.  He has more important IPDL-compiler patches to review ;).

Comment 50

[Joe Drew (not getting mail)]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > (Chris - one thought I just had was creating a templated base class function
> > that then calls the undefined function IsHandleValidImpl(aHandle). I don't know
> > if that's actually better, but I think it'd work.)
> > 
> 
> Templated on what?

template <typename T>
bool IsHandleValid(T aHandle)
{
    return IsHandleValidImpl(aHandle);
}

Comment 51

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

Then you'd also need a templated virtual IsHandleValidImpl() declaration, and we wouldn't catch type mismatches until link time (with undefined symbols).

Comment 52

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

(In reply to comment #47)
> >+  NS_OVERRIDE
> >+  virtual bool Create(size_t aNbytes)
> >+  {
> >+    int id = shmget(IPC_PRIVATE, aNbytes, IPC_CREAT | 0666);
> 
> 0600
> 
> (
> and tell me why this is the way it is:
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/x11_util.cc#49
> )

I have no clue, to be honest.  0600 is what we want and it works, so I'm OK leaving things be until we find a reason to change it.

> >+    if (mem == (void*) -1) {
> >+      fprintf(stderr, "shmat(): %s (%d)\n", strerror(errno), errno);
> >+      return false;
> 
> don't use fprintf.  Use NS_WARNING()
> 

FWIW fprintf was used here because we want a formatted string, which NS_WARNING doesn't support.  I changed this locally to use snprintf() + NS_WARNING().

> 
> >diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
> 
> I really do not understand these changes.
> 
> 
> >diff --git a/ipc/ipdl/test/cxx/TestShmem.cpp b/ipc/ipdl/test/cxx/TestShmem.cpp
> >--- a/ipc/ipdl/test/cxx/TestShmem.cpp
> >+++ b/ipc/ipdl/test/cxx/TestShmem.cpp
> >@@ -9,17 +9,17 @@ namespace _ipdltest {
> > //-----------------------------------------------------------------------------
> > // Parent
> > 
> > void
> > TestShmemParent::Main()
> > {
> >     Shmem mem;
> >     size_t size = 12345;
> >-    if (!AllocShmem(size, &mem))
> >+    if (!AllocShmem(size, SharedMemory::TYPE_BASIC, &mem)
> 
> 
> Tests for the other type of memory?
> 

Yep, see the list of attachments.

Comment 53

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

http://hg.mozilla.org/mozilla-central/rev/4116478e2002
http://hg.mozilla.org/mozilla-central/rev/41ceb429c51d

Leaving open for the bits that need to land on e10s.

Comment 54

[Joe Drew (not getting mail)]

Attachment: make async draw work with Qt's shared image drawing

This patch should handle both the Xlib surfaces passed out from CreateOffscreenSurface() by the GTK widget backend, by using XShm, and the image surfaces handed out by the Qt widget backend. We do the latter by just using shared memory image surfaces between content and chrome, and blitting to the shared memory surface that the Qt widget backend's nsWindow::DoPaint() passes us. (It then usees XShm to draw to the X server, so we get all the same speed benefits.)

Comment 55

[Joe Drew (not getting mail)]

Attachment: support 16bpp in canvas XShm support

This adds 16bpp support to canvas's shared memory support. We unconditionally set our format to RGB16_565 when optimizing for mobile; this doesn't feel right, but I don't see a better way around it.

This patch depends on a couple of the patches in bug 545632; in particular, the Thebes 16bpp support and the Cairo 16bpp support (that will get rolled into Cairo proper once bug 542605 lands).

Comment 56

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

none of the IPC stuff is in canvas on the trunk; what bug is that patch in?

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 441208 [details] [diff] [review]
support 16bpp in canvas XShm support

No, you can't unconditionally set RGB16 here.  Ideally we'd use the context attributes stuff (which isn't checked in yet) to allow you to specify this at getContext time.  For now, hardcode specific platforms (e.g. Maemo), not the generic optimize mobile flag.

Comment 58

[Joe Drew (not getting mail)]

This is all in the electrolysis branch. Look in dependent bug 524180 and its dependent bug 505847 for more of its lineage.

Comment 59

[Joe Drew (not getting mail)]

Attachment: support 16bpp in canvas XShm support, v2

The net interdiff of this is more or less

+#if defined(MOZ_WIDGET_QT) && defined(MOZ_X11)
+#include <QX11Info>
+#endif

and

-#ifdef MOZ_GFX_OPTIMIZE_MOBILE
+#if defined(MOZ_WIDGET_QT) && defined(MOZ_X11)
+            if (QX11Info().depth() == 16)
+                format = gfxASurface::ImageFormatRGB16_565;
+#elif defined(MOZ_PLATFORM_MAEMO)
             format = gfxASurface::ImageFormatRGB16_565;
 #else
             format = gfxASurface::ImageFormatRGB24;
 #endif

Comment 60

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

Comment on attachment 441570 [details] [diff] [review]
support 16bpp in canvas XShm support, v2

>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
>@@ -999,41 +1003,51 @@ nsCanvasRenderingContext2D::Redraw(const
>     return mCanvasElement->InvalidateFrameSubrect(r);
> }
> 
> #ifdef MOZ_IPC
> bool
> nsCanvasRenderingContext2D::CreateShmemSegments(PRInt32 width, PRInt32 height,
>                                                 gfxASurface::gfxImageFormat format)
> {
>+    int bytes = gfxASurface::BytePerPixelFromFormat(format);
>+
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-        AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>+        AllocShmem(width * height * bytes, SharedMemory::TYPE_SYSV,
>                    &mFrontBuffer))
>         return false;
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-                AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>-                           &mBackBuffer))
>+        AllocShmem(width * height * bytes, SharedMemory::TYPE_SYSV,
>+                   &mBackBuffer))
>         return false;
> 

This isn't going to compile for non-HAVE_SHAREDMEMORYSYSV platforms,
not sure how much we care atm.

>@@ -3684,16 +3700,18 @@ nsCanvasRenderingContext2D::AsyncDrawXUL
>             return NS_ERROR_FAILURE;
> 
>         mozilla::ipc::PDocumentRendererShmemParent *pdocrender =
>             child->SendPDocumentRendererShmemConstructor(x, y, w, h,
>                                                          nsString(aBGColor),
>                                                          renderDocFlags, flush,
>                                                          mThebes->CurrentMatrix(),
>                                                          mWidth, mHeight,
>+                                                         /// XXX DO NOT HARD CODE
>+                                                         gfxASurface::ImageFormatRGB16_565,

Oops?  I'm not sure what's going on here.

>diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h
>--- a/ipc/glue/IPCMessageUtils.h
>+++ b/ipc/glue/IPCMessageUtils.h
>+template<>
>+struct ParamTraits<gfxASurface::gfxImageFormat>
>+{
>+  typedef gfxASurface::gfxImageFormat paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    aMsg->WriteBytes(&aParam, sizeof(paramType));
>+  }
>+
>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    const char* out;
>+    if (!aMsg->ReadBytes(aIter, &out, sizeof(paramType)))
>+      return false;
>+    *aResult = *reinterpret_cast<const paramType*>(out);
>+    return true;
>+  }
>+

This looks weird to me ... isn't the format an enum?  If so, you
should serialize it as int32_t and check that the format value is
known.

I'd like to look at an updated patch.

Comment 61

[Joe Drew (not getting mail)]

Attachment: support 16bpp in canvas XShm support, v3

Fixed problems and addressed comments brought up by Chris. Also added a sanity check to child-side code to ensure that the format we've been passed actually is the format we want; i.e., that the size we calculate for the buffer actually is its size.

Comment 62

[Joe Drew (not getting mail)]

Attachment: support 16bpp in canvas XShm support, v4

Previous version was the same as V2 because I had another patch applied when I qrefreshed.

Comment 63

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

Comment on attachment 441206 [details] [diff] [review]
make async draw work with Qt's shared image drawing

>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
>@@ -456,18 +465,24 @@ protected:
> 
> #ifdef MOZ_IPC
>     PRPackedBool mShmem;
> 

Nit: maybe s/mShmem/mUseShmem/?  Not a big deal.

>     // We always have a front buffer. We hand the back buffer to the other
>     // process to render to, and then swap our two buffers when it finishes.
>     mozilla::ipc::Shmem mFrontBuffer;
>     mozilla::ipc::Shmem mBackBuffer;
>+    nsRefPtr<gfxASurface> mFrontSurface;
>     nsRefPtr<gfxASurface> mBackSurface;
> 
>+#ifdef MOZ_X11
>+    gfxSharedImageSurface* mFrontSharedImage;
>+    gfxSharedImageSurface* mBackSharedImage;
>+#endif
>+

Would like a comment describing the relationship of
m*Buffer/m*Surface/m*SharedImage.

>@@ -975,28 +1000,24 @@ nsCanvasRenderingContext2D::Redraw(const
> }
> 
> #ifdef MOZ_IPC
> bool
> nsCanvasRenderingContext2D::CreateShmemSegments(PRInt32 width, PRInt32 height,
>                                                 gfxASurface::gfxImageFormat format)
> {
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-        AllocShmem(width * height * 4, SharedMemory::TYPE_BASIC,
>+        AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>                    &mFrontBuffer))
>         return false;
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-                AllocShmem(width * height * 4, SharedMemory::TYPE_BASIC,
>+                AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>                            &mBackBuffer))
>         return false;
> 

I've made milquetoast-y comments about this a couple of times, but I
think we need to fix this, or else e10s non-linux tinderbox and later
android will burn.  Probably something like

  SharedMemory::Type type =
#ifdef HAVE_SYSV
    SharedMemory::TYPE_SYSV
#else
    SharedMemory::TYPE_BASIC
#endif
    ;

  if (!...AllocShmem(..., type))

ought to suffice.

> NS_IMETHODIMP
> nsCanvasRenderingContext2D::SetDimensions(PRInt32 width, PRInt32 height)
> {
>     Destroy();
>@@ -1004,29 +1025,57 @@ nsCanvasRenderingContext2D::SetDimension
>     nsRefPtr<gfxASurface> surface;
> 
>     // Check that the dimensions are sane
>     if (gfxASurface::CheckSurfaceSize(gfxIntSize(width, height), 0xffff)) {
>         gfxASurface::gfxImageFormat format = gfxASurface::ImageFormatARGB32;
>         if (mOpaque)
>             format = gfxASurface::ImageFormatRGB24;
> 
>+        surface = gfxPlatform::GetPlatform()->CreateOffscreenSurface
>+            (gfxIntSize(width, height), format);
>+
>+        if (surface && surface->CairoStatus() != 0)
>+            surface = nsnull;
>+
> #ifdef MOZ_IPC
>         if (mShmem && CreateShmemSegments(width, height, format)) {
>+            bool ok = false;
>+
>+#ifdef MOZ_X11
>+            if (surface->GetType() == gfxASurface::SurfaceTypeXlib) {
>+                mFrontSharedImage = new gfxSharedImageSurface();
>+                ok = mFrontSharedImage->Init(gfxIntSize(width, height), format,
>+                                             mFrontBuffer.GetSysVID());
>+
>+                mBackSharedImage = new gfxSharedImageSurface();
>+                ok = ok && mBackSharedImage->Init(gfxIntSize(width, height), format,
>+                                                  mBackBuffer.GetSysVID());
>+                if (!ok) {
>+                    delete mFrontSharedImage;
>+                    mFrontSharedImage = nsnull;
>+                    delete mBackSharedImage;
>+                    mBackSharedImage = nsnull;
>+                }
>+            }
> #endif
>+            if (!ok) {
>+                mBackSurface = new gfxImageSurface(mBackBuffer.get<unsigned char>(),
>+                                                   gfxIntSize(width, height),
>+                                                   width * 4, format);
>+                mFrontSurface = new gfxImageSurface(mFrontBuffer.get<unsigned char>(),
>+                                                    gfxIntSize(width, height),
>+                                                    width * 4, format);
> 
>+                // If we aren't using Xshm, we don't need a separate surface.
>+                surface = mFrontSurface;
>+            }
>+        }
>+#endif
>+

I think I understand the logic of this setup now, but if your comment
near m*Buffer/Surface/SharedImage doesn't cover the three
configurations selected between here, it'd be nice to document it
here.

>diff --git a/gfx/thebes/src/Makefile.in b/gfx/thebes/src/Makefile.in
>--- a/gfx/thebes/src/Makefile.in
>+++ b/gfx/thebes/src/Makefile.in
>@@ -129,18 +129,19 @@ endif
> EXTRA_DSO_LDOPTS += $(MOZ_PANGO_LIBS) $(ZLIB_LIBS) $(XLDFLAGS) $(XLIBS) $(XEXT_LIBS)
> endif
> 
> ifdef MOZ_DFB
> CPPSRCS += gfxDirectFBSurface.cpp
> endif
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>-CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp gfxSharedImageSurface.cpp
>+CPPSRCS += gfxQtPlatform.cpp gfxQPainterSurface.cpp
> CPPSRCS += gfxXlibSurface.cpp gfxQtNativeRenderer.cpp
>+CPPSRCS += gfxSharedImageSurface.cpp

I don't understand this change.

Comment 64

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

Comment on attachment 441699 [details] [diff] [review]
 support 16bpp in canvas XShm support, v4   

>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
> #ifdef MOZ_IPC
> bool
> nsCanvasRenderingContext2D::CreateShmemSegments(PRInt32 width, PRInt32 height,
>                                                 gfxASurface::gfxImageFormat format)
> {
>+    int bytes = gfxASurface::BytePerPixelFromFormat(format);
>+    SharedMemory::SharedMemoryType shmtype = SharedMemory::TYPE_BASIC;
>+
>+#ifdef MOZ_HAVE_SHAREDMEMORYSYSV
>+    shmtype = SharedMemory::TYPE_SYSV;
>+#endif
>+
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-        AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>-                   &mFrontBuffer))
>+        AllocShmem(width * height * bytes, shmtype, &mFrontBuffer))
>         return false;
>     if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-                AllocShmem(width * height * 4, SharedMemory::TYPE_SYSV,
>-                           &mBackBuffer))
>+        AllocShmem(width * height * bytes, shmtype, &mBackBuffer))
>         return false;
> 
>     return true;
> }
> #endif
> 

This should be in the other patch.

>diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h
>--- a/ipc/glue/IPCMessageUtils.h
>+++ b/ipc/glue/IPCMessageUtils.h
>+template<>
>+struct ParamTraits<gfxASurface::gfxImageFormat>
>+{
>+  typedef gfxASurface::gfxImageFormat paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    int32_t val = static_cast<int32_t>(aParam);
>+    WriteParam(aMsg, val);
>+  }
>+
>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    int32_t val = -1;
>+    if (ReadParam(aMsg, aIter, &val)) {
>+      if (val < 0 || val > static_cast<int32_t>(gfxASurface::ImageFormatUnknown))

Should be
  |val >= static_cast<int32_t>(gfxASurface::ImageFormatUnknown)|.
Also you don't need to static cast here, you can use |int32_t(blah)|.

Comment 65

[Oleg Romashin (:romaxa)]

Attachment: More simple implementation using updated gfxSharedImageSurface class

Comment 66

[Oleg Romashin (:romaxa)]

Comment on attachment 443048 [details] [diff] [review]
More simple implementation using updated gfxSharedImageSurface class

bad patch

Comment 67

[Oleg Romashin (:romaxa)]

Attachment: Canvas SHM rendering with Copy

This patch looks more nice from my point of view.
It depends from https://bugzilla.mozilla.org/attachment.cgi?id=443098&action=edit
new Shmem based gfxSharedImageSurface implementation.

This version we are creating Front surface and shared back surface. and instead of surfaces swapping we are copying data from back surface to front (in X case stuff optimized with XShmPutImage)

Comment 68

[Oleg Romashin (:romaxa)]

Also fro Qt port, we should add this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=443073&action=edit - make Qt port independent from old sharedImageSurface, and more compatible with new gfxSharedImageSurface implementation

Comment 69

[Oleg Romashin (:romaxa)]

Attachment: Right implementation without XShmPutImage and with proper surface swap

Comment 70

[Oleg Romashin (:romaxa)]

Comment on attachment 443115 [details] [diff] [review]
Right implementation without XShmPutImage and with proper surface swap

To get this patches working we should apply these two first:
https://bugzilla.mozilla.org/attachment.cgi?id=443073&action=edit
and 
https://bugzilla.mozilla.org/attachment.cgi?id=443098&action=edit

Comment 71

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 441206 [details] [diff] [review]
make async draw work with Qt's shared image drawing

This looks fine, though I'm more concerned about the earlier parts that went in to canvas -- I hear that romaxa is working on getting a gfxSharedImageSurface/ gfxSharedSurface going, which would be much better than putting a lot of this goop inside canvas.

Comment 72

[Joe Drew (not getting mail)]

Oleg, can you create a new version of your patch that's just a delta on top of attachment 441206 [details] [diff] [review] (I think that's what you're basing it on)? Right now it's smashed together, and hard for me to piece out what you're changing and what you're not.

Comment 73

[Oleg Romashin (:romaxa)]

Attachment: Diff between joe version.

Comment 74

[Joe Drew (not getting mail)]

Comment on attachment 443244 [details] [diff] [review]
Diff between joe version.

This looks more or less OK, but it's not clear to me that we're going to correctly use XShm - probably because I don't have all of the required patches right here.

Comment 75

[Oleg Romashin (:romaxa)]

Attachment: Full patch against latest e10s

Ok, here is the full patch which is covering all rendering on X11 environment cases (XSurface, ImageSurface...).

This patch is not dealing with XShm stuff at all.. because we don't need that.
Idea is to have Shmem based renderer which is supposed to work on all platforms... and NativeID renderer for Client-Server rendering platforms...

When we are rendering GTK(X) or QT(-graphicssystem native), Cairo-Xlib surface used for rendering content and Async canvas manipulation with Native X-Surface ID's across the processes.

When we are rendering GTK(offscreen image) or QT(-graphicssystem raster), Cairo-Image surface used for rendering content and Async canvas manipulation with Shared memory buffers across the processes.

Comment 76

[Oleg Romashin (:romaxa)]

Comment on attachment 445676 [details] [diff] [review]
Full patch against latest e10s

Ok, it is a bit too early for review... I need to fix some nits, and make sure that it is working finie on GTK/QT... and have proper ifdefs for compilation on other platforms

Comment 77

[Oleg Romashin (:romaxa)]

Attachment: Test on GTK and QT(X11/Raster) port

Comment 78

[Joe Drew (not getting mail)]

Comment on attachment 445699 [details] [diff] [review]
Test on GTK and QT(X11/Raster) port

Man, my choice of making multiple DocumentRenderer<Whatever>.ipdl to get around the the inability to have multiple destructors is sure biting us in the ass. Sometime after this lands we should figure out some way of combining all of them.

A general comment: Make sure copyright years are 2010. And since you wrote this, I think it's fine if the Original Developer is Nokia.

Also, I think that the changes to DocumentRendererShmem should be part of another patch, possibly the gfxSharedImageSurface patch.

>diff -r cc2646c6f1dd content/canvas/public/nsICanvasRenderingContextInternal.h

>+  // 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(PRUint32 nativeID,
>+                  PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h) = 0;

This comment needs updating - there is no |back| here.

>diff -r cc2646c6f1dd content/canvas/src/DocumentRendererNativeIDChild.cpp

>+DocumentRendererNativeIDChild::RenderDocument(nsIDOMWindow *window, const PRInt32& x,

>+    // Draw directly into the output array.
>+    nsRefPtr<gfxASurface> surf;
>+#ifdef MOZ_X11
>+    // Initialize gfxXlibSurface from native XID by using toolkit functionality
>+    // Would be nice to have gfxXlibSurface(nativeID) implementation
>+#if defined(MOZ_WIDGET_GTK2)
>+    GdkDrawable *drawable = (GdkDrawable*)gdk_pixmap_foreign_new(nativeID);
>+    GdkScreen *gscreen = gdk_drawable_get_screen(drawable);
>+    Screen * screen = GDK_SCREEN_XSCREEN(gscreen);
>+    Display *dpy = DisplayOfScreen(screen);
>+    XVisualInfo vinfo;
>+    int foundVisual = XMatchVisualInfo(dpy,
>+                                       gdk_x11_screen_get_screen_number(gscreen),
>+                                       gdk_drawable_get_depth(drawable),
>+                                       TrueColor,
>+                                       &vinfo);
>+    if (!foundVisual)
>+        return false;
>+    Visual * visual = vinfo.visual;
>+#elif defined(MOZ_WIDGET_QT)
>+    QPixmap pixmap = QPixmap::fromX11Pixmap(nativeID);
>+    Visual *visual = static_cast<Visual*>(pixmap.x11Info().visual());
>+    Display *dpy = pixmap.x11Info().display();
>+#endif

These pointer declaration styles are all over the place - I think the prevailing style in new IPC code is 

Type* foo;

Can you change all your pointer stuff to that?

>+    surf = new gfxXlibSurface(dpy, nativeID, visual);
>+#endif

The code after this will fail if MOZ_X11 isn't set, so you should do #else #error not ported to non-X11 #endif.

>diff -r cc2646c6f1dd content/canvas/src/nsCanvasRenderingContext2D.cpp

> nsCanvasRenderingContext2D::~nsCanvasRenderingContext2D()
> {
>     Destroy();
> 
>+    mozilla::dom::ContentProcessParent *allocator = mozilla::dom::ContentProcessParent::GetSingleton(PR_FALSE);
>+    if (allocator && mBackSurface
>+        && mBackSurface->GetType() == gfxASurface::SurfaceTypeImage
>+        && mBackSurface->GetData(&gfxSharedImageSurface::SHM_KEY)) {

Why don't you define SurfaceTypeSharedImage?

>+        mozilla::ipc::Shmem mem = static_cast<gfxSharedImageSurface*>(mBackSurface.get())->GetShmem();
>+        allocator->DeallocShmem(mem);
>+    } else
>+    if (mBackSurface->GetType() == gfxASurface::SurfaceTypeXlib
>+        && !mIsBackSurfaceReadable)
>+        NS_WARNING("We must own back surface on destroy,");
>+
>+    mIsBackSurfaceReadable = PR_FALSE;

This is unnecessary in the destructor.

> void
> nsCanvasRenderingContext2D::Destroy()
> {
>+    mozilla::dom::ContentProcessParent *allocator = mozilla::dom::ContentProcessParent::GetSingleton(PR_FALSE);
>+    if (allocator && mSurface &&
>+        mSurface->GetType() == gfxASurface::SurfaceTypeImage
>+        && mSurface->GetData(&gfxSharedImageSurface::SHM_KEY)) {

Same comments as above.

> nsresult
> nsCanvasRenderingContext2D::SetStyleFromStringOrInterface(const nsAString& aStr,
>@@ -969,64 +1012,58 @@ nsCanvasRenderingContext2D::Redraw(const
>         return NS_OK;
> 
>     if (++mInvalidateCount > kCanvasMaxInvalidateCount)
>         return Redraw();
> 
>     return mCanvasElement->InvalidateFrameSubrect(r);
> }
> 
>-#ifdef MOZ_IPC
>-bool
>-nsCanvasRenderingContext2D::CreateShmemSegments(PRInt32 width, PRInt32 height,
>-                                                gfxASurface::gfxImageFormat format)
>-{
>-    if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-        AllocShmem(width * height * 4, SharedMemory::TYPE_BASIC,
>-                   &mFrontBuffer))
>-        return false;
>-    if (!mozilla::dom::ContentProcessParent::GetSingleton()->
>-                AllocShmem(width * height * 4, SharedMemory::TYPE_BASIC,
>-                           &mBackBuffer))
>-        return false;
>-
>-    mBackSurface = new gfxImageSurface(mBackBuffer.get<unsigned char>(),
>-                                       gfxIntSize(width, height),
>-                                       width * 4, format);
>-
>-    return true;
>-}
>-#endif
>-
> NS_IMETHODIMP
> nsCanvasRenderingContext2D::SetDimensions(PRInt32 width, PRInt32 height)
> {
>     Destroy();
> 
>     nsRefPtr<gfxASurface> surface;
> 
>     // Check that the dimensions are sane
>-    if (gfxASurface::CheckSurfaceSize(gfxIntSize(width, height), 0xffff)) {
>-        gfxASurface::gfxImageFormat format = gfxASurface::ImageFormatARGB32;
>-        if (mOpaque)
>-            format = gfxASurface::ImageFormatRGB24;
>+    gfxIntSize size(width, height);
>+    if (gfxASurface::CheckSurfaceSize(size, 0xffff)) {
>+
>+        gfxASurface::gfxImageFormat format = GetImageFormat();
>+
>+        surface = gfxPlatform::GetPlatform()->CreateOffscreenSurface
>+            (size, format);
>+
>+        if (surface && surface->CairoStatus() != 0)
>+            surface = nsnull;
> 
> #ifdef MOZ_IPC
>-        if (mShmem && CreateShmemSegments(width, height, format)) {
>-            NS_ABORT_IF_FALSE(mFrontBuffer.get<unsigned char>(), "No front buffer!");
>-            surface = new gfxImageSurface(mFrontBuffer.get<unsigned char>(),
>-                                          gfxIntSize(width, height),
>-                                          width * 4, format);
>-        } else
>+        if (mAsync && surface) {
>+#ifdef MOZ_X11
>+            if (surface->GetType() == gfxASurface::SurfaceTypeXlib) {
>+                mBackSurface =
>+                    gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format);

Here add NS_ABORT_IF_FALSE(mBackSurface->GetType() == gfxASurface::SurfaceTypeXlib, "need xlib surface");

>-NS_IMETHODIMP
>-nsCanvasRenderingContext2D::Swap(mozilla::ipc::Shmem& aBack, 
>-                                 PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h)
>+nsresult
>+nsCanvasRenderingContext2D::Swap(const gfxRect &aRect)
> {
>-#ifdef MOZ_IPC
>     // 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;

This comment is still meaningful (we can be resized when an asynchronous render call is being serviced), but you've removed the code checking that condition.. You should add another way of checking.

>     nsRefPtr<gfxContext> ctx = new gfxContext(mSurface);

This is equivalent to mThebes now, isn't it? (Since we're not swapping contexts anymore, just drawing to our current context.)

>+    nsRefPtr<gfxPattern> pat = ctx->GetPattern();

= getter_AddRefs(ctx->GetPattern());

>+NS_IMETHODIMP
>+nsCanvasRenderingContext2D::Swap(mozilla::ipc::Shmem& aBack,
>+                                 PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h)
>+{
>+#ifdef MOZ_IPC
>+    if (!mBackSurface)
>+        return NS_ERROR_FAILURE;
>+
>+    if (mBackSurface->GetType() == gfxASurface::SurfaceTypeImage
>+        && mBackSurface->GetData(&gfxSharedImageSurface::SHM_KEY)) {
>+        mozilla::ipc::Shmem& mem = static_cast<gfxSharedImageSurface*>(mBackSurface.get())->GetShmem();
>+        if (mem.IsReadable())
>+            NS_WARNING("Back surface readable before swap, this must not happen");

NS_ERROR

>+        mem = aBack;

What gets done with this?

>+    }
>+    return Swap(gfxRect(x, y, w, h));
> #endif
> }
> 
> NS_IMETHODIMP
>+nsCanvasRenderingContext2D::Swap(PRUint32 nativeID,
>+                                 PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h)
>+{
>+    if (mIsBackSurfaceReadable)
>+        NS_WARNING("Back surface readable before swap, this must not happen");

NS_ERROR

I want to take another look at this (hence the r-), but I'm pretty happy with where it's going.

Comment 79

[Oleg Romashin (:romaxa)]

Attachment: Fixed most of comments, gfxSharedImageSurface part

Comment 80

[Oleg Romashin (:romaxa)]

Attachment: nativeID Renderer part.

Comment 81

[Joe Drew (not getting mail)]

Comment on attachment 447068 [details] [diff] [review]
Fixed most of comments, gfxSharedImageSurface part


>diff -r 5c29542bba4e content/canvas/src/nsCanvasRenderingContext2D.cpp

>+    /**
>+     * Sync data from backSurface to frontSurface
>+     */
>+    nsresult Swap(const gfxRect& aRect);

There doesn't seem to be a frontSurface, though? Maybe say "from mBackSurface to mSurface."

>     // We always have a front buffer. We hand the back buffer to the other
>     // process to render to, and then swap our two buffers when it finishes.
>-    mozilla::ipc::Shmem mFrontBuffer;
>-    mozilla::ipc::Shmem mBackBuffer;
>     nsRefPtr<gfxASurface> mBackSurface;

we hand the back _surface_ to the other process.

>+nsresult
>+nsCanvasRenderingContext2D::Swap(const gfxRect& aRect)
> {
>-#ifdef MOZ_IPC
>     // 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.

You still need to implement this check.

Looks good. Let's go to vlad for sr of the API changes.

Comment 82

[Joe Drew (not getting mail)]

Comment on attachment 447069 [details] [diff] [review]
nativeID Renderer part.

>diff -r d3c7c125fd66 content/canvas/src/DocumentRendererNativeIDChild.cpp

>+bool
>+DocumentRendererNativeIDChild::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,
>+                                      const PRInt32& nativeID)

>+#else
>+    NS_ERROR("NativeID handler not implemented for your platform");

Would you change this just to #error, so we don't even compile?

Comment 83

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

>diff -r 5c29542bba4e content/canvas/src/nsCanvasRenderingContext2D.cpp
>--- a/content/canvas/src/nsCanvasRenderingContext2D.cpp	Fri May 21 16:18:19 2010 -0700
>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp	Mon May 24 13:14:06 2010 +0300

> using mozilla::ipc::SharedMemory;
> #endif
> 
> using namespace mozilla;

We already have mozilla and mozilla::ipc::SharedMemory being used here; places below shouldn't be fully qualified.


>+            mozilla::dom::ContentProcessParent* allocator = mozilla::dom::ContentProcessParent::GetSingleton();
>+            mBackSurface = new gfxSharedImageSurface();
>+            static_cast<gfxSharedImageSurface*>(mBackSurface.get())->Init(allocator, size, format, shmtype);
>+        }

Add a 'typedef mozilla::dom::ContentProcessParent ContentProcessParent;' somewhere, or just use the mozilla::dom namespace.

>+nsresult
>+nsCanvasRenderingContext2D::Swap(const gfxRect& aRect)
> {
>-#ifdef MOZ_IPC
>     // 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.

So, I hate to be nitpicky, but tradtionally, one draws to the back buffer and swaps to the front buffer for presentation.  It seems like that semantic is different here -- that we draw to the front buffer and then swap to the back buffer for transmission to the process that's going to display it.  Is that correct?  If so, I'd really prefer that the meanings/etc. be changed here.  If not, then I'm not understanding something correctly -- is the back buffer coming in from the remote process and it's being displayed to the front buffer?

>+    mThebes->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    mThebes->SetSource(mBackSurface);
>+    mThebes->Rectangle(aRect, PR_TRUE);
>+    mThebes->Clip();
>+    mThebes->Paint();

This needs a Save()/Restore() as well as a path save/path restore here -- unless the intent is that this "canvas" is just displaying the results of a remote canvas?  If so, I don't understand why the display has to be a <canvas> at all -- we'll never draw on it, no?

>+            mozilla::ipc::Shmem& backmem = static_cast<gfxSharedImageSurface*>(mBackSurface.get())->GetShmem();
>+            if (!backmem.IsWritable())
>+                return NS_ERROR_FAILURE;
>+            mozilla::ipc::PDocumentRendererShmemParent* pdocrender 

We already have a 'using mozilla::ipc::SharedMemory' -- let's just do a using mozilla::ipc, and use the bare names here to make this cleaner.  Or use ipc::Shmem.

Comment 84

[Oleg Romashin (:romaxa)]

Attachment: Fix vlad comments

Comment 85

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> This needs a Save()/Restore() as well as a path save/path restore here --
> unless the intent is that this "canvas" is just displaying the results of a
> remote canvas?  If so, I don't understand why the display has to be a <canvas>
> at all -- we'll never draw on it, no?

While a Save/Restore was added, a path save/restore was not.  Look at gfxContextPathAutoSaveRestore and gfxContextAutoSaveRestore -- ClearRect() does the right thing there.

I'd still like an answer to the second part of that question -- is this canvas ever actually rendered to?  Or do we just hand a surface off to another process and then composite it in when it's done?  If so, the 2d canvas isn't really the right place for this to happen, though that bad design ship may have already sailed -- that really should have been a new element (or, perhaps, a new canvas rendering context if we wanted to keep using the same frame code).  Given that I think much of this code will become obsolete once we have layers, I would strongly prefer to not have this intertwined with the 2D canvas code.

Comment 86

[Oleg Romashin (:romaxa)]

Attachment: Added gfxContextPathAutoSaveRestore, gfxContextAutoSaveRestore

I did not get second question... are you talking about general design of Shmem/Async canvas? or about design in my patch?

Current Shmem canvas code is using swap approach, and with my patch it is doing Flush/Composite backSurface to frontSurface.

Also here is the another bug about renaming canvas context name from shmem ot IPC, see bug 568632

Comment 87

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm asking about the design, since this is the first time that I've seen it -- it seems like the front end canvas is just a shell that's intended to draw a surface provided by the remote child -- is that correct?  If so, at worst it should be its own context, and at best should be its own frame type.

Comment 88

[Oleg Romashin (:romaxa)]

(In reply to comment #87)
> I'm asking about the design, since this is the first time that I've seen it --

seen this implementation? or remote shmem canvas implementation in general?

> it seems like the front end canvas is just a shell that's intended to draw a
> surface provided by the remote child -- is that correct?

yes., that is how it was before, with all async/shmem canvas implementations.

>  If so, at worst it
> should be its own context,

We are using own context for it already... canvas.MozGetShmemContext("2d");

> and at best should be its own frame type.
What do you mean here? new HTML element?

Comment 89

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 449190 [details] [diff] [review]
Added gfxContextPathAutoSaveRestore, gfxContextAutoSaveRestore

(In reply to comment #88)
> (In reply to comment #87)
> > I'm asking about the design, since this is the first time that I've seen it --
> 
> seen this implementation? or remote shmem canvas implementation in general?

the 2D canvas with shmem.

> > it seems like the front end canvas is just a shell that's intended to draw a
> > surface provided by the remote child -- is that correct?
> 
> yes., that is how it was before, with all async/shmem canvas implementations.

Yeah, ok.

> 
> >  If so, at worst it
> > should be its own context,
> 
> We are using own context for it already... canvas.MozGetShmemContext("2d");

You're using a different name (or, rather, a different function -- that doesn't make any sense to me either), but you're using the 2d canvas code for no reason.  I can see that on the -child- end (maybe), but it doesn't make sense to me on the front end.

> > and at best should be its own frame type.
> What do you mean here? new HTML element?

Yes, or new XUL element.

Regardless, that's not for this patch specifically, and I don't really want to block this patch for design decisions that were made before it -- but it's a discussion that I would like to have if we're going with this architecture going forward.  If we're going to move to IPC layers, and this code becomes obsolete, then we should make sure that we delete all this shmem stuff from the 2d canvas so that we don't have to maintain it in the future.

Comment 90

[Oleg Romashin (:romaxa)]

> going forward.  If we're going to move to IPC layers, and this code becomes
> obsolete, then we should make sure that we delete all this shmem stuff from the
> 2d canvas so that we don't have to maintain it in the future.

yep, with IPC layers this code must die, because nobody will use it.

Comment 91

[Oleg Romashin (:romaxa)]

Ok, hope it will not break anything.
http://hg.mozilla.org/projects/electrolysis/rev/ae03b58ae488