Closed Bug 947045 Opened 6 years ago Closed 6 years ago

Use Xlib surfaces in the software compositor

Categories

(Core :: Graphics: Layers, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files, 3 obsolete files)

Right now we allocate shmem surfaces and copy them back to xlib. We should just send xlib surfaces.
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.
Attachment #8344139 - Flags: feedback?(ncameron)
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.
Attachment #8344139 - Flags: feedback?(ncameron)
Comment on attachment 8344139 [details] [diff] [review]
part 1: consolidate texture creation

Okay, I don't think it actually helped me at all anyway :)
Attachment #8344139 - Attachment is obsolete: true
Attached patch wip v1 (obsolete) — Splinter Review
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.
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.
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.
Attachment #8344917 - Attachment is obsolete: true
Attachment #8369522 - Attachment is obsolete: true
Attachment #8370505 - Flags: review?(matt.woodrow)
Attached patch backup-x-fdSplinter Review
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.
Attachment #8370507 - Flags: review?(karlt)
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.
Attachment #8370507 - Flags: review?(karlt) → review+
Attachment #8370505 - Flags: review?(matt.woodrow) → review+
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.
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?
Depends on: 968635
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.
https://hg.mozilla.org/mozilla-central/rev/07cddaac17c2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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.
Oh, that is bug 968635 already. Thanks.
Duplicate of this bug: 779356
Blocks: 976015
You need to log in before you can comment on or make changes to this bug.