947045 - Use Xlib surfaces in the software compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Right now we allocate shmem surfaces and copy them back to xlib. We should just send xlib surfaces.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: consolidate texture creation

I'd like to funnel new texture creation into one function, to avoid potential duplication. With this you would just specify what properties you want your TextureClient to support.

It looks like CanvasClient doesn't actually require a BufferTextureClient so I also changed that.

Comment 2

[Nick Cameron [:nrc]]

Comment on attachment 8344139 [details] [diff] [review]
part 1: consolidate texture creation

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

This is probably what we want long term (so f+ for that), but for now I think it is a bad idea - things are going to get more complex due to using gralloc texture clients - these will only be used by content, not image/canvas (for now). Furthermore, until bug 908196 is fixed we really need to avoid using texture clients for non-content because allocation will fail further down the line. I'm not sure what Windows will look like, but I think we want a distinction there for content vs image/canvas. I think we have a separate method for buffer texture clients also because we really need a texture client that we can read/write not just write to.

So I guess having one method and using a flag like you do here will still work, but unless there is a really good reason to, I'd prefer to avoid it - all the flags in the layers code is getting confusing already and I think having distinct methods is clearer. If it does really clean things up to do this, then I won't object, but you should f? nical too in that case.

Also the 'Surface' path (as opposed to DrawTarget) should be being phased out soon, not sure how that will affect this though.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8344139 [details] [diff] [review]
part 1: consolidate texture creation

Okay, I don't think it actually helped me at all anyway :)

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: wip v0

Comment 5

[Tom S [:evilpie]]

Attachment: wip v1

I updated this patch to trunk. I also added a call FinishX in TextureClientX11::Unlock, which stops it from crashing all the time. This might be bias, but some stuff definitely feels faster.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This patch makes scrolling much smoother for me. In my light testing, it seems to bring graphics performance in e10s up to the same level as basic layers.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: textureclient-x11.patch

Here's a slightly updated version of the patch. With the file descriptor fix that I'm about to attach, I haven't noticed any problems with it.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: backup-x-fd

The reason we were getting crashes at shutdown is that the child process was exiting, then the pixmap was automatically getting released by the X server, and then the parent was trying to free the pixmap.

This patch uses the same technique we use for the plugin container: dup() the client's X server connection into the parent so that the child's resources aren't released when the child exits or crashes.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, and the reason I added the SendBackUpXResources call where I did is that gtk_init happens just a little bit earlier in the same function. I couldn't put it right next to gtk_init because the IPC connection hasn't opened at that time.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a7934ca853
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab19291191fe

Comment 11

[Tom S [:evilpie]]

Oh this is so awesome, good job on figuring this out. For some reason I wasn't cc'ed on this?! There are two followups. In TextureHostX11::Unlock it would be cleaner to use FinishX(msSurface->XDisplay()). The comment over BackUpXResources in PContent.ipdl talks about plugins.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I had to back out because it doesn't build on Windows/Mac. Oops.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fb8eeca62e

Tom, can you post a patch on top of the existing one?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Relanded with a few #ifdef changes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5a4ed50de4

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

And one more push, since bug 950235 landed right before me:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce6b2c04bab

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for IPC test crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a150cbbc959b

https://tbpl.mozilla.org/php/getParsedLog.php?id=34155855&tree=Mozilla-Inbound

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Relanded again. The oranges Ryan mentioned were fixed by the commit in comment 14.
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cddaac17c2

Comment 17

[Daniel Holbert [:dholbert]]

This busted my local warnings-as-errors build (using clang++ on linux), with this warning (treated as an error in this directory):
{
gfx/layers/basic/TextureClientX11.cpp:5:
../../dist/include/mozilla/layers/TextureClientX11.h:59:16: error: private field 'mTextureFlags' is not used [-Werror,-Wunused-private-field]
}

I'm guessing this didn't bust TBPL because this code is linux-only, and the warning is clang-only, and our official linux builders don't use clang.

I filed bug 968635 for the build warning; just posting here as a heads-up.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07cddaac17c2

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

I'm getting a build error with Clang 3.3 with warnings-as-errors:


 0:54.05 In file included from /hack/mozilla-central/gfx/layers/basic/TextureClientX11.cpp:5:
 0:54.05 ../../dist/include/mozilla/layers/TextureClientX11.h:59:16: error: private field 'mTextureFlags' is not used [-Werror,-Wunused-private-field]
 0:54.05   TextureFlags mTextureFlags;
 0:54.05                ^
 0:54.05 1 error generated.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Oh, that is bug 968635 already. Thanks.