Matt Woodrow (:mattwoodrow) Reporter
	Description • 8 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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 1 • 8 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 2 • 8 years ago

Once 53 starts, we can change 52-affected to just 53-affected as GPU process is nightly only for now.

	Marcia Knous [:marcia]
	Comment 4 • 8 years ago

[Tracking Requested - why for this release]: Carrying over tracking flag from dupe bug.

	Marcia Knous [:marcia]
	Comment 6 • 8 years ago

Tracking 52+ for this issue.

	Julien Cristau [:jcristau]
	Comment 7 • 8 years ago

moving tracking/status flags from 52 to 53 per comment 2, this only affects nightly

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 8 • 8 years ago

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?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 9 • 8 years ago

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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 10 • 8 years ago

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...

	sworddragon2
	Comment 11 • 8 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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 12 • 8 years ago

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.

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 13 • 8 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.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 14 • 8 years ago

(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?

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 15 • 8 years ago

(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.

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 16 • 8 years ago

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.

	Pulsebot
	Comment 17 • 8 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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 18 • 8 years ago

Patches that landed are slightly modified, but nothing important changed so carrying r+.

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fe2a50492750
https://hg.mozilla.org/mozilla-central/rev/742a79439ccf

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 8 years ago

George,  want to request uplift to aurora here?

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 21 • 8 years ago

Yes, we probably should. I will prepare the patches and make the appropriate flags. Thanks for following up on this!

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 22 • 8 years ago

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

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 23 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 25 • 8 years ago

This also affects 52. Not sure it is worth beta uplift but please ask if you would like that.

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/98cb18e24433
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2bfbcf5430c

	George Wright (:gw280) (needinfo me!) Assignee
	Comment 27 • 8 years ago

This doesn't affect Beta as the GPU process isn't enabled there (the patch is in 52, but it's ifdef NIGHTLY only).

	Andrei Vaida [:avaida]
	Comment 28 • 7 years ago

Setting qe-verify- based on George's assessment on manual testing needs (see Comment 23).