Last Comment Bug 738937 - Use Cairo image surfaces and XShmPutImage instead of XRender on GTK/Linux MTC
: Use Cairo image surfaces and XShmPutImage instead of XRender on GTK/Linux MTC
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 8 votes (vote)
: ---
Assigned To: Frederic Plourde (:fred23)
:
Mentors:
Depends on: gtk3
Blocks: 496204 635134 722012
  Show dependency treegraph
 
Reported: 2012-03-24 09:56 PDT by George Wright (:gw280) (:gwright)
Modified: 2014-07-03 07:01 PDT (History)
40 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use image surfaces (1.51 KB, patch)
2012-04-04 14:26 PDT, George Wright (:gw280) (:gwright)
no flags Details | Diff | Splinter Review
Remove-content-Xlib-surfaces-for-on-MTC.patch (12.10 KB, patch)
2014-04-24 12:39 PDT, Frederic Plourde (:fred23)
nical.bugzilla: feedback+
Details | Diff | Splinter Review
Remove-content-Xlib-surfaces-for-OMTC-basic.patch (14.08 KB, patch)
2014-04-29 09:36 PDT, Frederic Plourde (:fred23)
no flags Details | Diff | Splinter Review
Remove-content-Xlib-surfaces-for-on-MTC_2.patch (23.32 KB, patch)
2014-05-01 10:19 PDT, Frederic Plourde (:fred23)
no flags Details | Diff | Splinter Review
capture.ogv (207.06 KB, video/ogg)
2014-05-01 10:23 PDT, Frederic Plourde (:fred23)
no flags Details
Remove-content-Xlib-surfaces-for-on-MTC_2.patch (23.32 KB, patch)
2014-05-01 10:25 PDT, Frederic Plourde (:fred23)
no flags Details | Diff | Splinter Review
Remove-content-Xlib-surfaces-for-on-MTC_3.patch (23.59 KB, patch)
2014-05-01 13:11 PDT, Frederic Plourde (:fred23)
nical.bugzilla: feedback+
Details | Diff | Splinter Review
Remove-content-Xlib-surfaces-for-on-MTC_5.patch (20.91 KB, patch)
2014-05-13 07:53 PDT, Frederic Plourde (:fred23)
nical.bugzilla: feedback+
karlt: feedback-
Details | Diff | Splinter Review
talos_test_results_for_XshmCreatImage_Optimization.txt (3.04 KB, text/plain)
2014-05-27 13:13 PDT, Frederic Plourde (:fred23)
no flags Details
results_UseXRender.txt (1.47 KB, text/plain)
2014-05-27 13:45 PDT, Frederic Plourde (:fred23)
no flags Details
results_imageOffscreenSurfaces.txt (1.50 KB, text/plain)
2014-05-27 13:46 PDT, Frederic Plourde (:fred23)
no flags Details
Remove_Xlib_surfaces_from_CreateOffscreenSurface.patch (2.34 KB, patch)
2014-05-27 13:57 PDT, Frederic Plourde (:fred23)
nical.bugzilla: review+
karlt: feedback+
Details | Diff | Splinter Review

Description George Wright (:gw280) (:gwright) 2012-03-24 09:56:11 PDT
We should use Cairo's imagesurface backend to render to SHM surfaces then use XShmPutImage to show them. This should be (read: almost certainly is) faster than using Xrender on X11/Linux.

I've already pushed to try a patch turning this on for GTK and most reftests pass already. https://tbpl.mozilla.org/?tree=Try&rev=681a93c5b99f
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-26 19:31:07 PDT
What about GTK theme rendering? We'd basically have to give up on it for GTK2. For GTK3 we can probably hack something since it has an API to render themed content to a cairo context. Watch out when we're not using the same cairo that GTK3 is, though.
Comment 2 George Wright (:gw280) (:gwright) 2012-03-27 08:36:57 PDT
We render to an xlib surface then do a readback. Unfortunately this issue will need to be fixed as well if we switch to Skia.

