Closed Bug 1002846 Opened 6 years ago Closed 3 years ago

send telemetry on why GL compositor didn't initialize

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: gal, Assigned: ernest)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 999445
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gal
Attachment #8414132 - Flags: review?(bgirard)
Glad we're talking of using telemetry and not crashing on GL initialization failure, as Mac GL layers success rates are still not back to 100% (plateauing around 95% this week).
Attachment #8414132 - Flags: review?(bgirard) → review+
If we don't already, we will want something equivalent in the new feature/telemetry reporting setup.
Flags: needinfo?(bgirard)
Perhaps ernest can look at this. We ideally want to do something like bug 1272808 but for the OpenGL Compositor. If that goes well we will probably want to cover windows compositor decisions too.
Flags: needinfo?(bgirard) → needinfo?(eyim)
Attachment #8414132 - Attachment is obsolete: true
Flags: needinfo?(eyim)
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/1-2/
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

https://reviewboard.mozilla.org/r/56974/#review54436

Looks good, as discussed we're going to bubble up the failureid and have the caller decide what to do with it.
Attachment #8758858 - Flags: review?(bgirard)
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/2-3/
Attachment #8758858 - Attachment description: MozReview Request: Bug 1002846 - Add telemetry to OpenGL compositor → Bug 1002846 - Add telemetry to OpenGL compositor
Attachment #8758858 - Flags: review?(bgirard)
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/3-4/
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

https://reviewboard.mozilla.org/r/56974/#review57862

Looks good, we just need to decide if we're going to combine the data together or split it out. r- since I'd like to look at the revision again.

::: gfx/layers/opengl/CompositorOGL.cpp:434
(Diff revisions 3 - 4)
>      }
>    }
>  
>    reporter.SetSuccessful();
>  
> -  *out_failureReason = "FEATURE_FAILURE_OPENGL_ARB_EXT";
> +  *out_failureReason = "SUCCESS";

and here

