As explained in bug 738937 we should be able to disable xrender. This should be done through a preference in about:config.

My first patch. I tried to modify as few lines of code as possible. I am not absolutely certain that mozilla::gl::GLXLibrary is the best place to store the pref for every one to read it, but it already had mHasTextureFromPixmap serving that purpose (although HasTextureFromPixmap was not used outside of GLXContextProvider.cpp before as it seems). It's important to set GLXLibrary::mHasTextureFromPixmap to false when gfxPlatformGtk::UseClientSideRendering returns true, else firefox renders a black window if the pref layers.acceleration.force-enabled is on. I need some feedbacks.

Comment on attachment 613694 [details] [diff] [review] Allows disabling use of xrender through a pref. Review of attachment 613694 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderGLX.cpp @@ +252,4 @@ > GLLibraryLoader::LoadSymbols(mOGLLibrary, symbols_texturefrompixmap, > (GLLibraryLoader::PlatformLookupFunction)&xGetProcAddress)) > { > + mHasTextureFromPixmap = !mozilla::Preferences::GetBool("gfx.xrender.disabled"); I would rather this preference access happen in gfxPlatformGtk.cpp and have this function call UseClientSideRendering() it might also be worth changing the name of mHasTextureFromPixmap to mUseTextureFromPixmap.

Comment on attachment 613694 [details] [diff] [review] Allows disabling use of xrender through a pref. >+ mHasTextureFromPixmap = !mozilla::Preferences::GetBool("gfx.xrender.disabled"); I'd also think I'd rather have this pref called "gfx.xrender.enabled" Since, we want to eventually default to false, I think that makes more sense (though I'm not certain)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > >+ mHasTextureFromPixmap = !mozilla::Preferences::GetBool("gfx.xrender.disabled"); > > I'd also think I'd rather have this pref called "gfx.xrender.enabled" > Since, we want to eventually default to false, I think that makes more sense > (though I'm not certain) I don't think we should move the logic for "do we want to toggle xrender" to the mHasTextureFromPixmap bool. I think mHasTextureFromPixmap should always be whether that extension exists, and the logic for toggling xrender should be in UseClientSideRendering().

Thanks for the feedbacks, the new version incorporates your suggestions: - the pref is now gfx.xrender.enabled (false by default) - mHasTextureFromPixmap is not touched by the pref though we must be careful to check for gfxPlatformGtk::UseClientSideRendering() when using it. - the state is stored in gfxPlatformGtk::sUseXrender That makes much more sense indeed.

Comment on attachment 613765 [details] [diff] [review] Replaces first attachment, incorporating feedbacks. Review of attachment 613765 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderGLX.cpp @@ +329,4 @@ > void > GLXLibrary::DestroyPixmap(GLXPixmap aPixmap) > { > + if (!mHasTextureFromPixmap || gfxPlatformGtk::UseClientSideRendering()) { I would rename mHasTextureFromPixmap to mUseTextureFromPixmap and just set it using UseClientSideRendering() that way we don't need to add the calls in multiple places.

version 3. Same as v2, but instead of using both mHasTextureFromPixmap and UseClientSideRendering, mHasTextureFromPixmap has been renamed to mUseTextureFromPixmap and initialized to the value returned by UseClientSideRendering.

Comment on attachment 613987 [details] [diff] [review] v3. replaces previsous attachment. Review of attachment 613987 [details] [diff] [review]: ----------------------------------------------------------------- This looks good except for a couple of cosmetic things. ::: gfx/gl/GLContextProviderGLX.cpp @@ +251,4 @@ > GLLibraryLoader::LoadSymbols(mOGLLibrary, symbols_texturefrompixmap, > (GLLibraryLoader::PlatformLookupFunction)&xGetProcAddress)) > { > + mUseTextureFromPixmap = gfxPlatformGtk::UseClientSideRendering(); I believe this should be: mUseTextureFromPixmap = !gfxPlatformGtk::UseClientSideRendering(); ::: gfx/thebes/gfxPlatformGtk.cpp @@ +179,5 @@ > + // rendering, but until we have the ability to featuer test > + // this, we'll only disable this for maemo. > + return true; > +#elif defined(MOZ_X11) > + return sUseXrender; This should be !sUseXrender. I realize that there's some back and forth between the names and uses of these variables, which requires some '!'ing. If there's a way to make it easier to follow go with that, otherwise don't worry about it. @@ +183,5 @@ > + return sUseXrender; > +#else > + return false; > +#endif > +} Keeping this function definition in the header would make it easier to review the difference. ::: modules/libpref/src/init/all.js @@ +3430,5 @@ > > pref("layers.offmainthreadcomposition.enabled", false); > > +#ifdef MOZ_X11 > +pref("gfx.xrender.enabled",false); This should be true for now.

Oh, so sorry about the double negation. You are right about the variable names, I renamed UseClientsideRendering to UseXrender which makes it more coherent (it is not used anywhere else after all). UseXrender is also back in the header and gfx.xrender.enabled is true by default.

Comment on attachment 614071 [details] [diff] [review] Version 4. UseCliensideRendering is now UseXrender to avoid '!'ing everywhere. Xrender is enabled by default. Review of attachment 614071 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLContextProviderGLX.cpp @@ +843,4 @@ > > bool TextureImageSupportsGetBackingSurface() > { > + return sGLXLibrary.UseTextureFromPixmap(); Do we need a separate UseTextureFromPixmap() function when we already have SupportsTextureFromPixmap() which takes into account mHasTextureFromPixmap?

Comment on attachment 614071 [details] [diff] [review] Version 4. UseCliensideRendering is now UseXrender to avoid '!'ing everywhere. Xrender is enabled by default. Capitalize the R in UseXRender, and I agree with George's question.

backed out cause this broke Linux QT https://hg.mozilla.org/integration/mozilla-inbound/rev/e20a19a790dd /builds/slave/m-in-linuxqt/build/gfx/gl/GLContextProviderGLX.cpp:60:28: fatal error: gfxPlatformGtk.h: No such file or directory make[6]: *** [GLContextProviderGLX.o] Error 1 looks in need of some ifdefing

ehr, the backout changeset is https://hg.mozilla.org/integration/mozilla-inbound/rev/19780b0d3567

may have also caused these failures on Linux REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/border-radius/curved-border-background-nogap.html | image comparison (==) REFTEST TEST-UNEXPECTED-PASS | file:///home/cltbld/talos-slave/test/build/reftest/tests/dom/plugins/test/reftest/plugin-transform-2.html | image comparison (==)

(In reply to Marco Bonardo [:mak] from comment #16) > may have also caused these failures on Linux > REFTEST TEST-UNEXPECTED-FAIL | > file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ > border-radius/curved-border-background-nogap.html | image comparison (==) > REFTEST TEST-UNEXPECTED-PASS | > file:///home/cltbld/talos-slave/test/build/reftest/tests/dom/plugins/test/ > reftest/plugin-transform-2.html | image comparison (==) It definitely does. I think they're fine though; unexpected pass is correct because that's fails-if(x11) and we're not really x11 anymore with this patch, and the other one is a bit annoying, but not too bad. We can probably mark -nogap as an expected failure. Jeff - what do you think?

(In reply to George Wright (:gw280) from comment #17) > (In reply to Marco Bonardo [:mak] from comment #16) > > may have also caused these failures on Linux > > REFTEST TEST-UNEXPECTED-FAIL | > > file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ > > border-radius/curved-border-background-nogap.html | image comparison (==) > > REFTEST TEST-UNEXPECTED-PASS | > > file:///home/cltbld/talos-slave/test/build/reftest/tests/dom/plugins/test/ > > reftest/plugin-transform-2.html | image comparison (==) > > It definitely does. I think they're fine though; unexpected pass is correct > because that's fails-if(x11) and we're not really x11 anymore with this > patch, and the other one is a bit annoying, but not too bad. We can probably > mark -nogap as an expected failure. Jeff - what do you think? Oh this patch wasn't supposed to turn off xrender by default. Yeah, these test results are not good then :)

Sorry. Ill fix that and wait until I have access to try servers before submitting / breaking something else.

We're also seeing a pixman crash in test_image_layers.html, and test_movement_by_characters.html that might be caused by this patch: https://tbpl.mozilla.org/php/getParsedLog.php?id=10884466&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=10883675&tree=Mozilla-Inbound

https://hg.mozilla.org/mozilla-central/rev/e20a19a790dd https://hg.mozilla.org/mozilla-central/rev/19780b0d3567

This version should be good. I did not have access to try servers before so I hoped it would work. I should have asked someone to push to try for me though, sorry.

Comment on attachment 616314 [details] [diff] [review] Fixed platform problems and a stupid bug (forgot to negate a call to UseXRender somewhere I should have) Review of attachment 616314 [details] [diff] [review]: ----------------------------------------------------------------- You have a typo in your commit message: 'gfx.xrender.enableded' should 'gfx.xrender.enabled'

Mostly out of curiosity, when xrender is disabled, what drawing API gets used instead?

(In reply to Zack Weinberg (:zwol) from comment #26) > Mostly out of curiosity, when xrender is disabled, what drawing API gets > used instead? Cairo image surfaces i.e. pixman