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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

Attachments

(5 files, 1 obsolete file)

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.
Attachment #8438892 - Flags: review?(pkerr)
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 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 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+
Attachment #8438894 - Flags: review?(bugs) → review+
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+
Adds a limit to the capture size, and also correctly bumps to a new index when you close and reopen
Attachment #8439247 - Flags: review?(pkerr)
Attachment #8439248 - Flags: review?(pkerr)
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+
Attachment #8439248 - Flags: review?(pkerr) → review+
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
Attached patch simple bustage fix (obsolete) — Splinter Review
Simple warn-as-error bustage fix
Attachment #8439320 - Flags: review?(pkerr)
Attachment #8439320 - Attachment description: z → simple bustage fix
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)
Attachment #8439327 - Flags: review?(pkerr)
Attachment #8439327 - Flags: review?(pkerr) → review+
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?
(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)
(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 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+
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.