::: gfx/tests/gtest/TestCompositor.cpp:143
(Diff revisions 3 - 4)
>      MOZ_CRASH(); // No support yet
>  #endif
>    }
>    nsCString failureReason;
>    if (!compositor || !compositor->Initialize(&failureReason)) {
> -    Telemetry::Accumulate(Telemetry::OPENGL_COMPOSITING_FAILURE_ID, failureReason);
> +    Telemetry::Accumulate(Telemetry::COMPOSITING_FAILURE_ID, failureReason);

I'm not sure we want to report this in the test code. I'm not sure if Telemetry is setup correctly under GTest. It's best to just skip this. We're going to log and crash anyways.

::: toolkit/components/telemetry/Histograms.json:10917
(Diff revisions 3 - 4)
> -   "OPENGL_COMPOSITING_FAILURE_ID": {
> +   "COMPOSITING_FAILURE_ID": {
>      "alert_emails": ["bgirard@mozilla.com"],
>      "expires_in_version": "never",
>      "kind": "count",
>      "keyed": true,
> -    "description": "OpenGL compositor runtime and dynamic failure IDs. This will record a count for each context creation success or failure. Each failure id is a unique identifier that can be traced back to a particular failure branch or blocklist rule.",
> +    "description": "OpenGL / D3D11 / D3D9 compositor runtime and dynamic failure IDs. This will record a count for each context creation success or failure. Each failure id is a unique identifier that can be traced back to a particular failure branch or blocklist rule.",

I'm not convinced that we should combined these. Can you justify this? On one side we'd be sending/collecting less data but analyzing the data will be more difficult. We wont be able to know exactly how many people succeed/fail for each feature. We might end up with a mix of a failure id for one feature and a success for another feature in the same ping. Then we will aggregate many such pings together and it's going to be difficult to make sense of the data.

::: gfx/layers/d3d11/CompositorD3D11.cpp:461
(Diff revision 4)
> +    *out_failureReason = "FEATURE_FAILURE_D3D11_INIT_COMPOSITOR";
>      return false;
>    }
>  
>    reporter.SetSuccessful();
> +  *out_failureReason = "SUCCESS";

I think it's best to leave failureReason alone if there's no failure. This way we can check for empty. We should check 'if (failureReason.IsEmpty()) { failureReason = "SUCCESS"; }' as we're about to report to telemetry.

::: gfx/layers/d3d9/CompositorD3D9.cpp:63
(Diff revision 4)
> +    *out_failureReason = "FEATURE_FAILURE_D3D9_INIT_COMPOSITOR";
>      return false;
>    }
>  
>    reporter.SetSuccessful();
> +  *out_failureReason = "SUCCESS";

and here
Attachment #8758858 - Flags: review?(bgirard) → review-
Will make changes, as for the question of why I combined all the telemetry into one... there were several options, either have one for d3d and one for ogl, one for d3d and ogl, or one for each d3d9, d3d11 and ogl. (I needed to implement the windows (d3d compositor side) due to the initialization override.) I don't have a particular strong reasoning for choosing having only one failure, except that we can find all our compositor failures in one location, rather than checking three.
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/4-5/
Attachment #8758858 - Flags: review- → review?(bgirard)
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

https://reviewboard.mozilla.org/r/56974/#review57980

Looks good!

::: gfx/layers/opengl/CompositorOGL.cpp:244
(Diff revision 5)
> +    *out_failureReason = "FEATURE_FAILURE_OPENGL_NO_ANDROID_CONTEXT";
>      NS_RUNTIMEABORT("We need a context on Android");
>  #endif
>  
>    if (!mGLContext)
> +  {

No new line here.
Attachment #8758858 - Flags: review?(bgirard) → review+
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/5-6/
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/6-7/
I'm assuming this patch is causing this to appear in about:support Graphics section:

Decision Log
------------
OPENGL_COMPOSITING	Blocklisted; failure code BLOCKLIST_FEATURE_FAILURE_OGL_ATI_DIS

I am using the very latest AMD Crimson drivers on Windows 10.  The OpenGL version in these drivers is 6.14.10.13440.  Is the blocking a mistake?


Graphics
--------

Features
Compositing: Direct3D 11
Asynchronous Pan/Zoom: wheel input enabled; touch input enabled
WebGL Renderer: Google Inc. -- ANGLE (AMD Radeon (TM) R9 390 Series Direct3D11 vs_5_0 ps_5_0)
Hardware H264 Decoding: Yes; Using D3D11 API
Direct2D: true
DirectWrite: true (10.0.10586.0)
GPU #1
Active: Yes
Description: AMD Radeon (TM) R9 390 Series
Vendor ID: 0x1002
Device ID: 0x67b1
Driver Version: 16.200.1035.0
Driver Date: 6-21-2016
Drivers: aticfx64 aticfx64 aticfx64 amdxc64 aticfx32 aticfx32 aticfx32 amdxc32 atiumd64 atidxx64 atidxx64 atiumdag atidxx32 atidxx32 atiumdva atiumd6a atitmm64
Subsys ID: 00000000
RAM: 4095

Diagnostics
AzureCanvasAccelerated: 0
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo

Decision Log
OPENGL_COMPOSITING:
Blocklisted; failure code BLOCKLIST_FEATURE_FAILURE_OGL_ATI_DIS
Should be Bug 1277314, will take a look, as the opengl feature and blocklist check was just added in that patch.
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/7-8/
Comment on attachment 8758858 [details]
Bug 1002846 - Add telemetry to OpenGL compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56974/diff/8-9/
Pushed by b56girard@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e56956f272d
Add telemetry to OpenGL compositor r=BenWa
https://hg.mozilla.org/mozilla-central/rev/9e56956f272d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1288829
Depends on: 1289873
Blocks: 1254008
Assignee: gal → eyim
You need to log in before you can comment on or make changes to this bug.