Add a pref to disable xrender backend

RESOLVED FIXED in mozilla15

Status

()

Core
Hardware Abstraction Layer (HAL)
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nical, Assigned: nical)

Tracking

Trunk
mozilla15
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
As explained in bug 738937 we should be able to disable xrender. This should be done through a preference in about:config.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 722012
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().
(Assignee)

Comment 5

5 years ago
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.
Attachment #613694 - Attachment is obsolete: true
Attachment #613765 - Flags: review?(jmuizelaar)
(Assignee)

Updated

5 years ago
Attachment #613765 - Attachment is patch: true
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.
Attachment #613765 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 7

5 years ago
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.
Attachment #613765 - Attachment is obsolete: true
Attachment #613987 - Flags: review?(jmuizelaar)
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.
Attachment #613987 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 9

5 years ago
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.
Attachment #613987 - Attachment is obsolete: true
Attachment #614071 - Flags: review?(jmuizelaar)
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.
Attachment #614071 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

5 years ago
Attachment #614071 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #614071 - Flags: checkin+ → checkin?
(Assignee)

Comment 12

5 years ago
Created attachment 614569 [details] [diff] [review]
version 4 with capitalized R in XRender
Attachment #614071 - Attachment is obsolete: true
Attachment #614071 - Flags: checkin?
Attachment #614569 - Flags: review?(jmuizelaar)
Attachment #614569 - Flags: checkin?
Attachment #614569 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e20a19a790dd
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 :)
(Assignee)

Comment 19

5 years ago
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
(Assignee)

Comment 22

5 years ago
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.
Attachment #614569 - Attachment is obsolete: true
Attachment #614569 - Flags: checkin?
(Assignee)

Updated

5 years ago
Attachment #616314 - Flags: review?(jmuizelaar)
Whiteboard: [autoland:616314:-p linux,linuxqt,linux64,android,android-xul -u all -t none]
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'
Attachment #616314 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8c388f622f1
Whiteboard: [autoland:616314:-p linux,linuxqt,linux64,android,android-xul -u all -t none]
https://hg.mozilla.org/mozilla-central/rev/f8c388f622f1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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
You need to log in before you can comment on or make changes to this bug.