Open Bug 483409 Opened 11 years ago Updated 7 months ago

Optionally use SHM for rendering on X11

Categories

(Core :: Widget: Gtk, defect, P5)

All
Linux
defect

Tracking

()

Tracking Status
fennec 1.0- ---

People

(Reporter: vlad, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch use shm for gtk rendering (obsolete) — Splinter Review
We often can't trust the performance/stability of X for what we want to render, and it's often faster for us to do all our rendering client-side.  This is especially true of mobile devices, which is where this patch is targetted.

The patch creates a screen-sized shared memory XImage at the native depth.  If that depth isn't 24, it also creates a local chunk of memory for the 24bpp equivalent of that.  On draw time, before calling XShmPutImage, the code uses pixman to convert from 24 to 16 if necessary.

A screen-sized SHM segment is used because for the target users of this, our app is generally going to be fullscreen anyway, and the screen is quite small; locking down one chunk of SHM is better than trying to manage a separate SHM chunk per toplevel window.  For a 800*480 16-bit display, we end up with 800*480*2 + 800*480*4 = ~2.2MB (and then never allocate temporary render targets except when we need to PushGroup for opacity handling).

This patch mostly works as is.  There are some issues that I think are mostly our/gtk's fault, and not synchronization problems or something with XShm itself.  I have a patch that actually watches for XShmCompletion events, and we very very rarely do another XShmPutImage before the current one is finished.  As long as we don't draw directly into the shm buffer, we should be fine.

Also, nsWindow::OnExposeEvent is a disaster, and this patch doesn't help.  This won't get checked in without doing some cleanup, but I'll do the cleanup patch as one on top of this one (will attach to this same bug).

With this patch, when firing up a rendering-intensive dhtml app, we sit at 90% inside firefox and less than 10% inside X, so we can easily continue to optimize our internal paths.
Flags: wanted-fennec1.0?
Comment on attachment 367390 [details] [diff] [review]
use shm for gtk rendering

Jeff, can you take a look at this and see if you see anything dumb?  It's somewhat similar to your older XSHM patch in bug 450400, though it doesn't try to keep a shm image around per widget.

Also, looking at this again, we should be able to use gdk to create GdkImages which would be a little more portable for something like Gdk-DFB, but it's probably not worth doing that right away (especially since then we'd need a GdkGC, and I don't know how expensive creating one of those is compared to an X GC).
Attachment #367390 - Flags: review?(jmuizelaar)
tracking-fennec: --- → ?
Flags: wanted-fennec1.0? → wanted-fennec1.0+
So I think the problem that's left is that I made the assumption that widgets would always be painted/exposed in back-to-front order.  I don't think this is the case, and the problem occurs because we receive an expose event for a widget behind another one... X takes care of the clipping, but we'll have to do it manually here.
Attached patch updated (obsolete) — Splinter Review
Updated patch.  The widget overlap issue wasn't the problem, I just had a stupid Rectangle() setting a clip to the entire boundsRect instead of the specific pieces.. also the pixman path needed to do composites for each update region separately, not the whole thing (though the whole thing shouldn't hurt).
Attachment #367390 - Attachment is obsolete: true
Attachment #367790 - Flags: review?(jmuizelaar)
Attachment #367390 - Flags: review?(jmuizelaar)
Updated again; also did cleanup of nsWindow::OnExposeEvent to remove a bunch of code duplication and meandering code paths there.
Attachment #367790 - Attachment is obsolete: true
Attachment #367862 - Flags: review?(roc)
Attachment #367790 - Flags: review?(jmuizelaar)
+
+        if (gXImage) {
+            gShmSurfaceData = nsnull;
+            XShmDetach(gdk_x11_get_default_xdisplay(), &gShmInfo);
+            XDestroyImage(gXImage);
+            shmdt(gShmInfo.shmaddr);
+            shmctl(gShmInfo.shmid, IPC_RMID, 0);
+            gXImage = nsnull;
+
+            if (gSrcPixmanImage) {
+                pixman_image_unref(gSrcPixmanImage);
+                gSrcPixmanImage = nsnull;
+            }
+
+            if (gDstPixmanImage) {
+                pixman_image_unref(gDstPixmanImage);
+                gDstPixmanImage = nsnull;
+            }
+        }

Should be #ifdef MOZ_X11

+        targetSurface->SetDeviceOffset(gfxPoint(screenPos.x, screenPos.y));

What is the point of doing this? Couldn't we just always put the data at (0,0), and remove the use of screenPos to compute rsx/rsy?

