David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 11 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 1 • 11 years ago

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.

	Nick Cameron [:nrc]
	Comment 2 • 11 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 11 years ago

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

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

	Tom S [:evilpie]
	Comment 5 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 6 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 7 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 8 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 9 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 10 • 10 years ago

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

	Tom S [:evilpie]
	Comment 11 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 12 • 10 years ago

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?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 13 • 10 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 14 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 16 • 10 years ago

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

	Daniel Holbert [:dholbert]
	Comment 17 • 10 years ago

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.

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 19 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 20 • 10 years ago

Oh, that is bug 968635 already. Thanks.