about:support graphics info only shows values for the UI process

RESOLVED FIXED in Firefox 53

Status

()

Core
Graphics: Layers
P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: mattwoodrow, Assigned: gw280)

Tracking

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 disabled, firefox53+ fixed, firefox54 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
The graphics values shown in about:support are what we computed for the UI process.

When we have the GPU process enabled, we try to disable all GPU things in the UI process, so we show the content backend as SKIA and that hardware video decoding isn't working.

These things are true for the UI process, but not for the content process (which are likely the ones we care about more).

We probably need a much more nuanced approach to reporting this data where we iterate specific windows (in the UI process) and places that forward content to windows (chrome in the UI process, tabs in the content process) and list the specific configuration for each of these.
(Assignee)

Updated

2 years ago
Assignee: nobody → gwright
An example where we forward this information: gfxPlatform::NotifyCompositorCreated and gfxPlatform::GetCompositorBackend.  Perhaps that's a reasonable pattern to follow, or even extract out of gfxPlatform.
Once 53 starts, we can change 52-affected to just 53-affected as GPU process is nightly only for now.
Priority: -- → P3
Whiteboard: [gfx-noted]
Duplicate of this bug: 1315540
[Tracking Requested - why for this release]: Carrying over tracking flag from dupe bug.
tracking-firefox52: --- → ?
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1316062
Tracking 52+ for this issue.
tracking-firefox52: ? → +
moving tracking/status flags from 52 to 53 per comment 2, this only affects nightly
status-firefox52: affected → ---
status-firefox53: --- → affected
tracking-firefox52: + → ---
tracking-firefox53: --- → +
Created attachment 8811981 [details] [diff] [review]
0001-IPC-over-diagnostic-info-from-content-gpu-processes.patch

Looking for feedback. My original idea was to add IPC methods to allow the GPU/content processes to communicate back to the UI process for whatever diagnostic info they wanted to communicate via about:support.

This patch implements that framework in a very generic way so that we can expand upon it if we ever need to add more diagnostic info than just backend type, etc.

A couple of issues:

- There's the possibility of intra-process discrepancies with gfx info; e.g. we can fail to create a swapchain and fall out of d3d layers on new window creation. Arguably this is a separate issue to this bug though.

- There are roughly two categories of diagnostic information we have in about:support for gfx; stuff that's known ahead of time, and stuff that's calculated at time of request. Notably, the H264 decoder support is in the latter. This works by calling MP4Decoder::IsVideoAccelerated(), which in turn attempts to create a decoder and checks ad-hoc whether it's functional and backed by hardware or not. See http://searchfox.org/mozilla-central/source/dom/media/fmp4/MP4Decoder.cpp#246. I'm not entirely sure what the best way to deal with this is.

In order to address the first point, it might be worth looking into operating on each window like we do here: http://searchfox.org/mozilla-central/source/toolkit/modules/Troubleshoot.jsm#340, and adding various methods to grab the relevant diagnostic info from them. layerManagerType is already a property on the nsIDOMWindowUtils interface that we can grab, so that might be all we need for per-window information, and the rest can be dealt with via the framework in this patch for more "general" diagnostics. Does that sound sane?
Attachment #8811981 - Flags: feedback?(matt.woodrow)
It feels like gfxVars are closely related here - except that these are the values we get in the UI process and then pass to content/gpu.  Perhaps we can somehow combine it with the concepts discussed here? Feels like we're solving a very similar problem, but perhaps too independently.
Discussed this with Matt and David in Hawaii. Here's the plan going forward:

- It doesn't make much sense to do the on-demand check for h264 by creating a dummy video decoder, as we could succeed in creating one for about:support but there's no info on what the actual videos are. Apparently Jean-Yves is working on a debug overlay for videos which will include accelerated status, which is better. Therefore we should just remove this from about:support.

