Move MEDIA_CAN_CREATE_XX_DECODER telemetry to off-main-thread

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
2 months ago
15 days ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1][bhr])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

2 months ago
As pointed out in bug 1300126, we're calling HTMLMediaElement.canPlayType() on the main thread in the chrome process (from JS) in order to report telemetry as to whether the user's machine can play H.264 and AAC.

This will cause us to load the WMF DLLs from disk. So that means we're doing I/O on the main thread of the chrome process. Meaning we're likely causing jank.
(Assignee)

Comment 1

2 months ago
Created attachment 8864744 [details]
BHR showing canPlayType hanging for > 8 seconds

I also found a handful of BHR showing HTMLMediaElement.canPlayType hanging for > 8 seconds in the last BHR dump [1].

[1] https://people-mozilla.org/~mlayzell/bhr/20170429/hangs.json
(Assignee)

Updated

2 months ago
Whiteboard: [qf][bhr]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P1

Comment 4

2 months ago
mozreview-review
Comment on attachment 8864746 [details]
Bug 1362212 - Remove canPlayTelemetry from _delayedStartup.

https://reviewboard.mozilla.org/r/136402/#review139560
Attachment #8864746 - Flags: review+

Updated

2 months ago
Attachment #8864746 - Flags: review?(mconley)

Comment 5

2 months ago
mozreview-review
Comment on attachment 8864745 [details]
Bug 1362212 - Move canPlayType telemetry to an idle service observer off main thread.

https://reviewboard.mozilla.org/r/136400/#review139856

r+ for the code.

But I'm wondering (aloud) if we still need this level of commitment to test aac/h264 decoders at every run.
What about catching voluntary aac/h264 checks (when the user is actually trying to play some media) and reporting the first result for each?
Of course, this would slightly change the meaning of these telemetry probes, but it may in fact make them better match actual use.

Just a suggestion! If you agree, it would need more work, which should probably be done in a future bug; so this one here is still welcome anyway to remove existing jank.
What do you think?
Attachment #8864745 - Flags: review?(gsquelart) → review+

Comment 6

2 months ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f263d6fb16d
Move canPlayType telemetry to an idle service observer off main thread. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ccb5b3503b66
Remove canPlayTelemetry from _delayedStartup. r=dao
Backed out for build bustage on Linux - fails testing/xpcshell/selftest.py | XPCShellTestsTests.testAddTaskStackTrace:

https://hg.mozilla.org/integration/autoland/rev/903d3921e250855908266e5c31d7ef29eec20b46
https://hg.mozilla.org/integration/autoland/rev/5c9a70a7af09885ca53bfb26c3fcc194ba738a41

The push has also many more failure, e.g. in gl suites on Windows and everything on Android: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ccb5b3503b663283ce238d10db8fc0188dfe9f98&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Build log with failure: https://treeherder.mozilla.org/logviewer.html#?job_id=97176418&repo=autoland

