1112424 - Provide an about:media page with debug information about media elements and MSE

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[cajbir (:cajbir)]

It would be useful to have a place to view all active media elements in tabs along with detailed debug information about the internals of them for gecko debugging purposes.

Comment 1

[cajbir (:cajbir)]

Attachment: Initial stab at an implementation

Provides an about:media page that lists all media elements in all tabs along with debug information about it.

Comment 2

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

This patch adds methods to the MediaSource and HTMLMediaElement object to return the debug data as a string that gets displayed directly in the about:media page, inside a <pre> element. Patch 2 implements the MediaSource decoder methods popular the debug string with data. Patch 3 implements the about:media pages itself.

Comment 3

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 2 - Implement MediaSource methods to return debug information for about:media

Implements the debug methods for MediaSource that provide data to be displayed in the about:media page.

Comment 4

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 3 - Add an about:media page to show debug information about active HTML media elements

Adds about:media which shows internal debug data for HTMLMediaElement and MediaSource in particular. The intent is for this to be used by Gecko developers to help debug video playback and to be able to provide data to be copy/pasted by testers when problems arise. It calls internal JS APIs which are implemented in Part 1 and Part 2 of the patch.

I picked :gavin for review based on previous reviewing of about:webrtc patch. Please let me know if there is a more recommended reviewer.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8543744 [details] [diff] [review]
Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

We shouldn't be exposing these to the web at large, right?  I'd assume they should only be exposed to about:media or whatever other internals plan to use them.

Comment 6