+#ifdef MOZ_X11
+    if (doingShm) {
+        // If we were doing Shm, draw it into place

Can we break up this function into some helpers? It's way too long. I guess you offered to do that as a followup patch?

There is a potential synchronization risk here, right? The server could read the buffer while we're copying into it from our buffer, and render part or all of the wrong window. Right?
(In reply to comment #5)
> [...]
> Should be #ifdef MOZ_X11

Whoops, yes.

> +        targetSurface->SetDeviceOffset(gfxPoint(screenPos.x, screenPos.y));
> 
> What is the point of doing this? Couldn't we just always put the data at (0,0),
> and remove the use of screenPos to compute rsx/rsy?

Because in some cases (the 24bpp case) we draw directly to that buffer, though we double buffer when we do.  That buffer ends up representing the entire display, so we want things to be in the right spot there in cause there's an outstanding Shm read in flight (though see below).  Fennec is generally fullscreen or mostly fullscreen, so putting things into the right spot doesn't hurt anything.

> 
> +#ifdef MOZ_X11
> +    if (doingShm) {
> +        // If we were doing Shm, draw it into place
> 
> Can we break up this function into some helpers? It's way too long.
> I guess you offered to do that as a followup patch?

This cleanup was the followup ;)  We can, it would just require passing a whole bunch of data down to a one-off helper function.  Don't think it would help readability that much.

> There is a potential synchronization risk here, right? The server could read
> the buffer while we're copying into it from our buffer, and render part or all
> of the wrong window. Right?

Yup, but see my comment #0 -- in practice, we very very rarely have a PutImage that doesn't complete before we queue up another one, even on a device.  If it becomes a problem we can do the synchronization with a second surface (essentially keep buffering things into the backbuffer, until a PutImage turns it in the frontbuffer), but I don't think there's any reason to do that right now.
I'm not all that comfortable landing a patch that's known to cause rendering glitches, especially rare random ones.
Why?  The patch isn't buggy, it does what it says it does...  the potential for a race condition is there, but I have never seen a glitch actually appear, just the conditions in which one -could- appear.

They would only happen given the right combination of invalidations; basically, two invalidation rectangles that overlap in some area.. it's possible to have the invalidate of the first one show the content from the overlap from the second, but it'll be fixed when the next putimage happens, so at worst it'll look like delayed rendering.  This is the main reason why the actual screen position is used and not just the 0,0 for each update.
I'm concerned about the situation where there are two widgets visible and they overlap. Let's say widget #1 is in front of widget #2. We paint widget #1 and then widget #2. The painting for widget #2 might be written back to the shared memory segment before the X server has copied the shared memory to the screen for widget #1. So, the overlapping region would be showing widget #2 instead of widget #1.

Are you assuming that only one widget will be visible?
Yeah, that's what I was worried about in comment #2 and comment #3.. but I don't actually see that happening, even with overlapping widgets.  There's #if 0'd code in the patch that clips out child widgets should we end up needing it, and can be extended to handle any other widgets that might overlap.
It could indeed be a toplevel widget so that code would have to be extended.

Maybe you don't see it now, but I don't want to land something with a known problem that could emerge at any time. Most likely if it does emerge the person who sees it won't have a clue what's going on and won't be able to reproduce it.
The only problem I've been seeing with this patch is some painting problems when srcolling native widgets, for example in Fennec's preferences.
tracking-fennec: ? → 1.0b2+
I tried this patch against Firefox trunk and it didn't yield good results. Except when clicking on a widget, or upon the first draw, every widget is drawn black. The interesting thing is this also happens if both new prefs from the patch are switched to false.
Comment on attachment 367862 [details] [diff] [review]
updated, with nsWindow::OnExpose cleanup

>+static nsAutoPtr<PRUint8> gShmSurfaceData;

>+            gShmSurfaceData = new PRUint8[gXImage->width * gXImage->height * 4];

gShmSurfaceData should be nsAutoArrayPtr instead, right?
It would be good to get this landed if possible and start fixing the bugs in it...
It would be interesting to know how this patch is affected by compositor...
The problem described in comment #13 is caused by a lack of clipping in the final drawing pass in the non-Shm case. This means that a image source which was created with clipping is painted to the global Thebes context in a non-clipped fashion, resulting in drawing where it was not expected.
jeff: tested patch -- it is pretty busted
(In reply to comment #19)
> jeff: tested patch -- it is pretty busted

Too bad. 

I leave for vacation today, so I won't be able to have a look at this till next week. Sorry.
tracking-fennec: 1.0b2+ → 1.0+
I've taken Vlad's original patch while looking at Jeff's and rewritten much of it so that it compiles and appears to actually work in a Fennec linux desktop build.  The patch is against mozilla-central.

Would be really helpful to get a couple of eyes on this since I've currently only got a pretty limited understanding of the proper way this should be done (from what I understand, the original patch itself was also a rough demonstration in its time).

Beware that, while the patch adds code to get the pref value for mozilla.widget.use-x11-shm, it also immediately hardcodes to PR_TRUE right after for testing purposes.
tracking-fennec: 1.0+ → 1.0-
Comment on attachment 367862 [details] [diff] [review]
updated, with nsWindow::OnExpose cleanup

obsolete
Attachment #367862 - Flags: review?(roc) → review-
This is probably becoming obsolete as we move towards GL compositing for Fennec.
Bug 597336 implements this for mobile, using updated versions of the patch here.  The impl there however isn't going to work with popup widgets etc., so this bug can cover "make xshm rendering work in general".
Depends on: 597336
... if we still care when we have better GL compositing support.
Assignee: vladimir → nobody
You need to log in before you can comment on or make changes to this bug.