Do we still need to ship GTK2-able builds for legacy reasons? Or can we switch entirely to GTK3?
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-27 15:09:43 PDT
(In reply to George Wright (:gw280) from comment #2)
> We render to an xlib surface then do a readback.

That's likely to be horrific for performance.

Unfortunately the GTK3 patches haven't even landed yet.
Comment 4 Karl Tomlinson (:karlt) 2012-03-27 15:10:05 PDT
(In reply to George Wright (:gw280) from comment #2)
> Do we still need to ship GTK2-able builds for legacy reasons? Or can we
> switch entirely to GTK3?

There are probably few systems with a functional GTK3 library at present, so we'd still need to ship GTK2 builds.  That shouldn't necessary preclude optimizing GTK3 builds.

(In reply to George Wright (:gw280) from comment #0)
> We should use Cairo's imagesurface backend to render to SHM surfaces then
> use XShmPutImage to show them. This should be (read: almost certainly is)
> faster than using Xrender on X11/Linux.

When should synchronous CPU-based rendering be faster than asynchronous GPU-based rendering?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-03-27 15:16:13 PDT
(In reply to Karl Tomlinson (:karlt) from comment #4)
> When should synchronous CPU-based rendering be faster than asynchronous
> GPU-based rendering?

XRender performance is notoriously unpredictable, and the usage of X pixmaps is the most likely candidate reason for why we have trouble enabling GL layers on Linux (we see glitches and/or slowness depending on drivers).
Comment 6 Jeff Gilbert [:jgilbert] 2012-03-27 15:36:09 PDT
(In reply to Benoit Jacob [:bjacob] from comment #5)
> (In reply to Karl Tomlinson (:karlt) from comment #4)
> > When should synchronous CPU-based rendering be faster than asynchronous
> > GPU-based rendering?
> 
> XRender performance is notoriously unpredictable, and the usage of X pixmaps
> is the most likely candidate reason for why we have trouble enabling GL
> layers on Linux (we see glitches and/or slowness depending on drivers).

Why should pixmaps be a problem when we basically don't use them with GLContext?
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-03-27 16:35:41 PDT
If we render content to pixmaps (e.g. using XRender), then compositing that with GL is 'harder' (we have to use texture_from_pixmap) than if we had rendered content to a plain image surface. By 'harder' I mean it's a new code path, it uses new code paths in drivers, all of that could explain the glitches we're seein with GL layers on Linux/x11. But that's only a theory, it's also possible that the glitches might be just our bugs.
Comment 8 Karl Tomlinson (:karlt) 2012-03-27 16:43:51 PDT
There is synchronization required between GL and the X server to ensure they order their commands correctly.  This may affect performance, and will affect correctness if we or the drivers don't get it right.

Looks like this is handled in BindTexImage and SwapBuffers.
http://hg.mozilla.org/mozilla-central/annotate/244991519f53/gfx/gl/GLContextProviderGLX.cpp#l840
http://hg.mozilla.org/mozilla-central/annotate/244991519f53/gfx/gl/GLContextProviderGLX.cpp#l348

But XShm is not going to solve anything there.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-27 16:47:04 PDT
That's why my personal opinion is that we're better off not using X at all for any drawing. Maybe I'm off-topic in this bug.
Comment 10 George Wright (:gw280) (:gwright) 2012-03-27 17:29:47 PDT
(In reply to Karl Tomlinson (:karlt) from comment #4)
> When should synchronous CPU-based rendering be faster than asynchronous
> GPU-based rendering?

My understanding is the XRender is notoriously badly implemented in pretty much all the drivers. This bug stems from the direct recommendation of one of the xorg developers who basically flat out said that xrender is a disaster and that we would likely see far better performance using Cairo's imagesurface backend with shm. We'd also get GL layers for free which would help to an extent. Obviously we'd need to benchmark and my local builds using imagesurfaces "seem" faster, but I realise that means nothing.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> (In reply to George Wright (:gw280) from comment #2)
> > We render to an xlib surface then do a readback.
> 
> That's likely to be horrific for performance.
> 
> Unfortunately the GTK3 patches haven't even landed yet.

Indeed; I'm not sure what the best solution here is. I think actually we're already, even in the XRender case, rendering to a temporary xlib surface in order to do alpha extraction. So when we copy the image data back from the temporary xlib surface using cairo it's either copying it back to our main xlib surface, or our main image surface depending on what we're using. I'm not sure what the relative cost of those two different copies are though..
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-27 17:55:56 PDT
(In reply to George Wright (:gw280) from comment #10)
> Indeed; I'm not sure what the best solution here is. I think actually we're
> already, even in the XRender case, rendering to a temporary xlib surface in
> order to do alpha extraction.

No. In many cases, hopefully most cases, we have opaque ThebesLayers backed by an X pixmap and we simply tell GTK2 to draw directly into that pixmap.

> This bug stems from the direct recommendation of one of the xorg developers who
> basically flat out said that xrender is a disaster and that we would likely see
> far better performance using Cairo's imagesurface backend with shm.

Yes. Over the years we have seen lots of performance problems induced by Xrender and drawing with X in general. I think drawing with image surfaces is the way to go, apart from the GTK2 theme issue. (There's also an issue with windowless Flash wanting to draw to an X drawable, but that's much less important.)

One option would be to simply stop doing theme rendering on GTK2. No other browser does it. If we were to go that way, we'd need some solid performance numbers to back up the decision.
Comment 12 Karl Tomlinson (:karlt) 2012-03-27 18:58:05 PDT
RENDER has in general outperformed GDI for example in my experience.

There have been instances where there have been bugs, and GPU rendering is
more difficult because, if it goes wrong, things are much slower.  We had
similar glitches with D2D also, but we put in effort to work around the
issues.

Many of the things said about RENDER might also be said about OpenGL.
That is likely even less tested than RENDER.

At least there are some things that GL can do which RENDER can't, so there is
sense in using GL, but it may still require considerable effort to make
GL work well.  The last time I upgraded my rendering stack, I was able to switch kwin from using RENDER to GL without adding additional CPU load, so there is hope.
Comment 13 George Wright (:gw280) (:gwright) 2012-03-27 19:17:21 PDT
What drivers are you using? I think the major concern is that xrender can be faster (nvidia's binary driver was cited as a candidate where xrender might be faster than images), but that that's definitely not the case for most drivers. I'm running off second hand information here, though.
Comment 14 Karl Tomlinson (:karlt) 2012-03-27 19:19:51 PDT
Having an image/XShm rendering path would makes sense for when "Use hardware acceleration when available" is disabled.  That would help compare performance in different situations.

I've been using radeon drivers.
Comment 15 Marco Castelluccio [:marco] 2012-03-28 09:03:05 PDT
Did you see: https://ickle.wordpress.com/2012/03/28/cairo-1-12-let-the-releases-roll/
Comment 16 George Wright (:gw280) (:gwright) 2012-03-28 10:21:46 PDT
(In reply to Marco Castelluccio from comment #15)
> Did you see:
> https://ickle.wordpress.com/2012/03/28/cairo-1-12-let-the-releases-roll/

Interesting read. However, it seems that image surfaces tend to do better than UXA most of the time. From my (very limited) understanding about SNA, it's got severe issues with rendering fidelity so it would seem to be a bit misleading to show that it's faster when it's not rendering well, which I believe is one of the reasons why it's not the default.
Comment 17 Marco Castelluccio [:marco] 2012-04-02 13:23:48 PDT
Other two posts (radeon and nvidia):
https://ickle.wordpress.com/2012/04/02/cairo-performance-on-radeon/
https://ickle.wordpress.com/2012/03/30/cairo-performance-on-ion/

The radeon driver is a lot slower. The NVIDIA driver performs better.
Comment 18 George Wright (:gw280) (:gwright) 2012-04-02 13:31:18 PDT
This to me suggests that we really should be using cairo image surfaces then. The problem we have is that we're treating XRENDER as a "one size fits all" scheme, but that means that some proportion of our users (and it appears this proportion is non-trivial in size) will have a terrible user experience. I'd rather get to a state where our Linux backend is "great for everyone and has predictable performance" rather than "excellent for some people, good for others, and pathologically bad for the rest".

Maybe in the future we can implement a black or white list to determine which drivers to use XRENDER on, but that seems like more work than we have time to invest in right now.
Comment 19 Karl Tomlinson (:karlt) 2012-04-02 14:56:06 PDT
The most recent firefox traces (canvas, asteroids, particles) on ickle's posts were generated with Debian's Firefox september-ish last year.
That is likely Firefox 6.  At that stage we were avoiding GPU rendering for all images on non-Intel systems by using EXTEND_NONE (bug 468496).
Comment 20 George Wright (:gw280) (:gwright) 2012-04-02 14:58:28 PDT
But we're still rasterising using Cairo's xlib surfaces, which uses XRENDER.
Comment 21 George Wright (:gw280) (:gwright) 2012-04-04 14:26:32 PDT
Created attachment 612336 [details] [diff] [review]
use image surfaces

patch for reference
Comment 22 Nicolas Silva [:nical] 2012-04-09 12:31:57 PDT
Attachment 612336 [details] [diff] results in a completely black window on linux when layers.acceleration.force-enabled is set to true.

pixmaps are still used in GLContextProviderGLX.cpp.
If you comment out this line where mHasTextureFromPixmap is set to true http://dxr.lanedo.com/mozilla-central/gfx/gl/GLContextProviderGLX.cpp#l254
and have GLXLibrary::SupportsTextureFromPixmap always return false
http://dxr.lanedo.com/mozilla-central/gfx/gl/GLContextProviderGLX.cpp#l275
it seems to work again.

If we want to remove completely the use of pixmaps there's quite a few lines that can be removed from this file.
Comment 23 Frederic Plourde (:fred23) 2014-04-08 07:23:27 PDT
I'm currently working on a Xlib-surface-removal patch for GTK/Linux, using XShmPutImage and will update here as soon as I have something decent.
Comment 24 Frederic Plourde (:fred23) 2014-04-15 12:22:43 PDT
I have come up with a decent patch for this problem. For practical reasons, all tracking will happen in Bug 496204 from now on, since this is more about "stopping the use of cairo xlib surfaces" and this will have impacts on on-MTC and OMTC paths.
Comment 25 Frederic Plourde (:fred23) 2014-04-24 12:39:34 PDT
Created attachment 8412069 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC.patch

Here's the updated patch for on-MTC content Xlib surface removal.

It's building ok, and working ok too (apart from some XShmPutImage rect placement and a segfault when exiting FF) and basically what it does is to use nsShmImages (mShmBufferImage and mShmBufferImageOnWhite) **instead** of the usual DrawTargets (mDTBuffer and mDTBufferOnWhite) to paint the layer buffer.

This patch is less invasive and way more self-contained than the previous one, and does not touch to DrawTarget classes at all (since discussions with Bas and al. made it clear DrawTargets had nothing to do with XShm). Also, notice that since drawing target (aTarget) is the widget pixmap itself, and since we're using XShmPutImage to upload to it, only completely opaque and unmasked layer buffer contents go this Shm road.

Notes and rationales :
----------------------

1) For all the XShm circuitry, I initially would have liked to use cairo's new cairo-xlib-surface-shm features, but I discovered it did not support shared 'Images' (no XShmCreateImage), but only shared Pixmap (and we want to avoid going the shared pixmap way, since it's often not supported on recent linux setups). That is why I chose to use nsShmImage, since it had already all the required features. Now, unfortunately, this nsShmImage class is confined in /widgets, that's why for now, you see that I made it export itself in a not-so-nice way. This should be addressed further. Then, I made some extension to the class, adding a more general 'Put' that would take a drawable.

2) You see that mShmImageBufferOnWhite is *not* taken care of yet. It's planned for after we go over the patch together and get it reviewed by relevant people.

3) You finally see there's this weird change at moz.build to get RotatedBuffer.cpp out of the UNIFIED_SOURCES list. This is only temporary, as I had namespace conflicts on Xlib stuff probably caused by some other cpp "using" the mozilla::gfx namespace in the global scope. I'll fix that later.
Comment 26 Frederic Plourde (:fred23) 2014-04-29 09:36:13 PDT
Created attachment 8414559 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-OMTC-basic.patch
Comment 27 Benoit Jacob [:bjacob] (mostly away) 2014-04-29 10:38:03 PDT
Comment on attachment 8412069 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC.patch

Review of attachment 8412069 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not very qualified to comment on this, so I'm clearing my feedback flag, but I can help identify the right people to comment on different parts:

 - Karl is probably the right person for nsShmImage.cpp.
 - Bas or Nical are probably the right person for RotatedBuffer.* and ContentClient.*.

Without knowing much about this code myself, I have one concern: ContentClient and RotatedBuffer are meant to be generic, cross-platform code. I wonder if it could be avoided to stick X11-specific paths directly there. Bas and Nical can comment on this.

Removing Snorp (apparently not involved so far) and jrmuizel (on PTO at the moment).
Comment 28 George Wright (:gw280) (:gwright) 2014-04-29 12:37:06 PDT
Comment on attachment 8412069 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC.patch

Review of attachment 8412069 [details] [diff] [review]:
-----------------------------------------------------------------

Some quick comments. By no means a comprehensive review.

::: gfx/layers/RotatedBuffer.cpp
@@ +114,5 @@
>    }
>  
> +  // We can go the XShmPutImage road if we have opaque cairo target without mask!
> +  if (mShmImageBuffer && aOpacity == 1.0f && !aMask &&
> +      aTarget->GetType() == BackendType::CAIRO) {

Do we want to account for the BackendType::SKIA case here yet? If not, probably put a comment saying we'll want to in the future.

@@ +129,5 @@
> +    }
> +
> +    nsRefPtr<gfxASurface> surf =
> +      gfxASurface::Wrap((cairo_surface_t *)
> +        aTarget->GetNativeSurface(NativeSurfaceType::CAIRO_SURFACE));

Probably better to use gfxPlatform::GetThebesSurfaceForDrawTarget() here

@@ +137,5 @@
> +                          (surf.get())->XDrawable(), mBufferRect, fillRect);
> +
> +      return;
> +    } 
> +    

Trailing spaces

::: gfx/layers/client/ContentClient.cpp
@@ +118,5 @@
> +  
> +  // We are forcing this thebes layer to use an Image as backend surface so
> +  // also allocate an XShmImage to upload it with no penalty when rendering
> +  // the layer
> +  if (PR_GetEnv("MOZ_LAYERS_USE_IMAGE_SURFACES")) {

Why not a pref?

@@ +126,5 @@
> +                                 mShmImageBuffer);
> +    if (surf) {
> +      *aBlackDT =
> +        gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf,
> +                                           IntSize(aRect.width, aRect.height));

Maybe we would want to roll this functionality into a gfxPlatform::CreateShmDrawTarget()? I'm thinking this will be an important factory and it'd be nice if we could avoid using gfxASurfaces directly in places where we're creating these DrawTargets.
Comment 29 Nicolas Silva [:nical] 2014-04-30 05:46:06 PDT
Comment on attachment 8412069 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC.patch

Review of attachment 8412069 [details] [diff] [review]:
-----------------------------------------------------------------

As Benoit said, RotatedBuffer and ContentClient should not be affected by the type of memory we are using. At least not the code that is used in the OMTC case.
It looks like this is targeting main-thread compositing only, so I can live with a bit of platform specific code if it is entirely contained in ContentClientBasic. Beware that non-omtc code (including ContentClientBasic) is set to be removed soon-ish.
RotatedBuffer and ContentClient are quite hard to maintain as they are now, so I want to be very careful about the complexity we add there. For anything OMTC-related this kind of memory type specificities must be added as TextureClient/Host implementations rather than in the ContentClient/Host and RotatedBuffer code.

So let's find a way to not add platform specific code to RotatedBuffer.
Comment 30 Benoit Jacob [:bjacob] (mostly away) 2014-04-30 06:02:58 PDT
Nical, could you make a specific recommendation as to how this should be implemented, to add these new code paths (around RotatedBuffer/ContentClient) without breaking the principles explained in comment 29?
Comment 31 Frederic Plourde (:fred23) 2014-04-30 06:20:46 PDT
Thanks George for your feedback.
I will update the patch shortly, but for now, some replies/questions to some of your comments : 

(In reply to George Wright (:gw280) from comment #28)
> > +  // We are forcing this thebes layer to use an Image as backend surface so
> > +  // also allocate an XShmImage to upload it with no penalty when rendering
> > +  // the layer
> > +  if (PR_GetEnv("MOZ_LAYERS_USE_IMAGE_SURFACES")) {
> 
> Why not a pref?

Why not indeed, but what is generally the considered criteria at Mozilla for choosing a pref over an Env var ?
 

> > +    if (surf) {
> > +      *aBlackDT =
> > +        gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf,
> > +                                           IntSize(aRect.width, aRect.height));
> 
> Maybe we would want to roll this functionality into a
> gfxPlatform::CreateShmDrawTarget()? I'm thinking this will be an important
> factory and it'd be nice if we could avoid using gfxASurfaces directly in
> places where we're creating these DrawTargets.

That would be an option, but IMHO, much more invasive than what is currently proposed in the patch. But it's worth discussing in here.

See, I'm creating a "regular" DrawTarget here for the Thebes layer in ::CreateContentClient because we'll need to draw to this thebes surface (image-backed) during the paint process just as before using its DrawTargetCairo (even if it's got XShm capabilities). Then, when "rendering" the layer in the compositor phase in RotatedBuffer::DrawBufferQuadrant, I'm not even using the layer DrawTarget because that's not the way it works... see, take a look at RotatedBuffer::DrawBufferQuadrant :

http://lxr.mozilla.org/mozilla-central/source/gfx/layers/RotatedBuffer.cpp#79

We're getting a DrawTarget aTarget as argument. That's our main window widget cairo_xlib surface's DT. And then, take a look at line #111 to 116. We're actually calling our layer's DT's ::snapshot() function to get a SourceSuface that will be composited on top of 'aTarget' using the destination target's ::MaskSurface() or ::DrawSurface() at line#147 or #155 (depending on whether you have a mask or not).

Now, let's say we go the CreateShmDrawTarget() road and create some DrawTargetCairoXShm for the layer (derived from DrawTargetCairo to make sure we can still draw on this layer using cairo), then we'll have to implement those *additional* changes :

1) Make our main widget's DrawTarget also be XShm-capable (DrawTargetCairoXShm) here: 
http://lxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp#2138

2) Subclass SourceSurfaceCairo into SourceSurfaceCairoXShm and somehow make it hold a ref on a nsShmImage passed-in as ctor argument (I'm not sure it would follow moz2D rules/philosophy here to have a DT manipulate XShm stuff.). Add the ::PutImage function here to trigger mShmImage->PutImage() that will be calling XShmPutImage.

3) Overload DrawTargetCairoXShm's ::Snapshot() function and make it create/return a SourceSurfaceCairoXShm when called from here :
http://lxr.mozilla.org/mozilla-central/source/gfx/layers/RotatedBuffer.cpp#113

... then to make the final compositing step work in here :
http://lxr.mozilla.org/mozilla-central/source/gfx/layers/RotatedBuffer.cpp#147   and
http://lxr.mozilla.org/mozilla-central/source/gfx/layers/RotatedBuffer.cpp#155

4) Implement DrawTargetCairoXShm ::MaskSurface() so that it masks the surface with Cairo just as before (because we can't support masking with XShmPutImage)

5) Implement DrawTargetCairo::DrawSurface, taking the SourceSurfaceCairoXShm as argument, and, if aOpacity is not 1.0, revert to using Cairo as before and if we're totally opaque, THEN that's when we will do something like :  "aSurface->PutImage()"


that's about it.
So you see, your suggestion has the advantage of *unifomizing* the way we render layers in DrawBufferQuadrant (that's nice), but it's way more invasive and required us to subclass more stuff.

I'm really not disregarding this alternate solution... guys ?  I'd like your input on this.. Bas ? Nicolas ?
thanks George :)
Comment 32 Frederic Plourde (:fred23) 2014-04-30 06:22:06 PDT
(In reply to Nicolas Silva [:nical] from comment #29)
> So let's find a way to not add platform specific code to RotatedBuffer.

hey, I guess we wrote our comments at approx. the same time. ;-)
Would what I just replied to George be a good starting point for what you just suggested maybe ?
Comment 33 Nicolas Silva [:nical] 2014-04-30 06:27:28 PDT
Comment on attachment 8412069 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC.patch

Review of attachment 8412069 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, I changed my mind, sorry. I think this is is fine because the xshm stuff is not that invasive and doesn't add too much logic. While looking fr a way to move that out of RotatedBuffer I felt that it would not improve that much. However, let's insist in the comments that these code paths are never taken with OMTC. because this area of the code is very hard to understand and debug with OMTC.
The general rule for future additions to RotatedBuffer is try to avoid adding states to it and avoid adding platform specific logic, but the current main-thread code doesn't help at all with decoupling the logic and the memory type, so in the case of your patch it's sufficiently simple that it's fine.

::: gfx/layers/RotatedBuffer.cpp
@@ +113,5 @@
>      }
>    }
>  
> +  // We can go the XShmPutImage road if we have opaque cairo target without mask!
> +  if (mShmImageBuffer && aOpacity == 1.0f && !aMask &&

Please add a comment saying that this branch is only taken in the main-thread compositing scenario

::: gfx/layers/RotatedBuffer.h
@@ +136,5 @@
> +   * ThebesLayer content (as per preference MOZ_LAYERS_USE_IMAGE_SURFACES).
> +   * In this case, mShmImageBuffer and mShmImageBufferOnWhite will be used
> +   * instead of base-class' DrawTargets (unless we hit a fallback) and then
> +   * 'mShmImageBuffer != nullptr' will be used to check if we're going the
> +   * shared image path.

Please also say that when we do off-main-thread compositing, mShmImageBuffer[OnWhite] are always null
Comment 34 Nicolas Silva [:nical] 2014-04-30 06:53:16 PDT
About pref vs environment variable, prefs are easier to setup on a per configuration basis on our test servers, so if only for that, I would favor using a pref. Usually we use prefs for features like this, and environment variables for debug stuff like activate some logging.
Comment 35 Nicolas Silva [:nical] 2014-04-30 07:00:50 PDT
(In reply to Frederic Plourde (:fred23) from comment #32)
> (In reply to Nicolas Silva [:nical] from comment #29)
> > So let's find a way to not add platform specific code to RotatedBuffer.
> 
> hey, I guess we wrote our comments at approx. the same time. ;-)
> Would what I just replied to George be a good starting point for what you
> just suggested maybe ?

I think that we really need to split all of what RotatedBuffer and ContentClient do into (mostly) stateless functions and reorganize everything so that we can more easily factor things out and avoid the current spaghetti logic. The current code makes it hard to not have platform specific logic mixed with the rest and it is beyond the scope of this bug. We can and should reorganize this later though.
Comment 36 Frederic Plourde (:fred23) 2014-05-01 10:19:31 PDT
Created attachment 8415958 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_2.patch
Comment 37 Frederic Plourde (:fred23) 2014-05-01 10:23:15 PDT
Created attachment 8415961 [details]
capture.ogv

Screen capture showing placement problem
Comment 38 Frederic Plourde (:fred23) 2014-05-01 10:25:50 PDT
Created attachment 8415965 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_2.patch

(In reply to George Wright (:gw280) from comment #28)
> Comment on attachment 8412069 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-on-MTC.patch
> > +  // We can go the XShmPutImage road if we have opaque cairo target without mask!
> > +  if (mShmImageBuffer && aOpacity == 1.0f && !aMask &&
> > +      aTarget->GetType() == BackendType::CAIRO) {
> 
> Do we want to account for the BackendType::SKIA case here yet? If not,
> probably put a comment saying we'll want to in the future.

In fact, nope. Here's we're talking about aTarget BackendType and in our case, using XShmPutImage, we'll never be uploading content onto surfaces which are *not* CAIRO xlib.
 
> @@ +129,5 @@
> > +    }
> > +
> > +    nsRefPtr<gfxASurface> surf =
> > +      gfxASurface::Wrap((cairo_surface_t *)
> > +        aTarget->GetNativeSurface(NativeSurfaceType::CAIRO_SURFACE));
> 
> Probably better to use gfxPlatform::GetThebesSurfaceForDrawTarget() here

totally right.
Done.

> @@ +137,5 @@
> > +                          (surf.get())->XDrawable(), mBufferRect, fillRect);
> > +
> > +      return;
> > +    } 
> > +    
> 
> Trailing spaces

Done.
 
> ::: gfx/layers/client/ContentClient.cpp
> @@ +118,5 @@
> > +  
> > +  // We are forcing this thebes layer to use an Image as backend surface so
> > +  // also allocate an XShmImage to upload it with no penalty when rendering
> > +  // the layer
> > +  if (PR_GetEnv("MOZ_LAYERS_USE_IMAGE_SURFACES")) {
> 
> Why not a pref?

Done.
I have added a pref "With Observer" here because I figured it could make sense for me to change the pref at runtime and see what happens for debugging purposes. If that's of no use for users, I'll just make it a plain on-gfxPlatform-Init-only pref.

 
> @@ +126,5 @@
> > +                                 mShmImageBuffer);
> > +    if (surf) {
> > +      *aBlackDT =
> > +        gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf,
> > +                                           IntSize(aRect.width, aRect.height));
> 
> Maybe we would want to roll this functionality into a
> gfxPlatform::CreateShmDrawTarget()? I'm thinking this will be an important
> factory and it'd be nice if we could avoid using gfxASurfaces directly in
> places where we're creating these DrawTargets.

So we talked about that with Nical, and seems we will stick to this version of the patch, not touching DrawTargets at all.


(In reply to Nicolas Silva [:nical] from comment #33)
> Comment on attachment 8412069 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-on-MTC.patch

> ::: gfx/layers/RotatedBuffer.cpp
> @@ +113,5 @@
> >      }
> >    }
> >  
> > +  // We can go the XShmPutImage road if we have opaque cairo target without mask!
> > +  if (mShmImageBuffer && aOpacity == 1.0f && !aMask &&
> 
> Please add a comment saying that this branch is only taken in the
> main-thread compositing scenario

Done.

> ::: gfx/layers/RotatedBuffer.h
> @@ +136,5 @@
> > +   * ThebesLayer content (as per preference MOZ_LAYERS_USE_IMAGE_SURFACES).
> > +   * In this case, mShmImageBuffer and mShmImageBufferOnWhite will be used
> > +   * instead of base-class' DrawTargets (unless we hit a fallback) and then
> > +   * 'mShmImageBuffer != nullptr' will be used to check if we're going the
> > +   * shared image path.
> 
> Please also say that when we do off-main-thread compositing,
> mShmImageBuffer[OnWhite] are always null

Done.


However guys, I'm still asking for "feedback" instead of "reviews", because it appears I'm having a bug with the patch concerning the placement of the source surface at compositing step. I'm attaching above a screencast (capture.ogv) to show the problem.. Maybe one of you will have ideas on what could possibly be going wrong. thanks !
Comment 39 Frederic Plourde (:fred23) 2014-05-01 13:11:13 PDT
Created attachment 8416080 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_3.patch

OK, this patch (#3) works like a charm. I had forgotten to apply aTarget transform (translation) to destination rect.

There is one occasional segfault, but I still have to make sure this was not already present without the patch applied. More news to come on that front. For now, any feedback would be highly appreciated.

(See previous comment for updates I made to the patch since last feedback)
Comment 40 George Wright (:gw280) (:gwright) 2014-05-01 13:20:16 PDT
Comment on attachment 8416080 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_3.patch

Review of attachment 8416080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. Can we push this to try with the pref turned on to see what it's like?
Comment 41 Marco Castelluccio [:marco] 2014-05-01 14:37:01 PDT
FYI, there are some new posts on https://ickle.wordpress.com/ that contain new comparisons between the different backends.
Comment 42 Nicolas Silva [:nical] 2014-05-02 06:06:53 PDT
Comment on attachment 8416080 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_3.patch

Review of attachment 8416080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, except the for the prefs observer boilerplate. Let's use gfxPrefs for this instead (see comment below).

::: gfx/layers/RotatedBuffer.cpp
@@ +117,5 @@
>  
> +  // We can go the XShmPutImage road only if we have an opaque cairo target
> +  // without mask nor target surface scaling.
> +  // Note that we are taking this branch only in the MTC case.
> +  // Note also that we only care here for BackendType::CAIRO, since 

nit: trailing space here and a few lines below

::: gfx/thebes/gfxPlatform.cpp
@@ +214,5 @@
>  
>      return NS_OK;
>  }
>  
> +class LayersUseImageSurfacesPrefsObserver MOZ_FINAL : public nsIObserver

Please use gfxPrefs (see gfxPrefs.h) for this. It's super simple and groups our prefs stuff in one place which is also nice. Also, let's call the pref "layers.use-xshm-surface" and query it with gfxPrefs::UseXShmSurface() in gfxPrefs.
ImageSurface can be confused with other stuff.
Comment 43 Frederic Plourde (:fred23) 2014-05-06 06:05:37 PDT
Thanks for your feedback guys, much appreciated.
While testing my patch, I noticed a regressions (crash) when resizing the window. I'll fix that and resubmit for review.
Comment 44 Karl Tomlinson (:karlt) 2014-05-08 19:05:26 PDT
IIUC the goal here is to use image surfaces for Thebes layers so that Skia can
be used to draw into these layers.

Echoing bug 496204 comment 17 more verbosely, these are my assumptions:

1. Image surfaces will only be used with GTK3.
   Otherwise native theme drawing will slow things down too much.
   Once we have a GTK3 port that most users can use, we can tolerate a
   slow GTK2 port as a fallback for those without GTK3.

2. The GTK3 port will use system cairo.

3. System cairo will use XShm, if available, for large uploads from image
   surfaces to Xlib surfaces (at least for versions of reasonable vintage).

(In reply to Frederic Plourde (:fred23) from comment #25)
> 1) For all the XShm circuitry, I initially would have liked to use cairo's
> new cairo-xlib-surface-shm features, but I discovered it did not support
> shared 'Images' (no XShmCreateImage), but only shared Pixmap (and we want to
> avoid going the shared pixmap way, since it's often not supported on recent
> linux setups).

This I didn't follow.  Cairo will use XShm automatically for large uploads from
image to xlib surfaces, whether or not shared pixmaps are available.

What doing XShmCreateImage in Gecko provides, I assume, is that it is no
longer necessary to do an extra client-side copy from plain memory to shared
memory (which is what cairo would do).

Is it really necessary to do this optimization?  Is it worth the extra code
paths?

Rather than doing this optimization, I would just use plain memory surfaces, ensure there are no remaining places choosing xlib surfaces for drawing, and put any optimization time and effort into the OffMTC and/or GL code paths, so that users can benefit there.  IIUC these paths are already designed from platform-specific optimizations.

> That is why I chose to use nsShmImage, since it had already
> all the required features.

It has some necessary features, but doesn't do the lazy synchronization
optimization that cairo provides.  The lazy syncrhonization may save a big
performance regression from the XSync()s.
Comment 45 Frederic Plourde (:fred23) 2014-05-09 05:58:36 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #44)
> IIUC the goal here is to use image surfaces for Thebes layers so that Skia
> can
> be used to draw into these layers.
> 
> Echoing bug 496204 comment 17 more verbosely, these are my assumptions:
> 
> 1. Image surfaces will only be used with GTK3.
>    Otherwise native theme drawing will slow things down too much.
>    Once we have a GTK3 port that most users can use, we can tolerate a
>    slow GTK2 port as a fallback for those without GTK3.
> 
> 2. The GTK3 port will use system cairo.
> 
> 3. System cairo will use XShm, if available, for large uploads from image
>    surfaces to Xlib surfaces (at least for versions of reasonable vintage).

I totally agree on those assumptions.
 
> (In reply to Frederic Plourde (:fred23) from comment #25)
> > 1) For all the XShm circuitry, I initially would have liked to use cairo's
> > new cairo-xlib-surface-shm features, but I discovered it did not support
> > shared 'Images' (no XShmCreateImage), but only shared Pixmap (and we want to
> > avoid going the shared pixmap way, since it's often not supported on recent
> > linux setups).
> 
> This I didn't follow.  Cairo will use XShm automatically for large uploads
> from
> image to xlib surfaces, whether or not shared pixmaps are available.

That's right, my bad. Cairo is creating XImages and doing XShmPutImage whether or not you're supporting Shared Pixmaps, maybe I did not fully/correctly explained the reasons under this choice of "not" going the cairo way... and I explain it better hereunder :

> 
> What doing XShmCreateImage in Gecko provides, I assume, is that it is no
> longer necessary to do an extra client-side copy from plain memory to shared
> memory (which is what cairo would do).

This is the reason why I initially thought using XShmCreateImage would make us gain over cairo regular image->xlib XPutImage, because I felt layers turnover can be high sometimes, hence give rise to a significant overhead. 

> Is it really necessary to do this optimization?  Is it worth the extra code
> paths?

You guys know the code behaviour way better than I do... You're telling me this would be an unnecessary optimization, and I can certainly consider that, and letting cairo manage the upload would anyway make gecko more uniform with regards to drawTargets. Others ?  What do you think ?

> Rather than doing this optimization, I would just use plain memory surfaces,
> ensure there are no remaining places choosing xlib surfaces for drawing, and
> put any optimization time and effort into the OffMTC and/or GL code paths,
> so that users can benefit there.  IIUC these paths are already designed from
> platform-specific optimizations.

GL OMTC is already on its way and is planned right after this patch is reviewed and accepted.
We're on the same page on that front ;)

> > That is why I chose to use nsShmImage, since it had already
> > all the required features.
> 
> It has some necessary features, but doesn't do the lazy synchronization
> optimization that cairo provides.  The lazy syncrhonization may save a big
> performance regression from the XSync()s.

right. good catch.
Comment 46 Karl Tomlinson (:karlt) 2014-05-11 17:58:30 PDT
The more important thing here, I think, is that we don't have every layer
being uploaded to the server on every composite, and the patch here already
provides what it necessary to avoid that (when XShm can be used) - only
damaged layers are uploaded.

Achieving this was awkward, I assume because these layers classes are not
designed to have a platform-dependent upload path.  (GL layers classes must
provide this, but I guess those classes are not involved here - I don't really
know.)  Using cairo instead of XShm directly is not going to make this less
awkward.  I assume OffMTC classes will better provide for this.

Right now, for OnMTC, whether XShm is used directly or via cairo is probably
less important than uploading only damaged layers.  I suggest proceeding with
the current plan if that is the quickest way to finish up here and move onto
OffMTC.

One thing to watch out for though is, if XShm is not available, the current
patch will result in fallback to existing paths, which I assume use Xlib
surfaces.  If the thebes layers MUST be image surfaces then there still needs
to be something else to force image surfaces.

For OffMTC, my guess is that cairo's lazy synchronization is going to be a
more important optimization than skipping a client-side copy, but it is hard
to know for sure without trying.  Perhaps a bigger advantage of the using
cairo is that it will handle fallback from XShmPutImage to XPutImage for us,
so there will likely be fewer code paths to write/maintain.
Comment 47 Frederic Plourde (:fred23) 2014-05-13 07:53:50 PDT
Created attachment 8421722 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_5.patch

Hi guys,  here's an updated patch :

  1) Crash on resize has been fixed.
  2) X_ShmPutImage X error has been fixed
  3) All previous comments/concerns from reviewers have been addressed, detailed hereunder :

