743830 - Add a pref to disable xrender backend

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Nicolas Silva [:nical]]

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

Comment 1

[Nicolas Silva [:nical]]

Attachment: Allows disabling use of xrender through a pref.

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 2

[Jeff Muizelaar [:jrmuizel]]

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 3

[Jeff Muizelaar [:jrmuizel]]

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)

Comment 4

[George Wright (:gw280) (needinfo me!)]

(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().

Comment 5

[Nicolas Silva [:nical]]

Attachment: Replaces first attachment, incorporating feedbacks.

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 6

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 7

[Nicolas Silva [:nical]]

Attachment: v3. replaces previsous attachment.

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 8

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 9

[Nicolas Silva [:nical]]

Attachment: Version 4. UseCliensideRendering is now UseXrender to avoid '!'ing everywhere. Xrender is enabled by default.

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 10

[George Wright (:gw280) (needinfo me!)]

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 11

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 12

[Nicolas Silva [:nical]]

Attachment: version 4 with capitalized R in XRender

Comment 13

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e20a19a790dd

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 (==)

Comment 17

[George Wright (:gw280) (needinfo me!)]

(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?

Comment 18

[George Wright (:gw280) (needinfo me!)]

(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 :)

Comment 19

[Nicolas Silva [:nical]]

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

Comment 20

[Matt Brubeck (:mbrubeck)]

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

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 22

[Nicolas Silva [:nical]]

Attachment: Fixed platform problems and a stupid bug (forgot to negate a call to UseXRender somewhere I should have)

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 23

[Jeff Muizelaar [:jrmuizel]]

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'

Comment 24

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c388f622f1

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f8c388f622f1

Comment 26

[Zack Weinberg (:zwol)]

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

Comment 27

[George Wright (:gw280) (needinfo me!)]

(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