Closed Bug 1175005 Opened 5 years ago Closed 5 years ago

Add monitor information to the telemetry environment

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(5 files, 6 obsolete files)

4.92 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.25 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.51 KB, patch
mstange
: review+
Details | Diff | Splinter Review
2.54 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.71 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
For each monitor we'd like to report its resolution, refresh rate, and whether or not it is a real monitor (versus something like a networked screen).
Attached patch part 1, nsIGfxInfo API (obsolete) — Splinter Review
Attachment #8622877 - Flags: review?(matt.woodrow)
Attached patch part 2, windows support (obsolete) — Splinter Review
Attachment #8622878 - Flags: review?(jmathies)
Attached patch part 3, OS X support (obsolete) — Splinter Review
Getting the refresh rate looked non-trivial on OS X so I left it out for now. If you know an easy way to query it I'll change it.
Attachment #8622882 - Flags: review?(mstange)
Attached patch part 4, linux support (obsolete) — Splinter Review
This doesn't support Xinerama and can't get the refresh rate, but I mostly just wanted it to support something vaguely accurate since the real target here is Windows.
Attachment #8622883 - Flags: review?(nical.bugzilla)
Attached patch part 5, telemetry reporting (obsolete) — Splinter Review
Some of the tests are Mac and Windows only since only they reliably report monitor information in xpcshell. (But again, those are really the only platforms we care about this telemetry for.)
Attachment #8622885 - Flags: review?(gfritzsche)
Attachment #8622878 - Flags: review?(jmathies) → review+
Comment on attachment 8622885 [details] [diff] [review]
part 5, telemetry reporting

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1137,5 @@
> +          refreshRates = {},
> +          pseudoDisplays = {},
> +          monitorCount = {};
> +      gfxInfo.getMonitors(monitorCount, screenWidths, screenHeights, refreshRates,
> +                          pseudoDisplays);

Side-note:
That is a surprising interface - couldn't it just return a jsval which is an array of {width:...,height:...,refreshRate:...,isPseudoDisplay:...} ?

