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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: joe, Assigned: joe)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 18 obsolete attachments)

4.83 KB, patch
Details | Diff | Splinter Review
51.01 KB, patch
dougt
: review+
Details | Diff | Splinter Review
11.46 KB, patch
cjones
: review+
Details | Diff | Splinter Review
17.64 KB, patch
cjones
: review+
Details | Diff | Splinter Review
38.83 KB, patch
joe
: review+
Details | Diff | Splinter Review
28.36 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 524180
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.
Summary: use Xshm and MIT shared memory for Shmem → Implement a SysV shared memory backend for Shmem and allow them to be used with Xshm
(Assignee)

Comment 2

9 years ago
Chris, is your plan to implement this soon? I may look into it otherwise.
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

9 years ago
Created attachment 429767 [details] [diff] [review]
hack joe was talking about - should be backed out when we land a real fix

Updated

9 years ago
Attachment #429767 - Flags: review?(jmuizelaar)
Who reviewed the original SHM/IPC stuff? It seems like they would be a better reviewer for this.
(Assignee)

Comment 6

9 years ago
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.
Attachment #429767 - Flags: review?(jmuizelaar) → review?(benjamin)
(Assignee)

Updated

9 years ago
Attachment #429767 - Flags: review?(benjamin) → review?(jmuizelaar)

Comment 7

9 years ago
In that case, can we land this and have it reviewed at the same time when the original SHM/IPC code is
(Assignee)

Comment 8

9 years ago
This can't land without bug 524180, so no, it can't land yet :)
(Assignee)

Comment 9

9 years ago
Created attachment 434663 [details] [diff] [review]
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.
Attachment #434663 - Flags: review?(jones.chris.g)
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.)
Hmm, this patch also has the downside of not compiling on e10s latest.
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.
Attachment #434663 - Flags: review?(jones.chris.g) → review-
Created attachment 435114 [details] [diff] [review]
Add a SysV Shmem backend and use that for <canvas>
Attachment #434663 - Attachment is obsolete: true
Attachment #435114 - Flags: review?(joe)
FWIW this code wfm in (modified) ipc/ipdl/test/cxx/TestShmem and fennec browser.
(Assignee)

Comment 15

9 years ago
Created attachment 436590 [details] [diff] [review]
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.
Assignee: nobody → joe
Attachment #435114 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #436590 - Flags: review?(jones.chris.g)
Attachment #435114 - Flags: review?(joe)
(Assignee)

Comment 16

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

Comment 18

