Closed Bug 1362212 Opened 4 years ago Closed 4 years ago

Move MEDIA_CAN_CREATE_XX_DECODER telemetry to off-main-thread

Categories

(Core :: Audio/Video: Playback, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

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

Attachments

(5 files, 1 obsolete file)

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.
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
Whiteboard: [qf][bhr]
Priority: -- → P1
Comment on attachment 8864746 [details]
Bug 1362212 - Remove canPlayTelemetry from _delayedStartup.

https://reviewboard.mozilla.org/r/136402/#review139560
Attachment #8864746 - Flags: review+
Attachment #8864746 - Flags: review?(mconley)
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+
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)
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)
Attachment #8864745 - Attachment is obsolete: true
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+
(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+
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 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)
(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 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+
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]
Depends on: 1371282
You need to log in before you can comment on or make changes to this bug.