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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 2

[Maire Reavy [:mreavy]]

Nico -- Can you take this?

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #2)
> Nico -- Can you take this?

Sure.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

No blocking flags (and no tracking-<release> flags either)

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faec5b6a30e3

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr

Review commit: https://reviewboard.mozilla.org/r/41045/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41045/

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: Switching WebrtcGlobalInformation/Child to using Start/StopAecLog.

Review commit: https://reviewboard.mozilla.org/r/41047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41047/

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr

Review commit: https://reviewboard.mozilla.org/r/41049/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41049/

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: bug-1256430 start AEC log independently of webrtc TRACE; r?pkerr

minor edit, removing debug printf

Review commit: https://reviewboard.mozilla.org/r/41051/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41051/

Comment 11

[Paul Kerr [:pkerr]]

(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

Comment 12

[Paul Kerr [:pkerr]]

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?

Comment 13

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(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.

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(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.

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=541cbcff6d75

Comment 16

[Nico Grunbaum [:ng, @chew:mozilla.org]]

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/

Comment 17

[Paul Kerr [: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

Comment 18

[Paul Kerr [:pkerr]]

https://reviewboard.mozilla.org/r/41045/#review38969

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Same patch as in the mozreview

Comment 20

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://reviewboard.mozilla.org/r/41045/#review38975

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d1c109f708b

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it apparently broke android builds like https://treeherder.mozilla.org/logviewer.html#?job_id=24648307&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/d210a03627b7

Comment 23

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8fd904eb3b

Comment 24

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: review-bug1256430-update-1.git.patch

Correcting the Android build breakage.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 26

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: review-bug1256430-update-2.git.patch

nits fixed, commits squished

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff7a5a4b24f

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ff7a5a4b24f