[cajbir (:cajbir)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> We shouldn't be exposing these to the web at large, right?  I'd assume they
> should only be exposed to about:media or whatever other internals plan to
> use them.

Can you give me a hint on how to do that?

Comment 7

[Boris Zbarsky [:bzbarsky]]

That depends on who you want to expose the API to.  If about:media is running as system ("chrome") code, then you can probably just use https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#ChromeOnly

If you need to expose it to about:media but it's not running as system, then you probably want https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Func and then check the document URI of the window's current document.  You can use xpc::WindowGlobalOrNull to get a pointer to an nsPIDOMWindow from the global the function is called, assuming the global involved is a window at all.

Comment 8

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

Thanks for the info, adds [ChromeOnly] annotation as suggested in comment 7.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8544310 [details] [diff] [review]
Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

We generally put a linebreak after the [ChromeOnly] bit.

r=me with that.

Comment 10

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

Address review comment, adding newline to ChromeOnly, carry r+ forward. Thanks for the review.

Comment 11

[cajbir (:cajbir)]

Try build: https://tbpl.mozilla.org/?tree=Try&rev=19b030f8cacb

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to cajbir (:cajbir) from comment #0)
> It would be useful to have a place to view all active media elements in tabs
> along with detailed debug information about the internals of them for gecko
> debugging purposes.

And are there cases where this use case can't adequately be addressed by having the enumaration code packaged in an add-on rather than in Firefox?

I think we've generally been too liberal with our addition of these limited-use about: pages - this is not the first, but I'd like to slow or reverse that trend rather than continue it.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #12)
> And are there cases where this use case can't adequately be addressed by
> having the enumaration code packaged in an add-on rather than in Firefox?
> 
> I think we've generally been too liberal with our addition of these
> limited-use about: pages - this is not the first, but I'd like to slow or
> reverse that trend rather than continue it.

This sounds to me like you'd like to have a plan but don't. When we have a plan for doing something else then we can go along with that.

Comment 14

[Lily Kim]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #12)
I could see the reasoning for other Firefox versions, but I don't see why it should be an addon for Nightlies, considering Nightly is more meant for testing.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

My plan is "stop adding more, and then remove the existing ones".

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #15)
> My plan is "stop adding more, and then remove the existing ones".

That is not an especially enabling plan because we don't have any examples to follow and lack expertise in this area. I urgently need some help with this. Do you have someone available immediately to assist with creating an addon?

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Examples that do something similar as an addon:

https://github.com/darktrojan/zippy/blob/16ed1afe59d00713585e47dc853511d9ba7b391c/bootstrap.js#L94
https://github.com/darktrojan/zippy/blob/16ed1afe59d00713585e47dc853511d9ba7b391c/components/about-zippy.js
https://github.com/darktrojan/zippy/blob/16ed1afe59d00713585e47dc853511d9ba7b391c/chrome/content/zippy.xhtml

Comment 18

[cajbir (:cajbir)]

Comment on attachment 8543747 [details] [diff] [review]
Bug 1112424 Part 3 - Add an about:media page to show debug information about active HTML media elements

Based on review comment I think the plan forward here will be to land the DOM changes and do an extension for the about:media page. Clearing review request. Thanks gavin.

Comment 19

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 4 - Adds size of the decoder resource to the media page

Adding this here so I don't lose it. It adds the size of the media resource to the about:media output. Useful for debugging eviction so you can track how much data is in each decoder resource.

Comment 20

[cajbir (:cajbir)]

Comment on attachment 8547873 [details] [diff] [review]
Bug 1112424 Part 4 - Adds size of the decoder resource to the media page

Adds size of decoders useful for tracking eviction. I'll commit the DOM changes when reviewed so we can get the addon working.

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8543747 [details] [diff] [review]
Bug 1112424 Part 3 - Add an about:media page to show debug information about active HTML media elements

Jan 14 14:38:53 <gavin>	kentuckyfriedtakahe: I will submit some review comments to make the patch in the bug landable (some minor tweaks)

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I sent this to Anthony, but noting it here too: I packaged this page as an addon:

https://github.com/gavinsharp/aboutmedia

I'll submit review comments on the patch when I'm back at work on Monday (I was on PTO this past Wed-Fri, sorry about that).

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: modified part 3

I've implemented my review comments in the form of a modified patch since that seemed most expedient.

I added proper e10s support, simplified a bit of the code, and removed the inline script. Let me know if this works for you.

Comment 24

[cajbir (:cajbir)]

Is it missing aboutMedia.js?

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: modified part 3

Absolutely!

Comment 26

[cajbir (:cajbir)]

Comment on attachment 8551643 [details] [diff] [review]
modified part 3

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

Approach looks ok but it doesn't seem to work. Something to do with the new idl methods being chrome only maybe? I'll investigate.

Comment 27

[cajbir (:cajbir)]

It's not the idl methods, the code that was changed to:

  text += ms.mozDebugReaderData.split("\n").map(line => { "\t" + line + "\n" }).join("");

seems to be broken in some manner.

Comment 28

[cajbir (:cajbir)]

The joys of JS. No '{' or '}' should be used:

  text += ms.mozDebugReaderData.split("\n").map(line => "\t" + line + "\n").join("");

Comment 29

[cajbir (:cajbir)]

Attachment: Modified part 3

Fixes the JS to work in part 3.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8552863 [details] [diff] [review]
Modified part 3

Sorry about that, must have been a late edit. The code should be:

text += ms.mozDebugReaderData.split("\n").map(line => { return "\t" + line + "\n"; }).join("");

(see bug 1083459)

r=me with that change.

Anthony and I agreed that we'd only land this on 36/37, I think, so assuming the plan is to land the platform patches on those branches too, let's avoid landing this patch on mozilla-central.

Comment 31

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

Rebase, carry forward r+.

Comment 32

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 2 - Implement MediaSource methods to return debug information for about:media

Rebase, carry forward r+.

Comment 33

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 3 - Adds size of the decoder resource to the media page

Rebase, carry forward r+. Renumbered to "Part 3". This was previously "Part 4". Parts 1-3 will be landed on mozilla-central and uplifted. Part 4 will not land on mozilla-central but will land on 36/37 as requested in comment 30.

Comment 34

[cajbir (:cajbir)]

Attachment: Bug 1112424 Part 4 - Add an about:media page to show debug information about active HTML media elements

Rebased, carry forward r+. This was previous "Part 3" and now renumbered to "Part 4" as it is landing last, and not on mozilla-central.

Comment 35

[cajbir (:cajbir)]

Try for parts 1-3: https://tbpl.mozilla.org/?tree=Try&rev=a721d2321ebc
Try for parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=dee2b340f06e

Comment 36

[cajbir (:cajbir)]

Parts 1-3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdc5ad31d58
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cca59cdfc0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbd365e86cc

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cdc5ad31d58
https://hg.mozilla.org/mozilla-central/rev/4cca59cdfc0d
https://hg.mozilla.org/mozilla-central/rev/1cbd365e86cc

Comment 38

[Brad Jackson]

Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1128161

Comment 39

[Ralph Giles (:rillian)]

Comment on attachment 8556235 [details] [diff] [review]
Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Harder to report video playback issues with debugging information. Difficultly backporting future fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This provides debugging code for the about:media page. It does touch HTMLMediaElement, but only to add new chrome-only interfaces. Risk is low.
[String/UUID change made/needed]: None.

Comment 40

[Ralph Giles (:rillian)]

Uplift request is for all patches on this bug that have landed on m-c.

Comment 41

[Ralph Giles (:rillian)]

Comment on attachment 8556239 [details] [diff] [review]
Bug 1112424 Part 4 - Add an about:media page to show debug information about active HTML media elements

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Harder to file useful bugs for Youtube playback issues.
[Describe test coverage new/current, TreeHerder]: Local testing only.
[Risks and why]: Adds a new debug page. Risk should be low.
[String/UUID change made/needed]: No uuid changes. I don't know if this counts as a string change. We're ok with it not being localized.

Comment 42

[Ralph Giles (:rillian)]

Comment on attachment 8556239 [details] [diff] [review]
Bug 1112424 Part 4 - Add an about:media page to show debug information about active HTML media elements

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Harder to report useful information about Youtube playback bugs.
[Describe test coverage new/current, TreeHerder]: local testing only.
[Risks and why]: This adds a new debug info page at about:media. Risk should be low.
[String/UUID change made/needed]: No uuid changes. I don't know if about: pages count as string changes, but we're ok with this not being localized.

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

Axel, Flod - Are you OK with the additional of this English only about: page?

Comment 44

[Francesco Lodolo [:flod]]

It would be much easier to answer if I could actually see the page: I tried opening about:media in Nightly but it's not a valid URI (did patch4 land anywhere? I can't recognize a link to inbound or m-c).

