Closed Bug 1548795 Opened 9 months ago Closed 9 months ago

StaticPrefs shouldn't be used in the GPU process

Categories

(Core :: Audio/Video: Playback, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox67 + fixed
firefox68 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 7 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

Following bug 1521370, in order to prevent crashes on some version of windows, we moved the detection to determine if a feature is supported or not to the WMF PDM initialisation.
https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/dom/media/platforms/wmf/WMFDecoderModule.cpp#93

This reads the StaticPrefs::MediaWmfVp9Enabled to check if HW decoding is enabled or not.

While backporting the fixes to ESR60, I hit some crashes. ESR60 didn't have StaticPrefs and instead used MediaPrefs. MediaPrefs had strong assertion that it wasn't used in the GPU process.

It appears that StaticPrefs is not functional when used in the GPU process.

If that's the case, the fix in bug 1521370 is bad and could disable HW decoding unnecessarily.

So we need to determine if StaticPrefs is indeed usable in the GPU process, and if it isn't put some code to prevent from being used (e.g. crash) and stop using it.

Priority: -- → P2

[Tracking Requested - why for this release]: regression potentially introduced in beta 67, and uplifted to release.

No longer blocks: 1521370
Assignee: nobody → jyavenard
Priority: P2 → P1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7136399c84c4fe59c368a635cd27a2e23c6a1ec0

We have a few of such use of StaticPrefs in the RDD process which aren't usable.

https://searchfox.org/mozilla-central/rev/b2015fdd464f598d645342614593d4ebda922d95/dom/media/ipc/RemoteVideoDecoder.cpp#155

I recall some discussions discussing on how the pref didn't work or something. Did that get fixed?

Actually, looking at the code, there's no handling of preferences in the RDD process, so calling StaticPrefs or gfxPrefs from the RDD will read rubbish.

Flags: needinfo?(mfroman)
Flags: needinfo?(achronop)

(In reply to Jean-Yves Avenard [:jya] from comment #3)

I recall some discussions discussing on how the pref didn't work or something. Did that get fixed?

Yes it's fixed, the issue there was that we did not check the pref at all. We had not included the logic to check the pref and trigger the appropriate decoder.

Actually, looking at the code, there's no handling of preferences in the RDD process, so calling StaticPrefs or gfxPrefs from the RDD will read rubbish.

Michael must have fixed the static pref handling in Bug 1539029.

Flags: needinfo?(achronop)

(In reply to Jean-Yves Avenard [:jya] from comment #3)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7136399c84c4fe59c368a635cd27a2e23c6a1ec0

We have a few of such use of StaticPrefs in the RDD process which aren't usable.

https://searchfox.org/mozilla-central/rev/b2015fdd464f598d645342614593d4ebda922d95/dom/media/ipc/RemoteVideoDecoder.cpp#155

I recall some discussions discussing on how the pref didn't work or something. Did that get fixed?

Actually, looking at the code, there's no handling of preferences in the RDD process, so calling StaticPrefs or gfxPrefs from the RDD will read rubbish.

Alex is correct about Bug 1539029. Pref handling must be setup for each process and that isn't completely obvious until an issue comes up. In our case, Alex and I uncovered it w/ the RDD process, and I fixed pref handling for RDD in Bug 1539029.

It should be fairly easy to add pref handling to the GPU process using Bug 1539029 as an example.

Flags: needinfo?(mfroman)

Jya, could you prepare a patch for beta uplift please? Thanks

Flags: needinfo?(jyavenard)

(In reply to Michael Froman [:mjf] from comment #5)

Alex is correct about Bug 1539029. Pref handling must be setup for each process and that isn't completely obvious until an issue comes up. In our case, Alex and I uncovered it w/ the RDD process, and I fixed pref handling for RDD in Bug 1539029.

It should be fairly easy to add pref handling to the GPU process using Bug 1539029 as an example.

I don't think this is what we want to do. It requires to load xpcom and use javascript services.
The RDD doesn't need it, and using a mechanism similar to gfxPrefs would have been preferred.

Flags: needinfo?(jyavenard)

Bug 1437438, added a call to https://searchfox.org/mozilla-central/rev/99a2a5a955960b0e58ceade1db1f7652d9db4ba1/xpcom/threads/nsThread.cpp#1123
mozilla::StaticPrefs::dom_performance_enable_scheduler_timing();

from testing, that always returns 1.

Do we want that typic of logging in the GPU process by default all the time?

Flags: needinfo?(tarek)
Regressed by: 1437438

(In reply to Jean-Yves Avenard [:jya] from comment #7)

(In reply to Michael Froman [:mjf] from comment #5)

Alex is correct about Bug 1539029. Pref handling must be setup for each process and that isn't completely obvious until an issue comes up. In our case, Alex and I uncovered it w/ the RDD process, and I fixed pref handling for RDD in Bug 1539029.

It should be fairly easy to add pref handling to the GPU process using Bug 1539029 as an example.

I don't think this is what we want to do. It requires to load xpcom and use javascript services.
The RDD doesn't need it, and using a mechanism similar to gfxPrefs would have been preferred.

actually, maybe we should just do that rather than ensuring that StaticPrefs isn't called from the GPU process.

This is a temporary fix, aimed to be easily uplifted to beta 67.
In a future change we will make Preferences and StaticPrefs work in the GPU process.

P2 will fix the issue.

Flags: needinfo?(tarek)

This is a temporary fix, aimed to be easily uplifted to beta 67.
In a future change we will make Preferences and StaticPrefs work in the GPU process.

:tarek : does this pref needs to be live?

Flags: needinfo?(tarek)
Attachment #9063434 - Attachment is obsolete: true

Not sure what you mean by "live". This pref is static because the scheduler is called before regular prefs are usable. about:performance and some other consumers relies on that pref to be activated in order to have counters incremented.

one option is to remove the pref and have the counters always on, since it's been the default for a while and it works.
I defer this decision to Nathan.

Flags: needinfo?(tarek) → needinfo?(nfroyd)

(In reply to Tarek Ziadé (:tarek) from comment #14)

Not sure what you mean by "live". This pref is static because the scheduler is called before regular prefs are usable. about:performance and some other consumers relies on that pref to be activated in order to have counters incremented.

one option is to remove the pref and have the counters always on, since it's been the default for a while and it works.
I defer this decision to Nathan.

It's live in that if you modify the pref, existing nsThread will immediately modify their behaviour.
Im my patches above, I made the pref read in the nsThread constructor only, so the pref is no longer "live"

Depends on D30463

This will allow to remove gfxPrefs later. On Windows in particular, the need to decide gfxPrefs vs StaticPrefs for the WMF decoders has caused several bugs in the past.
We will remove the confusion as a consequence.

Depends on D30464

Attachment #9063727 - Attachment description: Bug 1548795 - P7. Sync preferences with RDD process when then changed. r?mattwoodrow!,mjf! → Bug 1548795 - P7. Sync preferences with RDD process when they changed. r?mattwoodrow!,mjf!
Attachment #9063455 - Attachment is obsolete: true
Blocks: 1550422
Attachment #9063727 - Attachment is obsolete: true
Attachment #9063726 - Attachment is obsolete: true
Attachment #9063725 - Attachment is obsolete: true
Attachment #9063723 - Attachment is obsolete: true
Attachment #9063722 - Attachment is obsolete: true
Blocks: 1479716
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58de421f3fd0
P1. Don't use StaticPrefs in GPU process. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/43a787dd4553
P2. Remove dom.performance.enable_scheduler_timing preference. r=tarek
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

There was no uplift request and we are building RC today, are you going to request uplift or do you intent to request uplift for a potential 67 dot release?

Flags: needinfo?(jyavenard)

Comment on attachment 9062837 [details]
Bug 1548795 - P1. Don't use StaticPrefs in GPU process. r?mattwoodrow

Beta/Release Uplift Approval Request

  • User impact if declined: VP9 HW decoding may be randomly disabled leading to performance drop on video sites such as YouTube
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changing the accessor to the preference.
    Patch 2 is removing unused code, which has been unused for over one year
  • String changes made/needed:
Flags: needinfo?(jyavenard)
Attachment #9062837 - Flags: approval-mozilla-beta?
Attachment #9063732 - Flags: approval-mozilla-beta?

Sorry, I thought my request for tracking implied request to uplift to beta

Flags: needinfo?(nfroyd)

Comment on attachment 9062837 [details]
Bug 1548795 - P1. Don't use StaticPrefs in GPU process. r?mattwoodrow

Since we are past the beta cycle, I am approving this for 67 RC.

Attachment #9062837 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Attachment #9063732 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9063732 - Flags: approval-mozilla-beta+ → approval-mozilla-release+

Tried to uplift this to release and got a conflict using graft:

mozilla@ubuntu ~/mozilla-unified release(+1) $ hg graft -er 58de421f3fd0::43a787dd4553
grafting 540110:58de421f3fd0 "Bug 1548795 - P1. Don't use StaticPrefs in GPU process. r=mattwoodrow"
merging gfx/thebes/gfxPrefs.h
merging modules/libpref/init/StaticPrefList.h
grafting 540111:43a787dd4553 "Bug 1548795 - P2. Remove dom.performance.enable_scheduler_timing preference. r=tarek"
merging dom/base/DocGroup.cpp
merging dom/base/TimeoutManager.cpp
merging dom/chrome-webidl/ChromeUtils.webidl
merging dom/ipc/ContentChild.cpp
merging dom/ipc/ContentParent.cpp
merging dom/workers/WorkerDebugger.cpp
merging dom/workers/WorkerPrivate.cpp
merging dom/workers/WorkerPrivate.h
merging modules/libpref/init/StaticPrefList.h
merging modules/libpref/init/all.js
merging xpcom/threads/nsThread.cpp
warning: conflicts while merging modules/libpref/init/StaticPrefList.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging modules/libpref/init/all.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Flags: needinfo?(jyavenard)

To resolve the conflict you can use the version from the P1 patch for StatucPrefList.h and all.js, and then just remove the enable_scheduler_timing pref as done in the second patch. That's the pref we're getting ridd of

Flags: needinfo?(jyavenard)
See Also: → 1554559
You need to log in before you can comment on or make changes to this bug.