Closed
Bug 1256430
Opened 8 years ago
Closed 8 years ago
about:webrtc button Start AEC Logging does not ensure that AEC logging is configured
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: ng, Assigned: ng)
Details
Attachments
(1 file, 6 obsolete files)
5.68 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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
Assignee | ||
Comment 3•8 years ago
|
||
(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?
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
Flags: needinfo?(na-g) → needinfo?(rjesup)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #2) > Nico -- Can you take this? Sure.
Comment 5•8 years ago
|
||
No blocking flags (and no tracking-<release> flags either)
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → wontfix
status-firefox-esr45:
--- → affected
Flags: needinfo?(rjesup)
Updated•8 years ago
|
Rank: 17 → 15
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=faec5b6a30e3
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41045/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41045/
Attachment #8732228 -
Flags: review?(pkerr)
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41047/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41047/
Attachment #8732229 -
Flags: review?(pkerr)
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41049/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41049/
Attachment #8732230 -
Flags: review?(pkerr)
Assignee | ||
Comment 10•8 years ago
|
||
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/
Attachment #8732231 -
Flags: review?(pkerr)
Comment 11•8 years ago
|
||
(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•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8732230 -
Attachment is obsolete: true
Attachment #8732230 -
Flags: review?(pkerr)
Assignee | ||
Updated•8 years ago
|
Attachment #8732231 -
Attachment is obsolete: true
Attachment #8732231 -
Flags: review?(pkerr)
Assignee | ||
Updated•8 years ago
|
Attachment #8732228 -
Attachment is obsolete: true
Attachment #8732228 -
Flags: review?(pkerr)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=541cbcff6d75
Assignee | ||
Updated•8 years ago
|
Attachment #8732229 -
Attachment is obsolete: true
Attachment #8732229 -
Flags: review?(pkerr)
Assignee | ||
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: needinfo?(pkerr)
Updated•8 years ago
|
Attachment #8732228 -
Attachment is patch: true
Attachment #8732228 -
Attachment mime type: text/x-review-board-request → text/plain
Updated•8 years ago
|
Attachment #8732228 -
Attachment is patch: false
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8732228 -
Attachment is obsolete: true
Attachment #8734845 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
https://reviewboard.mozilla.org/r/41045/#review38975
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
Flags: needinfo?(na-g)
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8fd904eb3b
Assignee | ||
Comment 24•8 years ago
|
||
Correcting the Android build breakage.
Attachment #8734845 -
Attachment is obsolete: true
Flags: needinfo?(na-g)
Attachment #8734975 -
Flags: review?(pkerr)
Comment 25•8 years ago
|
||
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+
Assignee | ||
Comment 26•8 years ago
|
||
nits fixed, commits squished
Attachment #8734975 -
Attachment is obsolete: true
Attachment #8735681 -
Flags: review?(rjesup)
Updated•8 years ago
|
Attachment #8735681 -
Flags: review?(rjesup) → review+
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ff7a5a4b24f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•