- Due to information about Direct2D initialisation status being correct in about:support currently, we are able to work out which moz2d backends are in use for any given process. We can therefore remove the Moz2D backend info from about:support, as it's an oversimplification of our system and can't really be trusted to give accurate data (as it isn't right now). Or we can keep it around but label it explicitly as UI process.

---

There is one other thing we can do, but we ruled it out at our meeting, which is to iterate over each window and get each window's compositor backend, then iterate over each child process and tab child to get the moz2d backend for each tab. This was ruled out for a couple of reasons:

- Lots of effort for no real gain. We aren't particularly interested in making about:support user friendly; it's there to give the info required for us to debug issues, and this doesn't particularly help towards that goal.

- Potential perf impact when you have lots of windows/tabs/child processes/etc. I certainly saw a fair share of bugs when on e10s from users who had a truly mind boggling number of tabs/windows...
(Assignee)

Updated

2 years ago
Attachment #8811981 - Flags: feedback?(matt.woodrow)

Comment 11

2 years ago
(In reply to George Wright (:gw280) (:gwright) from comment #10)
> - It doesn't make much sense to do the on-demand check for h264 by creating
> a dummy video decoder, as we could succeed in creating one for about:support
> but there's no info on what the actual videos are.

Eventually this information can give a clue if the users Firefox instance is actually capable of utilizing hardware acceleration for H.264 at all and therefore should be maybe labeled as UI process too.
Created attachment 8820556 [details] [diff] [review]
0001-Bug-1314803-Remove-current-Azure-backend-and-hardwar.patch

As per comment 10, let's remove this.

Speaking to Jean-Yves, we would probably derive value by putting in a new field to state whether H264 decoding is present or not, rather than h/w accelerated when creating a dummy decoder. This will make it easy for us to determine if a user has the required libraries installed to support H264.

That shouldn't be too hard to implement, but should be done in a different bug.

I believe for everything else, either the diagnostic overlay or about:media should cover those.
Attachment #8820556 - Flags: review?(matt.woodrow)
(Reporter)

Comment 13

2 years ago
Comment on attachment 8820556 [details] [diff] [review]
0001-Bug-1314803-Remove-current-Azure-backend-and-hardwar.patch

Review of attachment 8820556 [details] [diff] [review]:
-----------------------------------------------------------------

Let's keep the azure backend stuff for now, it's still somewhat meaningful when the gpu process isn't enabled.
Attachment #8820556 - Flags: review?(matt.woodrow) → review+
(In reply to George Wright (:gw280) (:gwright) from comment #10)
> ...
> - Due to information about Direct2D initialisation status being correct in
> about:support currently, we are able to work out which moz2d backends are in
> use for any given process...

If we are indeed able to work this out, why can't we display the results in about:support instead of having to get it from second hand information?
(In reply to Milan Sreckovic [:milan] from comment #14)
> (In reply to George Wright (:gw280) (:gwright) from comment #10)
> > ...
> > - Due to information about Direct2D initialisation status being correct in
> > about:support currently, we are able to work out which moz2d backends are in
> > use for any given process...
> 
> If we are indeed able to work this out, why can't we display the results in
> about:support instead of having to get it from second hand information?

Fair enough. This shouldn't be too difficult to do, I suspect.
Created attachment 8830462 [details] [diff] [review]
0001-Bug-1314803-Report-correct-values-for-Azure-backends.patch

This basically determines if we have D2D enabled in the same way at we do here: http://searchfox.org/mozilla-central/source/widget/windows/GfxInfo.cpp#59

Will carry the r+ on the previous patch for removal of H264 info.
Attachment #8820556 - Attachment is obsolete: true
Attachment #8830462 - Flags: review?(matt.woodrow)
(Reporter)

Updated

2 years ago
Attachment #8830462 - Flags: review?(matt.woodrow) → review+

Comment 17

2 years ago
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2a50492750
Report correct values for Azure backends when the GPU process is enabled on Windows r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/742a79439ccf
Remove supportsHardwareH264 from about:support r=mattwoodrow
Patches that landed are slightly modified, but nothing important changed so carrying r+.

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe2a50492750
https://hg.mozilla.org/mozilla-central/rev/742a79439ccf
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
George,  want to request uplift to aurora here?
Flags: needinfo?(gwright)
Yes, we probably should. I will prepare the patches and make the appropriate flags. Thanks for following up on this!
Created attachment 8834193 [details] [diff] [review]
0001-Bug-1314803-Report-correct-values-for-Azure-backends.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1314133
[User impact if declined]: some incorrect data can be shown in about:support if the GPU process is on
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: other patch on this bug
[Is the change risky?]: no
[Why is the change risky/not risky?]: just changing some diagnostic reporting; no functional changes
[String changes made/needed]: none that are localised
Flags: needinfo?(gwright)
Attachment #8834193 - Flags: review+
Attachment #8834193 - Flags: approval-mozilla-aurora?
Created attachment 8834194 [details] [diff] [review]
0002-Bug-1314803-Remove-supportsHardwareH264-from-about-s.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1314133
[User impact if declined]: some incorrect data can be shown in about:support if the GPU process is on
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: other patch on this bug
[Is the change risky?]: no
[Why is the change risky/not risky?]: just changing some diagnostic reporting; no functional changes
[String changes made/needed]: none that are localised
Attachment #8834194 - Flags: review+
Attachment #8834194 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8830462 - Attachment is obsolete: true
Comment on attachment 8834193 [details] [diff] [review]
0001-Bug-1314803-Report-correct-values-for-Azure-backends.patch

Fix for GPU reporting, regression from 52. Let's uplift to aurora.
Attachment #8834193 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This also affects 52. Not sure it is worth beta uplift but please ask if you would like that.
status-firefox52: --- → affected

Updated

2 years ago
Attachment #8834194 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This doesn't affect Beta as the GPU process isn't enabled there (the patch is in 52, but it's ifdef NIGHTLY only).
status-firefox52: affected → disabled
Setting qe-verify- based on George's assessment on manual testing needs (see Comment 23).
Flags: qe-verify-

Updated

5 months ago
See Also: → bug 1432039
You need to log in before you can comment on or make changes to this bug.