Show codec used in logs

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Currently, most MSE debug log just show the address of the MS component.

Makes it rather difficult to debut what is what.

Would be nice if it showed the codec as well.

so you would get something like: reader[video/mp4](12345678)::Blah log
(Assignee)

Comment 1

4 years ago
Created attachment 8562004 [details] [diff] [review]
Show codec/container type in MSE logs

Rationalize MSE logs, they all have the same format now: ClassName::FunctionName(%p): blah, and if relevant to the object, will show the types (e.g. video/mp4, audio/mp4 etc)... This makes for easier debugging.
Attachment #8562004 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED

Comment 2

4 years ago
Comment on attachment 8562004 [details] [diff] [review]
Show codec/container type in MSE logs

Please post a link to a full try build and re-request review.
Attachment #8562004 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 4

4 years ago
The build failures are related to a stale earlier patch, and nothing to do with that one..

here it is again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fc1cafe10c5
(Assignee)

Comment 5

4 years ago
Created attachment 8562447 [details] [diff] [review]
Show codec/container type in MSE logs

Add polyfill to make it compile with MSVC (__func__ doesn't exist) and explicit constructor in ContainerParser
Attachment #8562447 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8562004 - Attachment is obsolete: true

Comment 6

4 years ago
Is there a try build with these latest changes?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 7

4 years ago
cajbir, you have so little faith :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ba7e2ecd08
Flags: needinfo?(jyavenard)

Updated

4 years ago
Attachment #8562447 - Flags: review?(cajbir.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/715a00dcf5c0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-firefox37: --- → affected
This is just logging changes, but doesn't apply cleanly to 37 aurora currently. If you'd like to do a port to 37, that would be great, otherwise I'll skip this.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 11

4 years ago
not porting this will make any changes made after a pain to backport.

Would be interesting to see what doesn't cleanly apply, as it more than likely indicates a missed backport.
Flags: needinfo?(jyavenard)
Or something got ported out of order. Anyway, yes it is making for something of a pain, but I was trying to get to the P1 bugs before tomorrow, and don't have time to trace everything before that. Feel free to have a look.
(Assignee)

Comment 13

4 years ago
I'm on my way back to Oz. I will craft a backported for aurora tomorrow morning. If you want it sooner let me know, and I'll try to get to it when I return home in about 4 hours
Flags: needinfo?(giles)
Tomorrow morning means it will miss 37b1, but I think that's fine. We have enough stuff to wrangle in the meantime, so don't worry about doing it tonight.
Flags: needinfo?(giles)
(Assignee)

Comment 15

4 years ago
I'm a bit confused...

this changeset appears to be in aurora already. I diffed the two repos, and everything is in aurora now there's no difference between central and aurora when it comes to mediasource.
Flags: needinfo?(giles)
Created attachment 8569462 [details] [diff] [review]
Backport to 37

Perhaps you looked after aurora became 38?

Here's a patch with everything which looked relevant from the diff of dom/media/mediasource between today's 37 and 38 branches.
Flags: needinfo?(giles)
Attachment #8569462 - Flags: review?(jyavenard)
Created attachment 8569472 [details] [diff] [review]
Backport to 37 v2

Add some missing bits.
Attachment #8569462 - Attachment is obsolete: true
Attachment #8569462 - Flags: review?(jyavenard)
Attachment #8569472 - Flags: review?(jyavenard)
Created attachment 8569480 [details] [diff] [review]
Backport to 37 v3

And remove some extra bits.
Attachment #8569472 - Attachment is obsolete: true
Attachment #8569472 - Flags: review?(jyavenard)
Attachment #8569480 - Flags: review?(jyavenard)
(Assignee)

Comment 19

4 years ago
Comment on attachment 8569480 [details] [diff] [review]
Backport to 37 v3

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

I did a diff between the patch I had and yours and found discrepancy. Found a missing #undef in mine while at it :)

you have integrated some stray changes in some of the code, which weren't backported: Bug 1126499
also some lines were shifted around in SourceBufferList.cpp
Attachment #8569480 - Flags: review?(jyavenard) → review-
(Assignee)

Comment 20

4 years ago
Created attachment 8569612 [details] [diff] [review]
Show codec/container type in MSE logs 37 backport

backport to 37
(In reply to Jean-Yves Avenard [:jya] from comment #19)

> I did a diff between the patch I had and yours and found discrepancy. Found
> a missing #undef in mine while at it :)

Yay!

> you have integrated some stray changes in some of the code, which weren't
> backported: Bug 1126499

Ah, you're right. Missed removing that one.
Comment on attachment 8569612 [details] [diff] [review]
Show codec/container type in MSE logs 37 backport

Requesting 37 uplift of this change.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Harder to diagnose issues, difficultly backporting further changes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Risk is very low. This changes MSE-specific code to improve debug logging. It touches a lot of code, so having this in 37 will simplify applying further fixes as well as improving the usefulness of logging for diagnosing new issues. But it's just logging and information propagation; there's no behavioural change.
[String/UUID change made/needed]: None.
Attachment #8569612 - Attachment description: Show codec/container type in MSE logs → Show codec/container type in MSE logs 37 backport
Attachment #8569612 - Flags: approval-mozilla-beta?
Attachment #8569480 - Attachment is obsolete: true
Comment on attachment 8569612 [details] [diff] [review]
Show codec/container type in MSE logs 37 backport

Taking this patch, which primarily contains logging changes. Going forward, I would like to restrict the speculative patches that we take (to support potential future landings) as was it was indicated the few additional changes in this patch are for. Beta+
Attachment #8569612 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Wes Kocher (:KWierso) from comment #26)
> Further fixes in
> https://hg.mozilla.org/releases/mozilla-beta/rev/20ea789e69df

Looks like this worked but I'm not near my computer that has all of the trees cloned, so someone else can push the further fixes around to other trees as needed.
(Assignee)

Comment 31

4 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/346743dfd466

This change will apply cleanly on both aurora and beta.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.