822704 - Enable webrtc/trunk WEBRTC_TRACE() logging via NSPR_LOG_MODULES

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

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

We want to be able to turn on WEBRTC_TRACE() logging without recompiling the code

Also moving 'ikran' to 'signaling'

Comment 1

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

Attachment: Enable WEBRTC_TRACE() logging via NSPR_LOG_MODULES and rename signaling log module

Note: the level is the webrtc TraceLevel value (bitmask); kTraceAll logs means setting to 65535 (not -1), see media/webrtc/trunk/src/common_types.h.  Note that kTraceAll logs are BIG, and they also are a circular log to avoid filling your disk.  The filename is 'WebRTC.log' in the executable dir unless you set the env var WEBRTC_TRACE_FILE

Comment 2

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

Not blocking but very handy

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 693479 [details] [diff] [review]
Enable WEBRTC_TRACE() logging via NSPR_LOG_MODULES and rename signaling log module

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

r=me, but see the comment about VoE.

::: media/webrtc/signaling/src/common/browser_logging/CSFLog.cpp
@@ +12,4 @@
>  
>  static PRLogModuleInfo *gLogModuleInfo = NULL;
>  
> +PRLogModuleInfo *GetSignalingLogInfo()

These parts should really be in a separate patch, but I'm not going to argue.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +108,5 @@
> +    }
> +    CSFLogDebug(logTag,  "%s Logging webrtc to %s level %d", __FUNCTION__,
> +                file, logs->level);
> +    mVideoEngine->SetTraceFilter(logs->level);
> +    mVideoEngine->SetTraceFile(file);

Don't we need one of these for the voice engine as well? Not sure if you'd want to share the same module for both, in which case you might want to rethink the module name.

Comment 4

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

(In reply to Timothy B. Terriberry (:derf) from comment #3)

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +108,5 @@
> > +    }
> > +    CSFLogDebug(logTag,  "%s Logging webrtc to %s level %d", __FUNCTION__,
> > +                file, logs->level);
> > +    mVideoEngine->SetTraceFilter(logs->level);
> > +    mVideoEngine->SetTraceFile(file);
> 
> Don't we need one of these for the voice engine as well? Not sure if you'd
> want to share the same module for both, in which case you might want to
> rethink the module name.

Voice and Video engines both call Trace::SetTraceLevel/File.  I.e. they're shared; there's only one control.  I may want to modify it to have a shared "turn on logging" from both conduits, but right now we're always creating a video conduit I think.

Comment 5

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

Attachment: interdiffs

Comment 6

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

Comment on attachment 693608 [details] [diff] [review]
interdiffs

Interdiffs (actually I had to use int instead of bool to keep the compiler happy)

Comment 7

[Timothy B. Terriberry (:derf)]

Comment on attachment 693608 [details] [diff] [review]
interdiffs

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

LGTM.

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/ccf586b1e679

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ccf586b1e679