Last Comment Bug 743830 - Add a pref to disable xrender backend
: Add a pref to disable xrender backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All Linux
: -- minor with 1 vote (vote)
: mozilla15
Assigned To: Nicolas Silva [:nical]
:
Mentors:
Depends on:
Blocks: 722012
  Show dependency treegraph
 
Reported: 2012-04-09 14:44 PDT by Nicolas Silva [:nical]
Modified: 2013-01-13 09:20 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allows disabling use of xrender through a pref. (4.10 KB, patch)
2012-04-10 11:37 PDT, Nicolas Silva [:nical]
no flags Details | Diff | Review
Replaces first attachment, incorporating feedbacks. (5.15 KB, patch)
2012-04-10 14:24 PDT, Nicolas Silva [:nical]
jmuizelaar: review-
Details | Diff | Review
v3. replaces previsous attachment. (6.61 KB, patch)
2012-04-11 07:33 PDT, Nicolas Silva [:nical]
jmuizelaar: review-
Details | Diff | Review
Version 4. UseCliensideRendering is now UseXrender to avoid '!'ing everywhere. Xrender is enabled by default. (5.69 KB, patch)
2012-04-11 11:15 PDT, Nicolas Silva [:nical]
jmuizelaar: review+
Details | Diff | Review
version 4 with capitalized R in XRender (5.69 KB, patch)
2012-04-12 14:29 PDT, Nicolas Silva [:nical]
jmuizelaar: review+
Details | Diff | Review
Fixed platform problems and a stupid bug (forgot to negate a call to UseXRender somewhere I should have) (10.35 KB, patch)
2012-04-18 15:25 PDT, Nicolas Silva [:nical]
jmuizelaar: review+
Details | Diff | Review

Description Nicolas Silva [:nical] 2012-04-09 14:44:47 PDT
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] 2012-04-10 11:37:07 PDT
Created attachment 613694 [details] [diff] [review]
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] 2012-04-10 12:48:08 PDT
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] 2012-04-10 12:54:59 PDT
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) (:gwright) 2012-04-10 13:06:15 PDT
(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] 2012-04-10 14:24:34 PDT
Created attachment 613765 [details] [diff] [review]
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] 2012-04-10 15:34:57 PDT
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] 2012-04-11 07:33:46 PDT
Created attachment 613987 [details] [diff] [review]
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] 2012-04-11 08:04:30 PDT
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] 2012-04-11 11:15:58 PDT
Created attachment 614071 [details] [diff] [review]
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) (:gwright) 2012-04-11 11:20:53 PDT
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] 2012-04-12 14:00:30 PDT
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] 2012-04-12 14:29:50 PDT
Created attachment 614569 [details] [diff] [review]
version 4 with capitalized R in XRender
Comment 13 George Wright (:gw280) (:gwright) 2012-04-13 11:38:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e20a19a790dd
Comment 14 Marco Bonardo [::mak] 2012-04-13 12:44:46 PDT
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] 2012-04-13 12:45:13 PDT
ehr, the backout changeset is https://hg.mozilla.org/integration/mozilla-inbound/rev/19780b0d3567
Comment 16 Marco Bonardo [::mak] 2012-04-13 12:51:56 PDT
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) (:gwright) 2012-04-13 12:53:59 PDT
(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) (:gwright) 2012-04-13 12:57:06 PDT
(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] 2012-04-13 13:04:32 PDT
Sorry. Ill fix that and wait until I have access to try servers before submitting / breaking something else.
Comment 20 Matt Brubeck (:mbrubeck) 2012-04-13 13:04:51 PDT
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 22 Nicolas Silva [:nical] 2012-04-18 15:25:42 PDT
Created attachment 616314 [details] [diff] [review]
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] 2012-04-20 20:00:22 PDT
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 25 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 21:18:00 PDT
https://hg.mozilla.org/mozilla-central/rev/f8c388f622f1
Comment 26 Zack Weinberg (:zwol) 2012-05-08 14:42:19 PDT
Mostly out of curiosity, when xrender is disabled, what drawing API gets used instead?
Comment 27 George Wright (:gw280) (:gwright) 2012-05-08 15:01:14 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.