@@ +1146,5 @@
> +          refreshRate: refreshRates.value[i],
> +          pseudoDisplay: pseudoDisplays.value[i],
> +        });
> +      }
> +    } catch (e) {

Where does this throw?
If this is to handle non-desktop platforms, can we skip retrieving that info on non-desktop platforms and log an error or warning on desktop?

::: toolkit/components/telemetry/docs/environment.rst
@@ +132,5 @@
>                  GPUActive: <bool>, // currently always true for the first adapter
>                },
>                ...
>              ],
> +            monitors: [ // Note: current only filled on Desktop.

Comment that this only has really reliable info on Windows?

@@ +136,5 @@
> +            monitors: [ // Note: current only filled on Desktop.
> +              {
> +                screenWidth: <number>,  // screen width in pixels
> +                screenHeight: <number>, // screen height in pixels
> +                refreshRate: <number>,  // refresh rate in hertz

Comment that this doesn't contain info on OS X?
Attachment #8622885 - Flags: review?(gfritzsche)
Comment on attachment 8622882 [details] [diff] [review]
part 3, OS X support

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

::: widget/cocoa/GfxInfo.mm
@@ +384,5 @@
> +{
> +  NSArray* screens = [NSScreen screens];
> +  unsigned screenCount = [screens count];
> +  for (unsigned i = 0; i < screenCount; i++) {
> +    NSScreen* screen = [screens objectAtIndex: i];

You can use Objective C fast enumeration here:
for (NSScreen* screen in [NSScreen screens]) {

@@ +392,5 @@
> +    // CVDisplayLinkGetNominalOutputVideoRefreshPeriod, but that's a little
> +    // involved. Ideally we could query it from vsync. For now, leave it 0.
> +    MonitorInfo info;
> +    info.screenWidth = (int)rect.size.width;
> +    info.screenHeight = (int)rect.size.height;

No resolution information? I suppose we have a separate piece of telemetry for that... but you might want to add a comment here that these values are in screen units, not in device pixels, so a HiDPI screen with a physical pixel count of 2880x1800 will be recorded as 1440x900.
Attachment #8622882 - Flags: review?(mstange) → review+
Comment on attachment 8622877 [details] [diff] [review]
part 1, nsIGfxInfo API

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

::: widget/GfxInfoBase.cpp
@@ +1177,5 @@
> +
> +  *aOutScreenWidths = (int32_t *)moz_xmalloc(monitors.Length() * sizeof(int32_t));
> +  *aOutScreenHeights = (int32_t *)moz_xmalloc(monitors.Length() * sizeof(int32_t));
> +  *aOutRefreshRates = (int32_t *)moz_xmalloc(monitors.Length() * sizeof(int32_t));
> +  *aOutIsPseudoDisplay = (bool *)moz_xmalloc(monitors.Length() * sizeof(bool));

moz_xmalloc is infallible, no need to check the return values (or use moz_malloc).

::: widget/nsIGfxInfo.idl
@@ +57,5 @@
> +  void getMonitors(out unsigned long monitorCount,
> +                   [array, size_is(monitorCount)] out long screenWidths,
> +                   [array, size_is(monitorCount)] out long screenHeights,
> +                   [array, size_is(monitorCount)] out long refreshRates,
> +                   [array, size_is(monitorCount)] out boolean isPseudoDisplay);

Need to update the uuid for this class.

Would be nice if we could return an array of objects rather than all these outparams, might be worth getting a second review from someone who knows .idl better than I do.
Attachment #8622877 - Flags: review?(matt.woodrow) → review+
I avoided returning a jsval at first since jsapi is really verbose. But it makes the telemetry reporting a little nicer since now we can just not report inaccurate fields.
Attachment #8622877 - Attachment is obsolete: true
Attachment #8623245 - Flags: review?(matt.woodrow)
(In reply to Markus Stange [:mstange] from comment #7)
> so a HiDPI screen with a physical
> pixel count of 2880x1800 will be recorded as 1440x900.

That's actually not completely true. It might also be reported as 1920x1200, or as any other resolution that the user has chosen. So it doesn't really have much to do with physical pixels anyway, and I think that's fine.
Same as the previous patch, just uses JSAPI instead.
Attachment #8622878 - Attachment is obsolete: true
Attachment #8623260 - Flags: review?(jmathies)
Now includes backingScaleFactor.
Attachment #8622882 - Attachment is obsolete: true
Attachment #8623262 - Flags: review?(mstange)
Attachment #8622883 - Attachment is obsolete: true
Attachment #8622883 - Flags: review?(nical.bugzilla)
Attachment #8623265 - Flags: review?(nical.bugzilla)
Attachment #8623245 - Flags: review?(matt.woodrow) → review+
Attached patch part 5, telemetry reporting v2 (obsolete) — Splinter Review
Addressed review comments, I think. I kept the try-catch because (1) all the other nsIGfxInfo calls are similarly protected (though I don't know why), and (2) the underlying OS calls can fail, though I could change it to never throw an error in that case.

It'd be nice to have mobile just start working if someone implements the API call. So if the try-catch is a problem I'd rather just remove the exception and return an empty list, than make the query desktop-only.
Attachment #8622885 - Attachment is obsolete: true
Attachment #8623267 - Flags: review?(gfritzsche)
Attachment #8623262 - Flags: review?(mstange) → review+
Comment on attachment 8623267 [details] [diff] [review]
part 5, telemetry reporting v2

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1133,5 @@
> +    let gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
> +    try {
> +      gfxData.monitors = gfxInfo.getMonitors();
> +    } catch (e) {
> +    }

If we don't expect this to throw usually on desktop, we should log an error here.
Otherwise we won't notice locally when this is broken.

We don't want to log errors on mobile or B2G though because it is not implemented there. It is easy to remove a check like that in the future, so i don't think we need to worry about this.

::: toolkit/components/telemetry/docs/environment.rst
@@ +132,5 @@
>                  GPUActive: <bool>, // currently always true for the first adapter
>                },
>                ...
>              ],
> +            // Note: current only filled on Desktop. On Linux, currently only a

Nit: "currently only added"
Attachment #8623267 - Flags: review?(gfritzsche)
(In reply to David Anderson [:dvander] from comment #14)
> Addressed review comments, I think. I kept the try-catch because (1) all the
> other nsIGfxInfo calls are similarly protected (though I don't know why),
> and (2) the underlying OS calls can fail, though I could change it to never
> throw an error in that case.
> 
> It'd be nice to have mobile just start working if someone implements the API
> call. So if the try-catch is a problem I'd rather just remove the exception
> and return an empty list, than make the query desktop-only.

I don't mind really which way we go, but i'd like to avoid to silence errors in the data collection.
Attachment #8623267 - Attachment is obsolete: true
Attachment #8623546 - Flags: review?(gfritzsche)
Comment on attachment 8623546 [details] [diff] [review]
part 5, telemetry reporting

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

Thanks!
Attachment #8623546 - Flags: review?(gfritzsche) → review+
Attachment #8623260 - Flags: review?(jmathies) → review+
Attachment #8623265 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8623245 [details] [diff] [review]
part 1, nsIGfxInfo changes v2

>+++ b/widget/GfxInfoBase.h
>@@ -49,17 +49,18 @@ public:
>   NS_IMETHOD GetWebGLParameter(const nsAString & aParam, nsAString & _retval) override;
> 
>-    NS_IMETHOD GetFailures(uint32_t *failureCount, int32_t** indices, char ***failures) override;
>+  NS_IMETHOD GetMonitors(JSContext* cx, JS::MutableHandleValue _retval);
>+  NS_IMETHOD GetFailures(uint32_t *failureCount, int32_t** indices, char ***failures) override;
>   NS_IMETHOD_(void) LogFailure(const nsACString &failure) override;
>   NS_IMETHOD GetInfo(JSContext*, JS::MutableHandle<JS::Value>) override;

The new GetMonitors() decl here was missing an 'override' annotation, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32aa45a637cb
Depends on: 1176856
backed out due to perf regression with no response in bug 1176856.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
maybe I didn't do the backout to be McMerge friendly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.