[task 2017-05-07T20:29:44.966346Z] 20:29:44     INFO - TEST-PASS | /home/worker/workspace/build/src/python/mozbuild/mozbuild/test/configure/test_toolkit_moz_configure.py | TestToolkitMozConfigure.test_necko_protocols
[task 2017-05-07T20:29:50.341108Z] 20:29:50     INFO - /home/worker/workspace/build/src/testing/xpcshell/selftest.py
[task 2017-05-07T20:29:50.341421Z] 20:29:50     INFO - TEST-PASS | /home/worker/workspace/build/src/testing/xpcshell/selftest.py | XPCShellTestsTests.testAddTaskRunNextTest
[task 2017-05-07T20:29:50.341739Z] 20:29:50  WARNING - TEST-UNEXPECTED-FAIL | /home/worker/workspace/build/src/testing/xpcshell/selftest.py | XPCShellTestsTests.testAddTaskStackTrace, line 1029: Value this_test_will_fail expected in log:
[task 2017-05-07T20:29:50.343396Z] 20:29:50     INFO - FAIL: testAddTaskStackTrace (__main__.XPCShellTestsTests)
[task 2017-05-07T20:29:50.343452Z] 20:29:50     INFO - Traceback (most recent call last):
[task 2017-05-07T20:29:50.343517Z] 20:29:50     INFO -   File "/home/worker/workspace/build/src/testing/xpcshell/selftest.py", line 1029, in testAddTaskStackTrace
[task 2017-05-07T20:29:50.343563Z] 20:29:50     INFO -     self.assertInLog("this_test_will_fail")
[task 2017-05-07T20:29:50.343623Z] 20:29:50     INFO -   File "/home/worker/workspace/build/src/testing/xpcshell/selftest.py", line 504, in assertInLog
[task 2017-05-07T20:29:50.343662Z] 20:29:50     INFO -     self._assertLog(s, True)
[task 2017-05-07T20:29:50.343720Z] 20:29:50     INFO -   File "/home/worker/workspace/build/src/testing/xpcshell/selftest.py", line 498, in _assertLog
[task 2017-05-07T20:29:50.343771Z] 20:29:50     INFO -     ========""" % (s, "expected" if expected else "not expected", l))
[task 2017-05-07T20:29:50.343913Z] 20:29:50     INFO - AssertionError: Value this_test_will_fail expected in log:
Flags: needinfo?(cpearce)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
This patch failed because the idle service observer was firing in xpcshell.exe, and the test for whether H264 is supported accesses graphics stuff, and that isn't available in xpcshell. We can simply test whether we're in Firefox and only report the telemetry there.
Flags: needinfo?(cpearce)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8864745 - Attachment is obsolete: true

Comment 15

a month ago
mozreview-review
Comment on attachment 8866144 [details]
Bug 1362212 - Implement HTMLMediaElement.reportCanPlayTelemetry.

https://reviewboard.mozilla.org/r/137746/#review140892

r+ with nit.
And have you thought about my suggestion in comment 5?

::: dom/media/MediaPrefs.h:184
(Diff revision 1)
>    DECL_MEDIA_PREF("media.rust.mp4parser",                     EnableRustMP4Parser, bool, true);
>  #else
>    DECL_MEDIA_PREF("media.rust.mp4parser",                     EnableRustMP4Parser, bool, false);
>  #endif
>  
> +  DECL_MEDIA_PREF("media.mp4.enabled", MP4Enabled, bool, false);

For consistency, could you please align `MP4Enabled` with the other function names in this list?
Attachment #8866144 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 16

a month ago
(In reply to Gerald Squelart [:gerald] from comment #15)
> Comment on attachment 8866144 [details]
> Bug 1362212 - Implement HTMLMediaElement.reportCanPlayTelemetry.
> 
> https://reviewboard.mozilla.org/r/137746/#review140892
> 
> r+ with nit.
> And have you thought about my suggestion in comment 5?
> 
> ::: dom/media/MediaPrefs.h:184
> (Diff revision 1)
> >    DECL_MEDIA_PREF("media.rust.mp4parser",                     EnableRustMP4Parser, bool, true);
> >  #else
> >    DECL_MEDIA_PREF("media.rust.mp4parser",                     EnableRustMP4Parser, bool, false);
> >  #endif
> >  
> > +  DECL_MEDIA_PREF("media.mp4.enabled", MP4Enabled, bool, false);
> 
> For consistency, could you please align `MP4Enabled` with the other function
> names in this list?

clang-format strikes again!
Comment on attachment 8866143 [details]
Bug 1362212 - Add chromeonly HTMLMediaElement.reportCanPlayTelemetry.

https://reviewboard.mozilla.org/r/137744/#review141210

::: dom/webidl/HTMLMediaElement.webidl:143
(Diff revision 1)
>    // the media element has a fragment URI for the currentSrc, otherwise
>    // it is equal to the media duration.
>    readonly attribute double mozFragmentEnd;
> +
> +  [ChromeOnly]
> +  void ReportCanPlayTelemetry();

Lowercase "report", please.  And update the last patch in this bug accordingly.

r=me with that.
Attachment #8866143 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 18

a month ago
Comment on attachment 8866145 [details]
Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once after browser startup.

Jared: Are you able to review this? I think mconley is busy.
Attachment #8866145 - Flags: review?(mconley) → review?(jaws)

Comment 19

a month ago
mozreview-review
Comment on attachment 8866145 [details]
Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once after browser startup.

https://reviewboard.mozilla.org/r/137748/#review141304

Thanks for the patch! Glad to get rid of this main thread work!

Generally, this looks fine - but I have a few questions. Clearing review flag until I hear more.

::: browser/components/nsBrowserGlue.js:219
(Diff revision 1)
>  const BOOKMARKS_BACKUP_MIN_INTERVAL_DAYS = 1;
>  // Maximum interval between backups.  If the last backup is older than these
>  // days we will try to create a new one more aggressively.
>  const BOOKMARKS_BACKUP_MAX_INTERVAL_DAYS = 3;
> +// Seconds of idle time before reporting media telemetry.
> +const MEDIA_TELEMETRY_IDLE_TIME_SEC = 20;

Out of curiosity, is this just a made up number, or do we have some confidence that 20s of idle time is a good time to do stuff like this?

::: browser/components/nsBrowserGlue.js:965
(Diff revision 1)
>      DateTimePickerHelper.init();
>  
>      this._firstWindowTelemetry(aWindow);
>      this._firstWindowLoaded();
> +
> +    this._mediaTelemetryIdleObserver = {

Out of curiosity, why the choice to create and stash your own observer here instead of adding nsBrowserGlue (which implements nsIObserver) itself?
(In reply to Chris Pearce (:cpearce) from comment #18)
> Comment on attachment 8866145 [details]
> Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once
> after browser startup.
> 
> Jared: Are you able to review this? I think mconley is busy.

I was able to review today, but had questions. Please see above.
Flags: needinfo?(cpearce)
(Assignee)

Comment 21

a month ago
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #20)
> (In reply to Chris Pearce (:cpearce) from comment #18)
> > Comment on attachment 8866145 [details]
> > Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once
> > after browser startup.
> > 
> > Jared: Are you able to review this? I think mconley is busy.
> 
> I was able to review today, but had questions. Please see above.

Oops, sorry no problem!

(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #19)
> Comment on attachment 8866145 [details]
> Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once
> after browser startup.
> 
> https://reviewboard.mozilla.org/r/137748/#review141304
> 
> Thanks for the patch! Glad to get rid of this main thread work!
> 
> Generally, this looks fine - but I have a few questions. Clearing review
> flag until I hear more.
> 
> ::: browser/components/nsBrowserGlue.js:219
> (Diff revision 1)
> >  const BOOKMARKS_BACKUP_MIN_INTERVAL_DAYS = 1;
> >  // Maximum interval between backups.  If the last backup is older than these
> >  // days we will try to create a new one more aggressively.
> >  const BOOKMARKS_BACKUP_MAX_INTERVAL_DAYS = 3;
> > +// Seconds of idle time before reporting media telemetry.
> > +const MEDIA_TELEMETRY_IDLE_TIME_SEC = 20;
> 
> Out of curiosity, is this just a made up number, or do we have some
> confidence that 20s of idle time is a good time to do stuff like this?

It's a made up number. I don't know what a good number for this is, so I just picked one.
 
> ::: browser/components/nsBrowserGlue.js:965
> (Diff revision 1)
> >      DateTimePickerHelper.init();
> >  
> >      this._firstWindowTelemetry(aWindow);
> >      this._firstWindowLoaded();
> > +
> > +    this._mediaTelemetryIdleObserver = {
> 
> Out of curiosity, why the choice to create and stash your own observer here
> instead of adding nsBrowserGlue (which implements nsIObserver) itself?

It's not clear to me what a good way to distinguish between the bookmarks archiving idle observer and the one added here would be.

The idle observer runs with an argument which is the duration idle, so one approach could be to check whether the idle time is greater than the _bookmarksBackupIdleTime or MEDIA_TELEMETRY_IDLE_TIME_SEC, and I'd need to track whether we'd already reported the idle telemetry. So just adding and removing a separate observer seemed to be cleaner overall.
Flags: needinfo?(cpearce)

Comment 22

a month ago
mozreview-review
Comment on attachment 8866145 [details]
Bug 1362212 - Call HTMLMediaElement.reportCanPlayTelemetry on idle once after browser startup.

https://reviewboard.mozilla.org/r/137748/#review141556

Alright, that's fair. The change is pretty straight-forward, and getting this stuff off of the main thread sounds worth it to me.
Attachment #8866145 - Flags: review+
Attachment #8866145 - Flags: review?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

a month ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0483029f6379
Remove canPlayTelemetry from _delayedStartup. r=dao
https://hg.mozilla.org/integration/autoland/rev/0aa0904d6ad8
Add chromeonly HTMLMediaElement.reportCanPlayTelemetry. r=bz
https://hg.mozilla.org/integration/autoland/rev/3522d1d79583
Implement HTMLMediaElement.reportCanPlayTelemetry. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b7703e22ff29
Call HTMLMediaElement.reportCanPlayTelemetry on idle once after browser startup. r=mconley
Whiteboard: [qf][bhr] → [qf:p1][bhr]

Comment 27

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0483029f6379
https://hg.mozilla.org/mozilla-central/rev/0aa0904d6ad8
https://hg.mozilla.org/mozilla-central/rev/3522d1d79583
https://hg.mozilla.org/mozilla-central/rev/b7703e22ff29
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1371282
You need to log in before you can comment on or make changes to this bug.