Closed Bug 1256430 Opened 4 years ago Closed 4 years ago

about:webrtc button Start AEC Logging does not ensure that AEC logging is configured

Categories

(Core :: WebRTC, defect, P1)

All
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- affected
firefox47 --- affected
firefox48 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- affected

People

(Reporter: ng, Assigned: ng)

Details

Attachments

(1 file, 6 obsolete files)

If the preference media.webrtc.debug.aec_log_dir is not set, the Start AEC Logging button will not trigger a default configuration being set. Currently these defaults are set in WebRtcLog::ConfigWebRtcLog when the about:webrtc button "Start Debug Mode" is pressed. This is a symptom of not having registered defaults for the webrtc logging settings.
Do we need to uplift this?  I guess yes; can you set the appropriate tracking flags to "affected" (or wontfix for 45 for example).  We might take it on 45esr though, though probably not (not set or stability)
Rank: 17
Flags: needinfo?(na-g)
Priority: -- → P1
Nico -- Can you take this?
Assignee: nobody → na-g
(In reply to Randell Jesup [:jesup] from comment #1)
> Do we need to uplift this?  I guess yes; can you set the appropriate
> tracking flags to "affected" (or wontfix for 45 for example).  We might take
> it on 45esr though, though probably not (not set or stability)
Should I set the blocking flag "?" on 46?
With regards to 45esr, I don't see "stability" where is that hiding?
Flags: needinfo?(na-g) → needinfo?(rjesup)
(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #2)
> Nico -- Can you take this?

Sure.
No blocking flags (and no tracking-<release> flags either)
Rank: 17 → 15
(In reply to Nico Grunbaum [:ng] from comment #0)
> If the preference media.webrtc.debug.aec_log_dir is not set, the Start AEC
> Logging button will not trigger a default configuration being set. Currently
> these defaults are set in WebRtcLog::ConfigWebRtcLog when the about:webrtc
> button "Start Debug Mode" is pressed. This is a symptom of not having
> registered defaults for the webrtc logging settings.

In the previous code base, a call was made to StartWebRtcLog in order to trigger the setup of the AEC log location configuration.

see: https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp#681
I have four review flags for what looks like an evolution of a patch to the AEC log configuration. It looks like that last one is the one you plan to land. Is this correct?
Flags: needinfo?(na-g)
(In reply to Paul Kerr [:pkerr] from comment #12)
> I have four review flags for what looks like an evolution of a patch to the
> AEC log configuration. It looks like that last one is the one you plan to
> land. Is this correct?

Correct, I can squash it to a single commit if that is SOP.
Flags: needinfo?(na-g) → needinfo?(pkerr)
(In reply to Nico Grunbaum [:ng] from comment #13)
> (In reply to Paul Kerr [:pkerr] from comment #12)
> > I have four review flags for what looks like an evolution of a patch to the
> > AEC log configuration. It looks like that last one is the one you plan to
> > land. Is this correct?
> 
> Correct, I can squash it to a single commit if that is SOP.

In fact I would rather submit it as a squashed patch. This was my first time submitting a non-squashed patch, and I wanted to see what it would do, and if I had been wasting time by doing the squashing myself. It is not what I had hoped for.
Attachment #8732230 - Attachment is obsolete: true
Attachment #8732230 - Flags: review?(pkerr)
Attachment #8732231 - Attachment is obsolete: true
Attachment #8732231 - Flags: review?(pkerr)
Attachment #8732228 - Attachment is obsolete: true
Attachment #8732228 - Flags: review?(pkerr)
Attachment #8732229 - Attachment is obsolete: true
Attachment #8732229 - Flags: review?(pkerr)
Comment on attachment 8732228 [details]
MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41045/diff/1-2/
Attachment #8732228 - Attachment description: MozReview Request: Start moving AEC logging control into its own functions. → MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr
Attachment #8732228 - Attachment is obsolete: false
Attachment #8732228 - Flags: review?(pkerr)
Comment on attachment 8732228 [details]
MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr

https://reviewboard.mozilla.org/r/41045/#review38323
Attachment #8732228 - Flags: review?(pkerr) → review+
Flags: needinfo?(pkerr)
Attachment #8732228 - Attachment is patch: true
Attachment #8732228 - Attachment mime type: text/x-review-board-request → text/plain
Attachment #8732228 - Attachment is patch: false
Attached patch Same patch as in the mozreview (obsolete) — Splinter Review
Attachment #8732228 - Attachment is obsolete: true
Attachment #8734845 - Flags: review+
Correcting the Android build breakage.
Attachment #8734845 - Attachment is obsolete: true
Flags: needinfo?(na-g)
Attachment #8734975 - Flags: review?(pkerr)
Comment on attachment 8734975 [details] [diff] [review]
review-bug1256430-update-1.git.patch

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

r+, with nit fix

::: media/webrtc/signaling/src/common/browser_logging/WebRtcLog.cpp
@@ -112,5 @@
>  
>  #if defined(ANDROID)
>    // Special case: use callback to pipe to NSPR logging.
>    aLogFile.Assign(default_log_name);
> -  // For AEC, do not use a default value: force the user to specify a directory.

please don't patch the same file multiple times in a single patch; splinter can't handle it and will only show the last patch unless you use "All Files"

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +680,5 @@
>  
>  void
>  WebrtcGlobalInformation::SetAecDebug(const GlobalObject& aGlobal, bool aEnable)
>  {
> +  if( aEnable ) {

nit: if (aEnable) {

@@ +884,5 @@
>  bool
>  WebrtcGlobalChild::RecvSetAecLogging(const bool& aEnable)
>  {
>    if (!mShutdown) {
> +    if( aEnable ) {

ditto
Attachment #8734975 - Flags: review?(pkerr) → review+
nits fixed, commits squished
Attachment #8734975 - Attachment is obsolete: true
Attachment #8735681 - Flags: review?(rjesup)
Attachment #8735681 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/0ff7a5a4b24f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.