9 years ago
Once you shmat() to the segment, you can safely mark it as removed. Map() calls shmat() then shmctl(IPC_RMID).
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
(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?
(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.
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.
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 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.
Attachment #436590 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 25

9 years ago
Definitely move that hunk.
(Assignee)

Comment 26

9 years ago
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.
Created attachment 436763 [details] [diff] [review]
Updated version of joe's patch
Attachment #436763 - Flags: review?(dougt)
(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().
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.
(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.
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.
(Assignee)

Comment 33

9 years ago
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.
(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.)
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.
> > -  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
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
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.
(Assignee)

Comment 39

9 years ago
Created attachment 438270 [details] [diff] [review]
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.
Attachment #436590 - Attachment is obsolete: true
Attachment #436763 - Attachment is obsolete: true
Attachment #438270 - Flags: review?(dougt)
Attachment #436763 - Flags: review?(dougt)
(Assignee)

Comment 40

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

Updated

9 years ago
Attachment #438271 - Flags: review?(jones.chris.g)
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.
(Assignee)

Comment 42

9 years ago
(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. :)
> 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 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 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 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.
Attachment #438271 - Flags: review?(jones.chris.g) → review+
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.
Attachment #438270 - Flags: review?(dougt) → review+
(Assignee)

Comment 48

9 years ago
(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?
(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 ;).
(Assignee)

Comment 50

9 years ago
(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);
}
Then you'd also need a templated virtual IsHandleValidImpl() declaration, and we wouldn't catch type mismatches until link time (with undefined symbols).
(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.
(Assignee)

Updated

9 years ago
Attachment #436765 - Attachment description: Test → Test for SysV shmem [Checkin: Comment 53]
(Assignee)

Updated

9 years ago
Attachment #438270 - Attachment description: fix non-debug build → fix non-debug build [Checkin: Comment 53]
(Assignee)

Comment 54

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

Comment 55

9 years ago
Created attachment 441208 [details] [diff] [review]
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).
Attachment #441208 - Flags: review?(jones.chris.g)
none of the IPC stuff is in canvas on the trunk; what bug is that patch in?
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.
Attachment #441208 - Flags: review-
(Assignee)

Comment 58

9 years ago
This is all in the electrolysis branch. Look in dependent bug 524180 and its dependent bug 505847 for more of its lineage.
(Assignee)

Comment 59

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

Comment 61

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

Comment 62

9 years ago
Created attachment 441699 [details] [diff] [review]
 support 16bpp in canvas XShm support, v4   

Previous version was the same as V2 because I had another patch applied when I qrefreshed.
Attachment #441641 - Attachment is obsolete: true
Attachment #441699 - Flags: superreview?(vladimir)
Attachment #441699 - Flags: review?(jones.chris.g)
Attachment #441641 - Flags: superreview?(vladimir)
Attachment #441641 - Flags: review?(jones.chris.g)
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.
Attachment #441206 - Flags: review?(jones.chris.g) → review+
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)|.
Attachment #441699 - Flags: review?(jones.chris.g) → review+
Created attachment 443048 [details] [diff] [review]
More simple implementation using updated gfxSharedImageSurface class
Attachment #443048 - Flags: review?(joe)
Comment on attachment 443048 [details] [diff] [review]
More simple implementation using updated gfxSharedImageSurface class

bad patch
Attachment #443048 - Flags: review?(joe)
Created attachment 443100 [details] [diff] [review]
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)
Attachment #443048 - Attachment is obsolete: true
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
Created attachment 443115 [details] [diff] [review]
Right implementation without XShmPutImage and with proper surface swap
Attachment #443115 - Flags: feedback?(joe)
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 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.
Attachment #441206 - Flags: superreview?(vladimir) → superreview+
Attachment #441699 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 72

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #443115 - Flags: feedback?(joe) → feedback-
Created attachment 443244 [details] [diff] [review]
Diff between joe version.
Attachment #443244 - Flags: feedback?(joe)
(Assignee)

Comment 74

9 years ago
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.
Attachment #443244 - Flags: feedback?(joe) → feedback+
Created attachment 445676 [details] [diff] [review]
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.
Attachment #443115 - Attachment is obsolete: true
Attachment #443244 - Attachment is obsolete: true
Attachment #445676 - Flags: review?(joe)
Attachment #445676 - Flags: review?(joe)
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
Created attachment 445699 [details] [diff] [review]
Test on GTK and QT(X11/Raster) port
Attachment #445676 - Attachment is obsolete: true
Attachment #445699 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #445699 - Flags: review?(joe) → review-
(Assignee)

Comment 78

9 years ago
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.
Created attachment 447068 [details] [diff] [review]
Fixed most of comments, gfxSharedImageSurface part
Attachment #445699 - Attachment is obsolete: true
Created attachment 447069 [details] [diff] [review]
nativeID Renderer part.
Attachment #443100 - Attachment is obsolete: true
Attachment #447068 - Flags: review?(joe)
Attachment #447069 - Flags: review?(joe)
(Assignee)

Comment 81

8 years ago
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.
Attachment #447068 - Flags: superreview?(vladimir)
Attachment #447068 - Flags: review?(joe)
Attachment #447068 - Flags: review+
(Assignee)

Comment 82

8 years ago
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?
Attachment #447069 - Flags: review?(joe) → review+
>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.
Created attachment 448426 [details] [diff] [review]
Fix vlad comments
Attachment #447068 - Attachment is obsolete: true
Attachment #448426 - Flags: review?(vladimir)
Attachment #447068 - Flags: superreview?(vladimir)
> 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.
Created attachment 449190 [details] [diff] [review]
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
Attachment #448426 - Attachment is obsolete: true
Attachment #448426 - Flags: review?(vladimir)
Attachment #449190 - Flags: superreview?(vladimir)
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.
(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 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.
Attachment #449190 - Flags: superreview?(vladimir) → superreview+
> 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.