(In reply to Nicolas Silva [:nical] from comment #42)
> Comment on attachment 8416080 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-on-MTC_3.patch
> 
> Review of attachment 8416080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, except the for the prefs observer boilerplate.
> Please use gfxPrefs (see gfxPrefs.h) for this.

Done.
However, I named it : useXshmSurfaces (with an 's') because from a general
gfxPrefs perspective, we are going to be creating many surfaces.



(In reply to Karl Tomlinson (needinfo?:karlt) from comment #46)
> The more important thing here, I think, is that we don't have every layer
> being uploaded to the server on every composite, and the patch here already
> provides what it necessary to avoid that (when XShm can be used) - only
> damaged layers are uploaded.
> 
> Achieving this was awkward, I assume because these layers classes are not
> designed to have a platform-dependent upload path.  (GL layers classes must
> provide this, but I guess those classes are not involved here - I don't
> really
> know.)  Using cairo instead of XShm directly is not going to make this less
> awkward.  I assume OffMTC classes will better provide for this.

yep, as they will make use of those Gl layer classes. That's the plan.

> Right now, for OnMTC, whether XShm is used directly or via cairo is probably
> less important than uploading only damaged layers.  I suggest proceeding with
> the current plan if that is the quickest way to finish up here and move onto
> OffMTC.

I totally agree, we should get this patch in soon so we can expand to OMTC + OGL.

> One thing to watch out for though is, if XShm is not available, the current
> patch will result in fallback to existing paths, which I assume use Xlib
> surfaces.  If the thebes layers MUST be image surfaces then there still needs
> to be something else to force image surfaces.

That's been taken care of in this newly submitted patch.
I'm forcing gfxImageSurface creation at the GfxPlatformGtk::CreateOffscreenSurface
level instead, and all the paths are going to end up there.

Now, enabling gfxImageSurface creation for everything that goes down through
the platform made one visual artifact appear. I'm still submitting the patch
here for feedback because I feel we're really close to landing this.
Please, help me get a hint of what's happenning here, see : 

https://www.youtube.com/watch?v=J_H8eBb-ABU

Once this is fixed, I believe the patch is ready for review.
Comment 48 Frederic Plourde (:fred23) 2014-05-13 08:05:46 PDT
Comment on attachment 8414559 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-OMTC-basic.patch

Karlt : please have a look at this patch for OMTC basic. It uses the same old XShmCreateImage path, but I guess for OMTC, I could let cairo manage the upload for us, as you suggested... The patch is still WIP, but other than than, any comment on the patch ? thanks.

Milan and Benoit : Remember on last meeting I said I would point you to what I've already come up with with regards to OMTC. That's the one. Please, get me to the active OMTC people so we can move forward on that front. thanks :)
Comment 49 Frederic Plourde (:fred23) 2014-05-13 08:52:09 PDT
Comment on attachment 8421722 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_5.patch

Sorry, feedback will be enough for now (instead of review), as there's one remaining glitch with gfxImageSurface enablement from gfxPlatformGtk::CreateOffscreenSurface which I intend to fix ASAP : 

Any hint on what could possibly be happening from this screen cast ?
https://www.youtube.com/watch?v=J_H8eBb-ABU
Comment 50 Benoit Jacob [:bjacob] (mostly away) 2014-05-13 10:23:22 PDT
Comment on attachment 8414559 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-OMTC-basic.patch

Nical and Bas are the right reviewers for the OMTC / Layers aspects of this work.
Comment 51 Nicolas Silva [:nical] 2014-05-14 07:16:27 PDT
Comment on attachment 8421722 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_5.patch

Review of attachment 8421722 [details] [diff] [review]:
-----------------------------------------------------------------

the "layers" part of things look good to me.
Comment 52 Karl Tomlinson (:karlt) 2014-05-15 23:32:37 PDT
Comment on attachment 8421722 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-on-MTC_5.patch

I'm sorry, I think I've misunderstood what was happening here.
I mis-assumed that this was using XShm to upload *to* the buffer.
In fact, it seems that DrawBufferQuadrant is uploading from the buffer.

This has the problem that the uploaded surface is not buffered and so, during
a composite, the upload must happen for every quadrant of every buffer/layer.

Unfortunately, I think that is going to strip any efficiency gained from
compositing on the GPU.  It would be more efficient I expect to composite with
the CPU and do a final single upload to the window, as is already provided I assume when UseXRender() returns false.

Although it would be possible to upload when drawing to the buffer, I guess
the classes have not been designed for this.  I'm not clear on the priorities
for the different goals in this project, but UseXRender() already provides a
fallback if we need one before OMTC is up and working well enough, so I
suggest just checking that UseXRender() false is behaving correctly, even when
XShm is not available, and then focus on OMTC.

(It may be a few days before I can provide useful feedback on the OMTC patch.)
Comment 53 Frederic Plourde (:fred23) 2014-05-16 07:57:09 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #52)
> Comment on attachment 8421722 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-on-MTC_5.patch
> 
> I'm sorry, I think I've misunderstood what was happening here.
> I mis-assumed that this was using XShm to upload *to* the buffer.
> In fact, it seems that DrawBufferQuadrant is uploading from the buffer.
> 
> This has the problem that the uploaded surface is not buffered and so, during
> a composite, the upload must happen for every quadrant of every buffer/layer.
> 
Yeah, well, having to dig into the code completely to understand how it worked, i did realize that lack in the code to buffer the layer content post-upload, but I'm quite sure I had drawn some people's attention on this previously, saying I found it weird enough (maybe you were not in the loop at that point, it's been a long time unfortunately) and having said that, I thought you guys wanted to go down that road, still.

> Unfortunately, I think that is going to strip any efficiency gained from
> compositing on the GPU.  It would be more efficient I expect to composite
> with
> the CPU and do a final single upload to the window, as is already provided I
> assume when UseXRender() returns false.

yeah, when UseXrender() is false, we're already creating gfxXImages that will be compositing in SW onto an nsShmImage-wrapped ThebesSurfaces (our destTarget) and this shared image is sent to widget's window drawable with XShmPutImage from inside nsShmImage class.

> Although it would be possible to upload when drawing to the buffer, I guess
> the classes have not been designed for this.  I'm not clear on the priorities
> for the different goals in this project, but UseXRender() already provides a
> fallback if we need one before OMTC is up and working well enough, so I
> suggest just checking that UseXRender() false is behaving correctly, even
> when
> XShm is not available, and then focus on OMTC.

Please, keep on giving feedback on this bug and the OMTC one (for which I'll paste the links hereunder, should I change the bug's thread for this one) because now I need to get as many relevant people to join in and help validate OMTC 'ogl' design before posting patch for a third round of review, thanks.
Comment 54 Karl Tomlinson (:karlt) 2014-05-16 16:06:06 PDT
(In reply to Frederic Plourde (:fred23) from comment #53)
> Yeah, well, having to dig into the code completely to understand how it
> worked, i did realize that lack in the code to buffer the layer content
> post-upload, but I'm quite sure I had drawn some people's attention on this
> previously, saying I found it weird enough (maybe you were not in the loop
> at that point, it's been a long time unfortunately) and having said that, I
> thought you guys wanted to go down that road, still.

I think no one is really clear on exactly how best to implement a solution here.
In the end the proof is in measurements of performance/cpu use etc.  I hope we can make some sensible guesses at what will perform well, but some things will end up just being experiments.  Regardless, a large part of the progress here is learning how things work so we can find a good solution.

I have been trying to follow discussions here, but there have been parts where I haven't understood the terms or class names, so I'm sorry if I missed this and wasn't able to speak up sooner.

One thing that is better about the XShm RotatedBuffer.cpp approach than UseXRender() false is that windowless plugins will perform better because their Pixmaps do not need to be read back into image surfaces.  I'm sure we could work to tune things and have a variant that would perform well on both plugins and thebes/content layers, but I think this will soak too much time and we should instead focus on OMTC.

My reason for suggesting focusing on OMTC is that I expect that by the time we have GTK3 shipping, we will no longer be using the (on) MTC path.  The graphics team may be better informed than i here, and so should feel free to overrule.
One of the problems with advice is that it is often impossible to follow all of it, and so please feel free to process it carefully.
Comment 55 Chris Lord [:cwiiis] 2014-05-17 01:47:01 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #54)
> My reason for suggesting focusing on OMTC is that I expect that by the time
> we have GTK3 shipping, we will no longer be using the (on) MTC path.  The
> graphics team may be better informed than i here, and so should feel free to
> overrule.

This is true, Linux OMTC is going to be turned on very soon (bug 994541).
Comment 56 Karl Tomlinson (:karlt) 2014-05-18 22:09:24 PDT
Comment on attachment 8414559 [details] [diff] [review]
Remove-content-Xlib-surfaces-for-OMTC-basic.patch

Similarly to the MTC patch, I don't think Compositor::DrawQuad() is the right
place for an upload.  The upload is not cached and arbitrary transforms could
be involved here.

https://wiki.mozilla.org/Gecko:Overview#Compositing is very useful.
Having a look around, it looks like DataTextureSource::Update() is the method
that we want to perform uploads.  An implementation of this that uploads image
surfaces to Pixmaps (in cairo xlib surfaces) would be sensible when using XRender to composite.

Perhaps this could be hooked up by creating an XRenderCompositor, derived
from the BasicCompositor, that overrides CreateDataTextureSource().

Again, people more familiar with the code may have better suggestions.

When using OpenGL to composite image surfaces, the appropriate
DataTextureSource is already implemented.

I don't know of a reason why a TextureClient from
CreateTextureClientForDrawing() should use XShm but one from
CreateBufferTextureClient() should not, but I suspect this will sort itself
out with the suggestions above.
Comment 57 Chris Lord [:cwiiis] 2014-05-19 07:44:38 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #56)
> Comment on attachment 8414559 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-OMTC-basic.patch
> 
> Similarly to the MTC patch, I don't think Compositor::DrawQuad() is the right
> place for an upload.  The upload is not cached and arbitrary transforms could
> be involved here.
> 
> https://wiki.mozilla.org/Gecko:Overview#Compositing is very useful.
> Having a look around, it looks like DataTextureSource::Update() is the method
> that we want to perform uploads.  An implementation of this that uploads
> image
> surfaces to Pixmaps (in cairo xlib surfaces) would be sensible when using
> XRender to composite.
> 
> Perhaps this could be hooked up by creating an XRenderCompositor, derived
> from the BasicCompositor, that overrides CreateDataTextureSource().
> 
> Again, people more familiar with the code may have better suggestions.
> 
> When using OpenGL to composite image surfaces, the appropriate
> DataTextureSource is already implemented.
> 
> I don't know of a reason why a TextureClient from
> CreateTextureClientForDrawing() should use XShm but one from
> CreateBufferTextureClient() should not, but I suspect this will sort itself
> out with the suggestions above.

I don't know the details of this bug, but it sounds like you want to create a new TextureClient/Host. If the first comment is still accurate, you would likely only need a new TextureHost in fact, there's already a shared memory TextureClient (though you may want some logic to make sure it always gets picked in the X11 case?).

Depending on whether this is with software compositing or GL compositing, I'd guess you'd want to take a look at TextureHostBasic (which seemingly already has a Mac-specific path in CreateTextureHostBasic), or TextureHostOGL.

CreateBufferTextureClient is to create a TextureClient that specifically has an in-system-memory buffer - though I suppose that doesn't preclude using XShm, the functions have evolved to pretty much mean backend-specific, 'accelerated' texture client (CreateTextureClientForDrawing) and 'software' texture client (CreateBufferTextureClient). The former will fall back to the latter if such a client doesn't exist/can't be created.
Comment 58 Frederic Plourde (:fred23) 2014-05-19 11:00:53 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #56)
> Comment on attachment 8414559 [details] [diff] [review]
> Remove-content-Xlib-surfaces-for-OMTC-basic.patch
> 
> Similarly to the MTC patch, I don't think Compositor::DrawQuad() is the right
> place for an upload.  The upload is not cached and arbitrary transforms could
> be involved here.

Yeah, in fact, I didn't have time to update this OMTC patch before you made comments, but knowing your position on the MTC patch, I could have told you'd have those comments on the OMTC one ;) I'm saying this with a smile though, because I think there are some really nice ideas in what you're suggesting. I'm currently working on (re)writing OMTC (basic + OGL) patches and will carefully consider what you just said. thanks.
Comment 59 Frederic Plourde (:fred23) 2014-05-21 05:40:34 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #56)
> Similarly to the MTC patch, I don't think Compositor::DrawQuad() is the right
> place for an upload.

It might not be the right place for uploads, as you say there are arbitraty transforms here and it's not buffered, but having had another look at this, I remember why I still hacked my way around at the ::DrawQuad level in a very similar way to what I did for MTC : In my case, OMTC basic was *not* buffered (just as MTC path had *no Compositor*, compositing directly from the Buffer), and I just discovered why... it's because BasicCompositor is considered "not stable enough for regular use" and we need to set this pref to force it to be used in the OMTC path : 

layers.offmainthreadcomposition.force-basic.

See : http://lxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsBaseWidget.cpp#875

It's setting the hint to LAYERS_NONE when on the "basic" path with the pref set to false.
Now, a short while after, this makes our CompositorParent *not* create any Compositor at all (hence falling back to a non-buffered scheme as in the MTC case) instead of having our BasicCompositor as we'd expect.

here : http://lxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#873

Now, by setting the pref *and* by writing /gtk/nsWindow::StartRemoteDrawing() GTK3 implementation (which was NOT done already), I could make the BasicCompositor kick in fine.

Karl: Chris: but... do you guys know more about this "BasicCompositor is not stable enough" statement that I mentioned previously ? I think the answer to this question will have a significant impact on the OMTC basic work. We should get more info on whether or not we want to be using BasicCompositor in the "regular" case and what initially led us to create that pref.
Comment 60 Frederic Plourde (:fred23) 2014-05-21 05:47:56 PDT
Here's the change that brought this "fallback for OMTC"

changeset:   142986:c63a1c48560b
user:        Nicholas Cameron <ncameron@mozilla.com>

Nicholas ?  Any hint about reasons/possible bugs for not setting LAYERS_BASIC in the OMTC without the force-basic pref...would be highly appreciated :)
Comment 61 Frederic Plourde (:fred23) 2014-05-21 09:55:33 PDT
Ok guys, got my answer, no need to reply.
The re-enablement of BasicCompositor for OMTC linux is being taken care of as we speak by Chris in Bug 994541. cheers!
Comment 62 Frederic Plourde (:fred23) 2014-05-22 08:45:13 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #56)
> https://wiki.mozilla.org/Gecko:Overview#Compositing is very useful.
> Having a look around, it looks like DataTextureSource::Update() is the method
> that we want to perform uploads.  An implementation of this that uploads
> image
> surfaces to Pixmaps (in cairo xlib surfaces) would be sensible when using
> XRender to composite.

Looks like ::Update functions are indeed the right place for an upload in a buffered OMTC scheme :)
Now that I have made BasicCompositor in OMTC linux kick in again (patch to come very soon), I'm having actual DataTextueSources and am at the moment designing something to upload to Xlib cairo drawtargets for buffering.
 
> Perhaps this could be hooked up by creating an XRenderCompositor, derived
> from the BasicCompositor, that overrides CreateDataTextureSource().

See here, I wouldn't subclass BasicCompositor, because we're still having the "basic" behavior... so I'd stick to the "basic" path (which is, instead of using, say, OGL to composite, is using cairo... and cairo will call XRender() for us to composite). I'm trying to come up with a nice way to integrate this into existent classes. 

> Again, people more familiar with the code may have better suggestions.

Yeah, any comments from nical, Bas, much appreciated if you've got free cycles. thanks.
Comment 63 Frederic Plourde (:fred23) 2014-05-23 07:41:00 PDT
I will keep this bug/patch pending until I get numbers from testing both solutions with Talos. Stay tuned.
Comment 64 Frederic Plourde (:fred23) 2014-05-27 13:13:13 PDT
Created attachment 8429502 [details]
talos_test_results_for_XshmCreatImage_Optimization.txt

Ok, I have finally got some numbers with talos sessions that I drove locally for the tart/tscroll/tresize tests. You can find the raw data in the attached result file.

Basically, the numbers show that the upload optimization we implemented using nsShmImage and XShmCreateImage at layer creation incurs performance degradation. So the proposed patch will be deprecated and another patch will be posted here, a simpler patch that just disables xlib offscreen surfaces from nsWindow::CreateOffscreenSurface
Comment 65 Frederic Plourde (:fred23) 2014-05-27 13:45:04 PDT
Created attachment 8429542 [details]
results_UseXRender.txt

This talos output is for REGULAR UseXRender() MTC GTK3 builds under Linux
Comment 66 Frederic Plourde (:fred23) 2014-05-27 13:46:05 PDT
Created attachment 8429544 [details]
results_imageOffscreenSurfaces.txt

This talos output is for imageOffscreenSurface (NO UseXRender()) MTC GTK3 builds under Linux
Comment 67 Frederic Plourde (:fred23) 2014-05-27 13:57:53 PDT
Created attachment 8429555 [details] [diff] [review]
Remove_Xlib_surfaces_from_CreateOffscreenSurface.patch

This simple patch reverts this MTC work to simply forcing the use of "image" offscreen surfaces and to using cairo for final layer content upload to our Xlib drawable (un-buffered compositing), just as previously suggested by Karl.

Since layers in the MTC basic case are not buffered, there's no way to "upload" layer content to gpu memory for upcoming compositing to be faster, hence, when Xlib surfaces will be completely removed, we'll take a performance hit in the MTC paths (if they still exist).

I have run local talos test for tart/tscrollx/tresize and it's showing an "expected" perf degradation between the REGULAR UseXRender case and this patch's case which makes no use of Xrender acceleration for compositing but rather use cairo's internal XshmPutImage when appropriate.


Regular path with UseXRender() == true
https://bug738937.bugzilla.mozilla.org/attachment.cgi?id=8429542

This patch'S image offscreen surfaces
https://bug738937.bugzilla.mozilla.org/attachment.cgi?id=8429544


Karl: please take a look at those numbers.  It's not rare to see 34% increase in resize/tabDrawing/etc.. time.
Comment 68 Karl Tomlinson (:karlt) 2014-05-27 18:41:51 PDT
Comment on attachment 8429555 [details] [diff] [review]
Remove_Xlib_surfaces_from_CreateOffscreenSurface.patch

Thanks for comparing the performance, Frederic.

Just checking: did the results in attachment 8429544 [details] (and the seconds set of results in attachment 8429502 [details]) come from a run with UseXRender() returning true?

If it's not too much trouble, it may be interesting to compare with UseXRender returning false.  The differences with false would be that compositing would happen onto image surfaces (without uploads for each layer) and uploads (of the final composited result) would be synchronous with nsShmImage.
However, it may be difficult to interpret such a comparison given that there are 2 major differences.

A member of the gfx team should review this.
Comment 69 Frederic Plourde (:fred23) 2014-05-28 07:07:16 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #68)
> Comment on attachment 8429555 [details] [diff] [review]
> Remove_Xlib_surfaces_from_CreateOffscreenSurface.patch
> 
> Thanks for comparing the performance, Frederic.
> 
> Just checking: did the results in attachment 8429544 [details] (and the
> seconds set of results in attachment 8429502 [details]) come from a run with
> UseXRender() returning true?

To answer your question, yes, the second set has UseXRender() returned true in the test, but little precision here, remember those are numbers for the MTC (basic). On this path, there is absolutely NO other use to UseXRender() than the one in 

http://lxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#95

So this means it does not matter and we're indeed comparing the two cases on a valid basis. Here :

In Regular path using XRender: https://bug738937.bugzilla.mozilla.org/attachment.cgi?id=8429542,
We are creating xlib offscreen surfaces for layers on content side.
We are then making up final frame by directly compositing those xlib surfaces to widget's drawable in RotatedBuffer::DrawBufferQuadrant and the *only* criteria for choosing how to composite is NOT UseXRender but rather by simply passing the surface to cairo that will, in turn, decide what to do with an xlib surface (use XRender) or with an image surface (use pixman_composite_32 or al.

Look at the 4 combinations : 

1-UseXRender == true and UseImageOffscreenSurfaces == true

  * ContentClientBasic::CreateBuffer will end up in gfxPlatformGtk::CreateOffscreenSurface and
  get an gfxImageSurface.
  * Composition will be done in SW via cairo in DrawTargetCairo::DrawSurface()


2-UseXRender == true and UseImageOffscreenSurfaces == false

  * ContentClientBasic::CreateBuffer will end up in gfxPlatformGtk::CreateOffscreenSurface and
  get an gfxXlibSurface.
  * Composition will be accelerated with XRender via cairo in DrawTargetCairo::DrawSurface()


3-UseXRender == false and UseImageOffscreenSurfaces == true

  * ContentClientBasic::CreateBuffer will end up in gfxPlatformGtk::CreateOffscreenSurface and
  get an gfxImageSurface.
  * Composition will be done in SW via cairo in DrawTargetCairo::DrawSurface()


4-UseXRender == false and UseImageOffscreenSurfaces == false

  * ContentClientBasic::CreateBuffer will end up in gfxPlatformGtk::CreateOffscreenSurface and
  get an gfxImageSurface.
  * Composition will be done in SW via cairo in DrawTargetCairo::DrawSurface()
 

You see in those four cases, There's no real link between composition type (HW or SW) and useXRender().
In fact, what really dictates type of composition we'll do in RotatedBuffer (that's for MTC path only, remember) is the type of surface that we're creating in the layer on client-side.

