Closed Bug 1425260 Opened 2 years ago Closed 2 years ago

Bring back webrendest functionality

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(1 file)

Either re-introduce the gfx.webrendest.enabled preference, or have gfx.webrender.enabled set all the necessary values for the recommended configuration for dogfooders that don't have the detailed information about keeping up.

See bug 1410824 where we removed gfx.webrendest.enabled as we didn't need it at the time.

Make it easy for a casual user/QA to switch to full WebRender functionality, but leave it possible for the engineers to tweak different values to debug or measure performance.
Priority: -- → P2
Priority: P2 → P3
Priority: P3 → P1
Assignee: nobody → milan
Comment on attachment 8939535 [details]
Bug 1425260: gfx.webrender.all turns on all preferences that are needed for webrender.

https://reviewboard.mozilla.org/r/209858/#review215584

::: gfx/thebes/gfxPrefs.h:504
(Diff revision 1)
>  
>    DECL_GFX_PREF(Live, "gfx.vsync.collect-scroll-transforms",   CollectScrollTransforms, bool, false);
>    DECL_GFX_PREF(Once, "gfx.vsync.compositor.unobserve-count",  CompositorUnobserveCount, int32_t, 10);
>  
> +  DECL_GFX_PREF(Once, "gfx.webrender.all",                     WebRenderAll, bool, false);
> +  DECL_GFX_PREF(Once, "gfx.webrender.enabled",                 WebRenderEnabled, bool, false);

We intentionally left gfx.webrender.enabled out of gfxPrefs.h so that people wouldn't accidentally use it instead of gfxVars::UseWebRender. If we add it here maybe it would be better to tack on a DoNotUseDirectly into the function name.

::: image/imgFrame.cpp:104
(Diff revision 1)
>      return Factory::CreateDataSourceSurfaceWithStride(size, format,
>                                                        stride, false);
>    }
>  #endif
>  
> -  if (!aIsAnimated && gfxPrefs::ImageMemShared()) {
> +  if (!aIsAnimated && (gfxPrefs::WebRenderAll() || gfxPrefs::ImageMemShared())) {

I would prefer making ImageMemShared an override pref (https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/gfx/thebes/gfxPrefs.h#79-90) rather than having to ensure every call site, present and future, has this WebRenderAll() check.

Ditto for the WebRenderBlobImages pref.

::: modules/libpref/init/all.js:869
(Diff revision 1)
>  pref("gfx.logging.peak-texture-usage.enabled", false);
>  
>  pref("gfx.ycbcr.accurate-conversion", false);
>  
>  #ifdef MOZ_ENABLE_WEBRENDER
> +pref("gfx.webrender.all", true);

This is going to turn on ImageMemShared and acceleration for anybody who builds with --enable-webrender in their mozconfig, not sure if that's desirable or not.
I started with the override prefs, but went the manual, but simple way of just checking on all call sites.  I expect the other preferences to (eventually) disappear.  I'll take a look again.
Comment on attachment 8939535 [details]
Bug 1425260: gfx.webrender.all turns on all preferences that are needed for webrender.

https://reviewboard.mozilla.org/r/209858/#review216618

Dropping review flag until issues are sorted out
Attachment #8939535 - Flags: review?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> ...              WebRenderEnabled, bool, false);
> 
> We intentionally left gfx.webrender.enabled out of gfxPrefs.h so that people
> wouldn't accidentally use it instead of gfxVars::UseWebRender. If we add it
> here maybe it would be better to tack on a DoNotUseDirectly into the
> function name.

Went with the "DoNotUseDirectly" to avoid this being a live preference, even if only evaluated on start.

> 
> I would prefer making ImageMemShared an override pref
> (https://searchfox.org/mozilla-central/rev/
> 
> Ditto for the WebRenderBlobImages pref.

Done.

> >  
> >  #ifdef MOZ_ENABLE_WEBRENDER
> > +pref("gfx.webrender.all", true);
> 
> This is going to turn on ImageMemShared and acceleration for anybody who
> builds with --enable-webrender in their mozconfig, not sure if that's
> desirable or not.

Pulled that out.
Comment on attachment 8939535 [details]
Bug 1425260: gfx.webrender.all turns on all preferences that are needed for webrender.

https://reviewboard.mozilla.org/r/209858/#review217630

LGTM, thanks!
Attachment #8939535 - Flags: review?(bugmail) → review+
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89d7853edd64
gfx.webrender.all turns on all preferences that are needed for webrender. r=kats
https://hg.mozilla.org/mozilla-central/rev/89d7853edd64
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.