Based on fourth patch I see 3 strings (currentTime:, start=, end=). I guess I'd be fine with this being in English for a few cycles, as long as we make it properly localizable on Nightly.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ac3263fdc2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c84af5a71ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/1677586c2c9e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f233a6863e6

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cba5f9bfe66c
https://hg.mozilla.org/releases/mozilla-beta/rev/763b9cbd7164
https://hg.mozilla.org/releases/mozilla-beta/rev/68c3b8df1065
https://hg.mozilla.org/releases/mozilla-beta/rev/23b7a843e75d

Comment 47

[Ralph Giles (:rillian)]

(In reply to Francesco Lodolo [:flod] from comment #44)
> It would be much easier to answer if I could actually see the page: I tried
> opening about:media in Nightly but it's not a valid URI (did patch4 land
> anywhere? I can't recognize a link to inbound or m-c).

Gavin didn't want this to be a permanent feature, so the about:media page itself is only in Firefox 36 and 37. For Nightly the idea is to provide the page as an addon, but it doesn't seem to be packaged yet. See comment #22.

Comment 48

[Ralph Giles (:rillian)]

For nightly you can install gavin's draft addon from https://people.mozilla.org/~rgiles/2015/aboutmedia.xpi It only shows content while there's a video open in another tab.

Comment 49

[Nathan Toone [:toonetown]]

This breaks the build if you have --disable-debug set, because DumpTimeRanges is only built when PR_LOGGING is defined.

Since this got uplifted, I actually noticed it break when I was building the aurora branch.

Comment 50

[Nathan Toone [:toonetown]]

Specifically, it is the calls to DumpTimeRanges within MediaSourceReader::GetMozDebugReaderData - the other calls do not cause the issue, due to their being wrapped in a the MSE_DEBUGV macro

Comment 51

[cajbir (:cajbir)]

(In reply to Nathan Toone from comment #50)
> Specifically, it is the calls to DumpTimeRanges within
> MediaSourceReader::GetMozDebugReaderData - the other calls do not cause the
> issue, due to their being wrapped in a the MSE_DEBUGV macro

See bug 1128161 which fixes that.

Comment 52

[cajbir (:cajbir)]

(In reply to Francesco Lodolo [:flod] from comment #44)
> Based on fourth patch I see 3 strings (currentTime:, start=, end=). I guess
> I'd be fine with this being in English for a few cycles, as long as we make
> it properly localizable on Nightly.

These strings match the W3 API names and I would think they wouldn't need to be localized would they? It'd make it hard to match up with the API if local user copy/pastes the report data to a developer who then has to translate them to an API name.

Comment 53

[cajbir (:cajbir)]

(In reply to Ralph Giles (:rillian) from comment #47)
> Gavin didn't want this to be a permanent feature, so the about:media page
> itself is only in Firefox 36 and 37. For Nightly the idea is to provide the
> page as an addon, but it doesn't seem to be packaged yet. See comment #22.

The XPI is in the github repository quoted in comment 22:

https://github.com/gavinsharp/aboutmedia

Comment 54

[Nathan Toone [:toonetown]]

I added bug 1129259 to track the failure to compile issue

Comment 55

[Nathan Toone [:toonetown]]

(In reply to Nathan Toone from comment #54)
> I added bug 1129259 to track the failure to compile issue

(In reply to cajbir (:cajbir) from comment #51)
> (In reply to Nathan Toone from comment #50)
> > Specifically, it is the calls to DumpTimeRanges within
> > MediaSourceReader::GetMozDebugReaderData - the other calls do not cause the
> > issue, due to their being wrapped in a the MSE_DEBUGV macro
> 
> See bug 1128161 which fixes that.

Oops - sorry.  I didn't notice the duplicate bug until just now :(

Comment 56

[Francesco Lodolo [:flod]]

(In reply to cajbir (:cajbir) from comment #52)
> These strings match the W3 API names and I would think they wouldn't need to
> be localized would they? It'd make it hard to match up with the API if local
> user copy/pastes the report data to a developer who then has to translate
> them to an API name.

It all depends on how much visibility this page will have. For example, if it's going to have other text (e.g. a title, a description of what the page does), it makes sense to have something localizable.

> Current time (currentTime):
> Start (start):
> End (end):

It won't also be the first non-localizable about page (about:cache is one).