> A member of the gfx team should review this.

Now, since MTC path are disappearing, I don't really care about the posted patch, this is something that I posted as-is in another bug (more appropriate) for OMTC basic where it makes more sense. By posting this patch in here, I just wanted to show unavoidable impact of Xlib surface removal on the MTC basic path, incurring unavoidable performance hit (if we don't add buffering capabilities to MTC, which I really doubt we'll do one day).
Comment 70 Karl Tomlinson (:karlt) 2014-05-28 17:29:03 PDT Comment hidden (obsolete)
Comment 71 Karl Tomlinson (:karlt) 2014-05-28 21:13:17 PDT Comment hidden (obsolete)
Comment 72 Karl Tomlinson (:karlt) 2014-05-29 04:22:22 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #71)
> Please let me know if I'm still missing something.

I see the UseXShmSurfaces() test in nsWindow now.
Comment 73 Frederic Plourde (:fred23) 2014-06-03 06:13:25 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #71)
> Currently there is little extra configuration provided by having both
> preferences, but I understand you plan to make them behave differently.

Exactly, it will all make total sense when both OMTB basic + OMTC ogl will be implemented.
We will need those combined UseXRender() + UseImageOffscreenSurfaces prefs working together
to open up those 4 possible path that I previously mentioned on Comment 69.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #72)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #71)
> > Please let me know if I'm still missing something.
> 
> I see the UseXShmSurfaces() test in nsWindow now.

Ys, indeed, it was necessary to check UseXShmSurface() when returning the ThebesSurface in GetThebesSurface()
Comment 74 Frederic Plourde (:fred23) 2014-06-09 06:58:26 PDT
Karl, do you have more thoughts on this ?
If not, I'd close this bug as WONTFIX because
 1) the MTC path is dying
 2) anyways, even if we patch this, the patch is already posted in the OMTC side and will be reviewed and pushed as part of the Bug 1015218.

thanks
Comment 75 Karl Tomlinson (:karlt) 2014-06-09 23:18:11 PDT
(In reply to Frederic Plourde (:fred23) from comment #74)
> If not, I'd close this bug as WONTFIX because
>  1) the MTC path is dying
>  2) anyways, even if we patch this, the patch is already posted in the OMTC
> side and will be reviewed and pushed as part of the Bug 1015218.

Sounds good.  Bug 1015218 seems a better place for attachment 8429555 [details] [diff] [review] because there it can add some additional configuration to UseXRender().

Note You need to log in before you can comment on or make changes to this bug.