Closed Bug 1131433 Opened 10 years ago Closed 10 years ago

Show codec used in logs

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Show codec/container type in MSE logs
95.16 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
Show codec/container type in MSE logs 37 backport
90.54 KB, patch
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review
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
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: nobody → jyavenard
Status: NEW → ASSIGNED
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)
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
Add polyfill to make it compile with MSVC (__func__ doesn't exist) and explicit constructor in ContainerParser
Attachment #8562447 - Flags: review?(cajbir.bugzilla)
Attachment #8562004 - Attachment is obsolete: true
Is there a try build with these latest changes?
Flags: needinfo?(jyavenard)
cajbir, you have so little faith :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=40ba7e2ecd08
Flags: needinfo?(jyavenard)
Attachment #8562447 - Flags: review?(cajbir.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/715a00dcf5c0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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)
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.
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)
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)
Attached patch Backport to 37 (obsolete) — — Splinter Review
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)
Attached patch Backport to 37 v2 (obsolete) — — Splinter Review
Add some missing bits.
Attachment #8569462 - Attachment is obsolete: true
Attachment #8569462 - Flags: review?(jyavenard)
Attachment #8569472 - Flags: review?(jyavenard)
Attached patch Backport to 37 v3 (obsolete) — — Splinter Review
And remove some extra bits.
Attachment #8569472 - Attachment is obsolete: true
Attachment #8569472 - Flags: review?(jyavenard)
Attachment #8569480 - Flags: review?(jyavenard)
Blocks: 1134064
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-
(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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: