Allow subpixel AA to be controlled using blocklists

NEW
Unassigned

Status

()

enhancement
P3
normal
2 years ago
11 months ago

People

(Reporter: milan, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

See bug 1357571 comment 15 for the background.  We want the ability to disable subpixel AA (it should already be behind a preference, if it isn't we should add it), then make sure that we recognize it and are able to block it using blocklists.
Assignee: nobody → a.beingessner
Blocks: 1357571
The preference is layers.componentalpha.enabled.

This should be similar to what we do for the GPU process (layers.gpu-process.enabled and GPUProcessEnabled on the prefs side and nsIGfxInfo::FEATURE_GPU_PROCESS on the blocklisting side.)
Comment on attachment 8873191 [details]
Bug 1365772 - Allow component alpha to be managed by blocklists.

https://reviewboard.mozilla.org/r/144650/#review148530

::: gfx/thebes/gfxPlatform.cpp:2420
(Diff revision 1)
> +  if (!IsGfxInfoStatusOkay(nsIGfxInfo::FEATURE_COMPONENT_ALPHA, &message, failureId)) {
> +    componentAlpha.Disable(FeatureStatus::Blacklisted, message.get(), failureId);
> +  }
> +
> +  // Safe Mode should clobber everything
> +  if (InSafeMode()) {

Probably don't want to disable this explicitly in safe mode.  It may get disabled anyway because of other things going on, but I'd say we leave the safe mode out of this part.

::: widget/nsIGfxInfo.idl:127
(Diff revision 1)
>    const long FEATURE_DX_INTEROP2 = 19;
>    /* Whether the GPU process is supported, starting in 52. */
>    const long FEATURE_GPU_PROCESS = 20;
>    /* Whether the WebGL2 is supported, starting in 54 */
>    const long FEATURE_WEBGL2 = 21;
> +  /* Whether per-color-component alpha (for sub-pixel-AA) is supported, starting in 55. */

We don't have a lot of time to test this, so let's sit on it until 56 (about 10 days), and only land then.
Comment on attachment 8873192 [details]
Bug 1365772 - Refactor gfx blocklist tests to fail on unsupported platforms.

https://reviewboard.mozilla.org/r/144652/#review148632

::: commit-message-0c6a6:1
(Diff revision 2)
> +Refactor gfx blocklist tests to fail on unsupported platforms. r?kats
> +
> +MozReview-Commit-ID: CPYdL242bGc

Please expand this commit message to also say *why* you are making this change. Also this is missing a bug number (I'm surprised MozReview even accepted the patch without a bug number)

::: toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Equal_DriverNew.js:35
(Diff revision 2)
>    blocklist.notify(null);
>  }
>  
>  // Performs the initial setup
>  function run_test() {
> +  do_check_eq(false, true);

This looks like it was left in accidentally
Attachment #8873192 - Flags: review?(bugmail) → review+
Comment on attachment 8873191 [details]
Bug 1365772 - Allow component alpha to be managed by blocklists.

https://reviewboard.mozilla.org/r/144650/#review148636

A minor thing but I'd like to see the new versions of the patches, so r- for now.

::: gfx/thebes/gfxPlatform.cpp:2407
(Diff revision 2)
> +  componentAlpha.SetDefaultFromPref(
> +    gfxPrefs::GetComponentAlphaEnabledPrefName(),
> +    true,
> +    gfxPrefs::GetComponentAlphaEnabledPrefDefault());

I would actually prefer to see the componentalpha pref removed from gfxPrefs so that people don't accidentally add more uses of it, since they are now supposed to use the Feature status instead. That's what we did for webrender as well.

::: gfx/thebes/gfxPlatform.cpp:2420
(Diff revision 2)
> +  if (InSafeMode()) {
> +    componentAlpha.ForceDisable(
> +      FeatureStatus::Blocked,
> +      "Safe-mode is enabled",

Agree with Milan that we don't really need to block this in safe mode. You can just take out this hunk.
Attachment #8873191 - Flags: review?(bugmail) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Comment on attachment 8873191 [details]
> Bug 1365772 - Allow component alpha to be managed by blocklists.
> 
> https://reviewboard.mozilla.org/r/144650/#review148636
> 
> A minor thing but I'd like to see the new versions of the patches, so r- for
> now.
> 
> ::: gfx/thebes/gfxPlatform.cpp:2407
> (Diff revision 2)
> > +  componentAlpha.SetDefaultFromPref(
> > +    gfxPrefs::GetComponentAlphaEnabledPrefName(),
> > +    true,
> > +    gfxPrefs::GetComponentAlphaEnabledPrefDefault());
> 
> I would actually prefer to see the componentalpha pref removed from gfxPrefs
> so that people don't accidentally add more uses of it, since they are now
> supposed to use the Feature status instead. That's what we did for webrender
> as well.

We've been dealing with this by tacking the "DoNotUseDirectly" to the function name.  Probably a good approach to take here, to avoid some of the complication around different platforms (this is ignored on mobile.)
(In reply to Milan Sreckovic [:milan] from comment #9)
> We've been dealing with this by tacking the "DoNotUseDirectly" to the
> function name.

Yup, that works too.
(In reply to Milan Sreckovic [:milan] from comment #9)
> (this is ignored on mobile.)

Oh, I just realized what you mean by this (the MOZ_GFX_OPTIMIZE_MOBILE define). But that also means that we shouldn't use SetDefaultFromPref to initialize the gfxFeature, because that unconditionally reads the pref, and will respect it even on mobile. That would be a difference in behaviour versus before. i.e. without these patches, setting layers.componentalpha.enabled on mobile has no effect, but with these patches it will turn on component alpha.

We will need to add another MOZ_GFX_OPTIMIZE_MOBILE block in InitComponentAlphaPrefs() that forces it back to disabled.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> We will need to add another MOZ_GFX_OPTIMIZE_MOBILE block in
> InitComponentAlphaPrefs() that forces it back to disabled.

This should really be "we will need to either add MOZ_GFX_OPTIMIZE_MOBILE block ... or not use SetDefaultFromPref and just put that code inline, with a special handling for the MOZ_GFX_OPTIMIZE_MOBILE case". Or find some other way to ensure the feature remains disabled on mobile.
Also, do we want a force-enable pref to override the blocklist? IIRC we have those in other places.
ni? milan for comment 15. I think we'll need to tweak the code for comment 13/14 regardless.
Flags: needinfo?(milan)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> ... i.e. without these patches, setting
> layers.componentalpha.enabled on mobile has no effect, but with these
> patches it will turn on component alpha.

ComponentAlphaEnabledDoNotUseDirectly(), and in general asking for a preference values of layers.componentalpha.enabled when MOZ_GFX_OPTIMIZE_MOBILE will always give back false, regardless of what the value set in prefs.js/all.js is.  That preference is of type "Skip" in that block, which means that it keeps the default, and doesn't respond to anything else.

Is that what you meant for comment 13/14?

For comment 15 - good point.  Being consistent is good, and while we have "override all blocklist", it isn't on release.  We should probably do it, but then may want to revisit the whole "override the blocklist in release" in another bug.
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #17)
> ComponentAlphaEnabledDoNotUseDirectly(), and in general asking for a
> preference values of layers.componentalpha.enabled when
> MOZ_GFX_OPTIMIZE_MOBILE will always give back false, regardless of what the
> value set in prefs.js/all.js is.  That preference is of type "Skip" in that
> block, which means that it keeps the default, and doesn't respond to
> anything else.
> 
> Is that what you meant for comment 13/14?

Not quite. I agree with what you're saying, but that's assuming the code actually calls gfxPrefs::ComponentAlphaEnabledDoNotUseDirectly(). However, the patch doesn't do that. What it does is call gfxPrefs::ComponentAlphaEnabledDoNotUseDirectlyPrefName(), and passes that to FeatureState::SetDefaultFromPref, which [1] then bypasses gfxPrefs and uses Preferences::GetBool to look up the value. That will bypass the "skip" notation in gfxPrefs and the MOZ_GFX_OPTIMIZE_MOBILE and will read the actual value from all.js or whatever prefs file.
Comment on attachment 8873191 [details]
Bug 1365772 - Allow component alpha to be managed by blocklists.

https://reviewboard.mozilla.org/r/144650/#review150750

Minusing, this needs updating for the issue in comment 18.
Attachment #8873191 - Flags: review?(bugmail) → review-
Comment on attachment 8873191 [details]
Bug 1365772 - Allow component alpha to be managed by blocklists.

https://reviewboard.mozilla.org/r/144650/#review152528

r=me with the fix below.

::: gfx/thebes/gfxPlatform.cpp:2410
(Diff revision 4)
> +
> +  // First check the about:config value
> +#ifdef MOZ_GFX_OPTIMIZE_MOBILE
> +  // If MOZ_GFX_OPTIMIZE_MOBILE is defined, we force component alpha off
> +  // and ignore the preference.
> +  componentAlpha.SetDefault(false, true, false);

The FeatureState::SetDefault function signature takes a FeatureStatus and const char* so this probably won't compile on android.

I think you can replace this with:
componentAlpha.DisableByDefault(FeatureStatus::Unavailable, "Component alpha not available on mobile", NS_LITERAL_CSTRING("FEATURE_FAILURE_MOBILE"));

or something along those lines.
Attachment #8873191 - Flags: review?(bugmail) → review+
The updated patches look good (sorry for the delay, this slipped my mind since there were no flags pending for me). I've kicked it to autoland.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 586d156e3e4c -d 016bcae14073: rebasing 402574:586d156e3e4c "Bug 1365772 - Allow component alpha to be managed by blocklists. r=kats"
merging gfx/config/gfxFeature.h
merging gfx/layers/Layers.cpp
merging gfx/thebes/gfxPlatform.cpp
merging gfx/thebes/gfxPlatform.h
merging gfx/thebes/gfxPrefs.h
warning: conflicts while merging gfx/config/gfxFeature.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/thebes/gfxPlatform.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/thebes/gfxPlatform.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased; just a new config that was added in parallel and led to adjacent line conflicts.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/112d854f652d
Allow component alpha to be managed by blocklists. r=kats
https://hg.mozilla.org/integration/autoland/rev/f978d13e38c7
Refactor gfx blocklist tests to fail on unsupported platforms. r=kats
Backed out for Windows bustage: 'gfxConfig': is not a class or namespace name at DeviceAttachmentsD3D11.cpp(179):

https://hg.mozilla.org/integration/autoland/rev/f86b5ef8947b1772d8bf5936f3a982f07f77883c
https://hg.mozilla.org/integration/autoland/rev/e67db7564a9256356027901ca0a3e9c0cc3e99bb

Also check this Valgrind log: https://treeherder.mozilla.org/logviewer.html#?job_id=108230475&repo=autoland

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f978d13e38c715cbbb49702ef2577f9e724b6d8a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=108230727&repo=autoland

 c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(179): error C2653: 'gfxConfig': is not a class or namespace name
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(179): error C2653: 'Feature': is not a class or namespace name
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(179): error C2065: 'COMPONENT_ALPHA': undeclared identifier
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(179): error C3861: 'IsEnabled': identifier not found
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(297): error C2653: 'gfxConfig': is not a class or namespace name
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(297): error C2653: 'Feature': is not a class or namespace name
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(297): error C2065: 'COMPONENT_ALPHA': undeclared identifier
c:/builds/moz2_slave/autoland-w64-00000000000000000/build/src/gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp(297): error C3861: 'IsEnabled': identifier not found

Please fix the issues and submit updated patches. Thank you.
Flags: needinfo?(a.beingessner)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66b6c25cacc7
Allow component alpha to be managed by blocklists. r=kats
https://hg.mozilla.org/integration/autoland/rev/3e869b1c08d0
Refactor gfx blocklist tests to fail on unsupported platforms. r=kats
Julian, the log in comment 37 has valgrind complaining about "unhandled syscall: 318". Is this a known issue?
Flags: needinfo?(jseward)
Also, there's a bunch of windows test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108318266&repo=autoland that I want to pin on these patches.
The xpcshell tests are confusing. They fail but they're supposed to -- my patch adds 

fail-if = !debug || os == "mac" || os == "linux" 

annotations to all these tests. The annotations seem to do the right thing locally (macos 10.12, optimized), but try doesn't seem to be understanding them on this platform?. Is there something wrong with my annotations, or does this mac builder do something strange?
Flags: needinfo?(a.beingessner)
Ah I pinged dvanders on this and he explained some key details. Will have a new patch up soon.
Dropping stale needinfo.
Flags: needinfo?(jseward)
Priority: -- → P3
Assignee: a.beingessner → nobody
You need to log in before you can comment on or make changes to this bug.