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

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

(Blocks: 1 bug)

33 Branch
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 unaffected, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(4 attachments, 10 obsolete attachments)

5.01 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
5.66 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
2.50 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
8.21 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 778617
(Assignee)

Comment 1

3 years ago
Created attachment 8537567 [details] [diff] [review]
Initial stab at an implementation

Provides an about:media page that lists all media elements in all tabs along with debug information about it.
Assignee: nobody → cajbir.bugzilla
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 2

3 years ago
Created attachment 8543744 [details] [diff] [review]
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.
Attachment #8537567 - Attachment is obsolete: true
Attachment #8543744 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

3 years ago
Created attachment 8543746 [details] [diff] [review]
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.
Attachment #8543746 - Flags: review?(ajones)
(Assignee)

Comment 4

3 years ago
Created attachment 8543747 [details] [diff] [review]
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.
Attachment #8543747 - Flags: review?(gavin.sharp)
Attachment #8543746 - Flags: review?(ajones) → review+
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.
Attachment #8543744 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 6

3 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

3 years ago
Created attachment 8544310 [details] [diff] [review]
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.
Attachment #8543744 - Attachment is obsolete: true
Attachment #8544310 - Flags: review?(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.
Attachment #8544310 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8544320 [details] [diff] [review]
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.
Attachment #8544310 - Attachment is obsolete: true
Attachment #8544320 - Flags: review+
(Assignee)

Comment 11

3 years ago
Try build: https://tbpl.mozilla.org/?tree=Try&rev=19b030f8cacb
(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.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
My plan is "stop adding more, and then remove the existing ones".
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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?
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
(Assignee)

Comment 18

3 years ago
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.
Attachment #8543747 - Flags: review?(gavin.sharp)
(Assignee)

Comment 19

3 years ago
Created attachment 8547873 [details] [diff] [review]
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.
(Assignee)

Comment 20

3 years ago
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.
Attachment #8547873 - Flags: review?(ajones)
Attachment #8547873 - Flags: review?(ajones) → review+
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)
Attachment #8543747 - Flags: review?(gavin.sharp)
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).
Created attachment 8551547 [details] [diff] [review]
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.
Attachment #8543747 - Attachment is obsolete: true
Attachment #8543747 - Flags: review?(gavin.sharp)
Attachment #8551547 - Flags: feedback?(cajbir.bugzilla)
Attachment #8551547 - Flags: feedback?(ajones)
(Assignee)

Comment 24

3 years ago
Is it missing aboutMedia.js?
Flags: needinfo?(gavin.sharp)
Created attachment 8551643 [details] [diff] [review]
modified part 3

Absolutely!
Attachment #8551547 - Attachment is obsolete: true
Attachment #8551547 - Flags: feedback?(cajbir.bugzilla)
Attachment #8551547 - Flags: feedback?(ajones)
Flags: needinfo?(gavin.sharp)
Attachment #8551643 - Flags: feedback?(cajbir.bugzilla)
Attachment #8551643 - Flags: feedback?(ajones)
(Assignee)

Comment 26

3 years ago
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.
Attachment #8551643 - Flags: feedback?(cajbir.bugzilla) → feedback+
(Assignee)

Comment 27

3 years ago
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.
(Assignee)

Comment 28

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

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

Comment 29

3 years ago
Created attachment 8552863 [details] [diff] [review]
Modified part 3

Fixes the JS to work in part 3.
Attachment #8551643 - Attachment is obsolete: true
Attachment #8551643 - Flags: feedback?(ajones)
Attachment #8552863 - Flags: review?(gavin.sharp)
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.
Attachment #8552863 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 31

3 years ago
Created attachment 8556235 [details] [diff] [review]
Bug 1112424 Part 1 - Add moz specific methods to retrieve debug data to media object IDL

Rebase, carry forward r+.
Attachment #8544320 - Attachment is obsolete: true
Attachment #8556235 - Flags: review+
(Assignee)

Comment 32

3 years ago
Created attachment 8556236 [details] [diff] [review]
Bug 1112424 Part 2 - Implement MediaSource methods to return debug information for about:media

Rebase, carry forward r+.
Attachment #8543746 - Attachment is obsolete: true
Attachment #8556236 - Flags: review+
(Assignee)

Comment 33

3 years ago
Created attachment 8556237 [details] [diff] [review]
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.
Attachment #8547873 - Attachment is obsolete: true
Attachment #8556237 - Flags: review+
(Assignee)

Comment 34

3 years ago
Created attachment 8556239 [details] [diff] [review]
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.
Attachment #8552863 - Attachment is obsolete: true
Attachment #8556239 - Flags: review+
(Assignee)

Comment 35

3 years ago
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
(Assignee)

Comment 36

3 years ago
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
Keywords: leave-open

Updated

3 years ago
Depends on: 1127674
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

3 years ago
Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1128161
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.
Attachment #8556235 - Flags: approval-mozilla-beta?
Attachment #8556235 - Flags: approval-mozilla-aurora?
Uplift request is for all patches on this bug that have landed on m-c.
status-firefox35: --- → unaffected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8556236 - Flags: approval-mozilla-beta+
Attachment #8556236 - Flags: approval-mozilla-aurora+
Attachment #8556237 - Flags: approval-mozilla-beta+
Attachment #8556237 - Flags: approval-mozilla-aurora+
Attachment #8556235 - Flags: approval-mozilla-beta?
Attachment #8556235 - Flags: approval-mozilla-beta+
Attachment #8556235 - Flags: approval-mozilla-aurora?
Attachment #8556235 - Flags: approval-mozilla-aurora+
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.
Attachment #8556239 - Flags: approval-mozilla-aurora?
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.
Attachment #8556239 - Flags: approval-mozilla-beta?
Axel, Flod - Are you OK with the additional of this English only about: page?
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
Attachment #8556239 - Flags: approval-mozilla-beta?
Attachment #8556239 - Flags: approval-mozilla-beta+
Attachment #8556239 - Flags: approval-mozilla-aurora?
Attachment #8556239 - Flags: approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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
status-firefox37: affected → fixed
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
status-firefox36: affected → fixed
Flags: needinfo?(l10n)
(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.
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.
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.
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
(Assignee)

Comment 51

3 years ago
(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.
(Assignee)

Comment 52

3 years ago
(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.
(Assignee)

Comment 53

3 years ago
(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
Depends on: 1129259
I added bug 1129259 to track the failure to compile issue
(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 :(
(Assignee)

Updated

3 years ago
Blocks: 1128161
(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).
You need to log in before you can comment on or make changes to this bug.