Closed
Bug 1024288
Opened 10 years ago
Closed 10 years ago
Add button to about:webrtc to start/stop AEC debug capture
Categories
(Core :: WebRTC: Audio/Video, defect)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: jesup, Assigned: jesup)
Details
Attachments
(5 files, 1 obsolete file)
15.38 KB,
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
7.18 KB,
patch
|
jib
:
review+
smaug
:
review+
Unfocused
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
11.40 KB,
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
2.06 KB,
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
Add button to about:webrtc to start/stop AEC debug capture. Preferably this would work even in the middle of an ongoing call. Also allow captures to be made by setting an NSPR logging value. We should check if this has any performance impact when turned off; if so consider turning it off in production, or using different functions instead of inline conditionals. Likely the impact is negligible.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8438892 -
Flags: review?(pkerr)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8438894 [details] [diff] [review] Add a button to about:webrtc to turn on/off AEC logging r? to jib, smaug and unfocused
Attachment #8438894 -
Flags: review?(jib)
Attachment #8438894 -
Flags: review?(bugs)
Attachment #8438894 -
Flags: review?(bmcbride)
Comment 4•10 years ago
|
||
Comment on attachment 8438894 [details] [diff] [review] Add a button to about:webrtc to turn on/off AEC logging Review of attachment 8438894 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the toolkit changes. ::: toolkit/content/aboutWebrtc.xhtml @@ +379,5 @@ > } > + if (WebrtcGlobalInformation.aecDebug) { > + setAECDebugButton(true); > + } else { > + setAECDebugButton(false); Could shorten this to just: setAECDebugButton(WebrtcGlobalInformation.aecDebug);
Attachment #8438894 -
Flags: review?(bmcbride) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8438894 [details] [diff] [review] Add a button to about:webrtc to turn on/off AEC logging Review of attachment 8438894 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, but do we need a separate button?
Attachment #8438894 -
Flags: review?(jib) → review+
Updated•10 years ago
|
Attachment #8438894 -
Flags: review?(bugs) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8438892 [details] [diff] [review] Allow aec debug data to be dumped on the fly Review of attachment 8438892 [details] [diff] [review]: ----------------------------------------------------------------- Most of the new debug code in aec_core.c and echo_cancellation.c is the same except for the file and structure names. It may save pain later to create some common functions, especially to do the open and close of the files sets.
Attachment #8438892 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Adds a limit to the capture size, and also correctly bumps to a new index when you close and reopen
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8439247 -
Flags: review?(pkerr)
Assignee | ||
Updated•10 years ago
|
Attachment #8439248 -
Flags: review?(pkerr)
Comment 9•10 years ago
|
||
Comment on attachment 8439247 [details] [diff] [review] interdiffs to allow max size for AEC logs, and fix multiple captures Review of attachment 8439247 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/audio_processing/aec/aec_core.c @@ +850,5 @@ > + if (aec->farFile) { > + (void)fwrite(farend_ptr, sizeof(int16_t), PART_LEN, aec->farFile); > + (void)fwrite(nearend_ptr, sizeof(int16_t), PART_LEN, aec->nearFile); > + aec->debugWritten += sizeof(int16_t) * PART_LEN; > + if (aec->debugWritten >= AECDebugMaxSize()) { Is it OK that the amount actually written can at most PART_LEN -1 over AECDebugMaxSize()? Since this is a file and not a fixed memory buffer I am assuming this is OK.
Attachment #8439247 -
Flags: review?(pkerr) → review+
Updated•10 years ago
|
Attachment #8439248 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b4feb3d3a39 https://hg.mozilla.org/integration/mozilla-inbound/rev/5d63a1316981 Adding webrtc-uplift since this was wanted badly for 32 for partners, who rarely have users willing to run nightlies, and this only affects a status/debug page.
Whiteboard: [webrtc-uplift]
Target Milestone: --- → mozilla33
Comment 11•10 years ago
|
||
Backed out for build failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=41608422&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=41608448&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eca793325b9d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7790b3c00e3a
Assignee | ||
Comment 12•10 years ago
|
||
Simple warn-as-error bustage fix
Attachment #8439320 -
Flags: review?(pkerr)
Assignee | ||
Updated•10 years ago
|
Attachment #8439320 -
Attachment description: z → simple bustage fix
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8439320 [details] [diff] [review] simple bustage fix Pulling to fix one other warning
Attachment #8439320 -
Attachment is obsolete: true
Attachment #8439320 -
Flags: review?(pkerr)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8439327 -
Flags: review?(pkerr)
Updated•10 years ago
|
Attachment #8439327 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 15•10 years ago
|
||
With warning/etc fixes https://hg.mozilla.org/integration/mozilla-inbound/rev/71eb309d3e77 https://hg.mozilla.org/integration/mozilla-inbound/rev/33aad7e2e1c3
https://hg.mozilla.org/mozilla-central/rev/71eb309d3e77 https://hg.mozilla.org/mozilla-central/rev/33aad7e2e1c3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8438894 [details] [diff] [review] Add a button to about:webrtc to turn on/off AEC logging Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: When have problems with echo in the field, without this it's basically impossible to get useful debugging as to why. And it's almost impossible to get most users of Webrtc services (like TokBox) to use anything newer than Beta, let alone a special debug build. [Describe test coverage new/current, TBPL]: N/A. Used by our devs with good success (along with the followon bug 1025176 that standardizes the destination paths; we'd want both to be uplifted together) [Risks and why]: Low risk: on a debugging page that's not highlighted unless we or someone else asks them to go there; then requires pushing a button. It automatically turns off after a couple of minutes to avoid chewing disk space. Modification to existing logging code to allow it to be triggered on the fly. [String/UUID change made/needed]: Adds a button which likely does not need any priority for localization. (That can be a follow-on) Typically we tell users "press this, then send us the files from path X".
Attachment #8438894 -
Flags: approval-mozilla-aurora?
Comment 18•10 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #17) The patch looks safe enough and I'm inclined to take this. I have three questions before marking aurora+: 1. about:webrtc appears to use all hard coded strings. Is this page localized? 2. Is about:webrtc linked anywhere in the product? 3. Is this fix desktop specific or do you expect this page to be used on fennec or b2g as well?
Flags: needinfo?(rjesup)
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #18) > (In reply to Randell Jesup [:jesup] from comment #17) > The patch looks safe enough and I'm inclined to take this. I have three > questions before marking aurora+: > > 1. about:webrtc appears to use all hard coded strings. Is this page > localized? No, not currently. The entire page needs serious webdev love; it's effectively a page to get debugging info we can't otherwise get. Few if any users will understand or be able to act on anything there without a fair bit more use - and then it will almost solely be developers building webrtc apps. For users, it's a "go here, hit this button, paste it into a report" page. > 2. Is about:webrtc linked anywhere in the product? No. > 3. Is this fix desktop specific or do you expect this page to be used on > fennec or b2g as well? We have no about: pages on b2g. It will be visible on android, though there you must set the pref to a saveable location before using this aec dump button. (so far as I know there's no universal good place to save to). Thanks!
Flags: needinfo?(rjesup)
Comment 20•10 years ago
|
||
Comment on attachment 8438894 [details] [diff] [review] Add a button to about:webrtc to turn on/off AEC logging Aurora+ based on the facts that this patch is largely to an about: page with no l10n impact with small modifications to logging code and provides a useful way to debug in the field.
Attachment #8438894 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f0efff0f075 https://hg.mozilla.org/releases/mozilla-aurora/rev/5ddb3118d9ae
Assignee | ||
Updated•10 years ago
|
Whiteboard: [webrtc-uplift]
You need to log in
before you can comment on or make changes to this bug.
Description
•