Add button to about:webrtc to start/stop AEC debug capture

RESOLVED FIXED in Firefox 32

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 8438892 [details] [diff] [review]
Allow aec debug data to be dumped on the fly
(Assignee)

Comment 2

4 years ago
Created attachment 8438894 [details] [diff] [review]
Add a button to about:webrtc to turn on/off AEC logging
(Assignee)

Updated

4 years ago
Attachment #8438892 - Flags: review?(pkerr)
(Assignee)

Comment 3

4 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 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+

Comment 6

4 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

4 years ago
Created attachment 8439247 [details] [diff] [review]
interdiffs to allow max size for AEC logs, and fix multiple captures

Adds a limit to the capture size, and also correctly bumps to a new index when you close and reopen
(Assignee)

Comment 8

4 years ago
Created attachment 8439248 [details] [diff] [review]
Read max aec dump size
(Assignee)

Updated

4 years ago
Attachment #8439247 - Flags: review?(pkerr)
(Assignee)

Updated

4 years ago
Attachment #8439248 - Flags: review?(pkerr)

Comment 9

4 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

4 years ago
Attachment #8439248 - Flags: review?(pkerr) → review+
(Assignee)

Comment 10

4 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
(Assignee)

Comment 12

4 years ago
Created attachment 8439320 [details] [diff] [review]
simple bustage fix

Simple warn-as-error bustage fix
Attachment #8439320 - Flags: review?(pkerr)
(Assignee)

Updated

4 years ago
Attachment #8439320 - Attachment description: z → simple bustage fix
(Assignee)

Comment 13

4 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

4 years ago
Created attachment 8439327 [details] [diff] [review]
bustage/warning fix
Attachment #8439327 - Flags: review?(pkerr)

Updated

4 years ago
Attachment #8439327 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/mozilla-central/rev/71eb309d3e77
https://hg.mozilla.org/mozilla-central/rev/33aad7e2e1c3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

4 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?
(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

4 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 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

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f0efff0f075
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ddb3118d9ae
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
(Assignee)

Updated

4 years ago
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.