Move RtcEventLog object from inside VoiceEngine to Call.

Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface from PeerConnectionFactory to PeerConnection, which makes more sense for this functionality. An API parameter to set an upper limit to the size of the logfile is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it is called from Chrome, it will be removed after Chrome is updated to use the PeerConnection interface.

BUG=webrtc:4741, webrtc:5603, chromium:609749
R=solenberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/1895526c6130e3d0e9b154f95079b8eda7567016
Cr-Commit-Position: refs/heads/master@{#13321}

[ivoc, 2016-03-01 14:17:46]

Description was changed from

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality.

BUG=webrtc:4741,webrtc:5603
==========

to

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. 
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603
==========

[ivoc, 2016-03-01 15:21:29]

Patchset #1 (id:1) has been deleted

[ivoc, 2016-03-01 15:34:17]

ivoc@webrtc.org changed reviewers:
+ solenberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org, tommi@webrtc.org

[the sun, 2016-03-02 09:56:12]

On 2016/03/01 15:34:17, ivoc wrote:
> mailto:ivoc@webrtc.org changed reviewers:
> + mailto:solenberg@webrtc.org, mailto:stefan@webrtc.org,
mailto:terelius@webrtc.org, mailto:tommi@webrtc.org

The first question is whether this change really makes sense from a usability
point of view?

Will the Chrome UI enable logging for all PCs in a tab? All PCs it knows about?
Is it a per-PC checkbox?

Much of the information in a log is useful on a per-PC basis. But is there, or
do we anticipate, information which is best treated in a per-process context?
I'm thinking about e.g. events to track audio glitches, cpu usage, etc. Unless
we all information in one place, such events won't tell the full story, leaving
us in a situation where we'll need to ask users to send us multiple log files
and we'll still not be sure what conclusions we can draw from any correlations
in the data.

[ivoc, 2016-03-02 10:52:19]

On 2016/03/02 09:56:12, the sun wrote:
> On 2016/03/01 15:34:17, ivoc wrote:
> > mailto:ivoc@webrtc.org changed reviewers:
> > + mailto:solenberg@webrtc.org, mailto:stefan@webrtc.org,
> mailto:terelius@webrtc.org, mailto:tommi@webrtc.org
> 
> The first question is whether this change really makes sense from a usability
> point of view?
> 
> Will the Chrome UI enable logging for all PCs in a tab? All PCs it knows
about?
> Is it a per-PC checkbox?
> 
> Much of the information in a log is useful on a per-PC basis. But is there, or
> do we anticipate, information which is best treated in a per-process context?
> I'm thinking about e.g. events to track audio glitches, cpu usage, etc. Unless
> we all information in one place, such events won't tell the full story,
leaving
> us in a situation where we'll need to ask users to send us multiple log files
> and we'll still not be sure what conclusions we can draw from any correlations
> in the data.

Usability is not my speciality, but the way I was envisioning this was to keep a
single checkbox in Chrome (to not overcomplicate things for users), but to
generate multiple event log files, one for each PC it knows about (which is what
currently ends up in the single file as well). 
Good question about logging global information like CPU usage. I have not heard
of anyone having plans to do this, but if it turns out to be useful, one option
would be to add it to each individual event log. Another option would be to
create a separate event log file for global data. If desired, the log files can
easily be combined later into a single file.

[ivoc, 2016-03-02 14:20:22]

Please have a look at this CL to move the Rtc event log object from VoE to Call
. This also moves the interface from PeerConnectionFactory to PeerConnection.
This CL does not remove the old function on PeerConnectionFactory yet, since
Chrome is currently using it. It will be removed as soon as Chrome is updated.

Unfortunately, passing a pointer into the RTP/RTCP module via the constructors
is no longer possible, so this is changed to a series of setter functions.

Since the CL is rather big (sorry!), feel free to only look at files within your
ownership.

[the sun, 2016-03-03 09:25:13]

That's a very nice change!

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnection.java:243: public boolean
startRtcEventLog(int file_descriptor) {
as discussed offline, add the max file size here to complete the interface, even
though the feature isn't implemented yet.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnection.java:249: public void
StopRtcEventLog() {
lower case 's' in 'stop'

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/PeerConnectionFactory.java (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnectionFactory.java:151: public boolean
startRtcEventLog(int file_descriptor) {
Do you need to leave these as dummy methods to avoid the build breaking?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2105: bool
PeerConnection::StartRtcEventLog(rtc::PlatformFile file) {
Nit: put methods in the order they are declared in the .h

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2118: if (call) {
Shouldn't need this condition; add an RTC_DCHECK() in call_w() before it returns
instead.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.h:70: bool StartRtcEventLog(rtc::PlatformFile
file) override { return false; }
Add a TODO that these should be removed

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.h:55: void SetRtcEventLog(webrtc::RtcEventLog*
event_log);
No need for this - pass it in the ctor instead.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:153: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_ =
RtcEventLog::Create();
Is there a thread checker in RtcEventLog::Create()? We want to enforce
construction/destruction is on the same thread.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:223:
congestion_controller_->GetBitrateController()->SetEventLog(event_log_.get());
Could we pass the log* in the CongestionController ctor?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:207: RtcEventLog* event_log_
GUARDED_BY(event_log_crit_);
Can you use critical_section_rtcp_sender_ instead?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:471: RtcEventLog* event_log_
GUARDED_BY(event_log_crit_);
Can you use send_critsect_ instead?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:503: { event_log_->LogAudioPlayout(ssrc); }
Remove the { } on this line.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:500: rtc::CriticalSection event_log_lock_;
nit: declare the lock above the members it protects

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_codec.h (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_codec.h:135: // Get a pointer to the event
logging object associated with this Voice
Remember that here you're changing an interface that is potentially used by 3rd
parties. How long have we had the GetEventLog() method? Is it reasonable to
assume nobody relies on it?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/test/...
File webrtc/voice_engine/test/cmd_test/voe_cmd_test.cc (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/test/...
webrtc/voice_engine/test/cmd_test/voe_cmd_test.cc:806:
codec->GetEventLog()->StartLogging(kDebugFileName, 30000);
Add a test at the PeerConnection level which exercises the whole stack.

[stefan-webrtc, 2016-03-03 10:08:25]

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:153: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_ =
RtcEventLog::Create();
Use std::unique_ptr. I'd also prefer if you create the log in the initializer
list.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:223:
congestion_controller_->GetBitrateController()->SetEventLog(event_log_.get());
On 2016/03/03 09:25:12, the sun wrote:
> Could we pass the log* in the CongestionController ctor?

That would be nicer.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:352: receive_stream->SetRtcEventLog(event_log_.get());
Isn't it possible to pass this via the constructor instead?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:78: RtcEventLog* event_log;
Why do we have to make this change? If it really is needed to change this after
construction, then I'd prefer to set a proxy here instead and change the object
in the proxy instead.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:207: RtcEventLog* event_log_
GUARDED_BY(event_log_crit_);
On 2016/03/03 09:25:13, the sun wrote:
> Can you use critical_section_rtcp_sender_ instead?

Or set it in the constructor and make it const?

[the sun, 2016-03-03 10:17:15]

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:78: RtcEventLog* event_log;
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> Why do we have to make this change? If it really is needed to change this
after
> construction, then I'd prefer to set a proxy here instead and change the
object
> in the proxy instead.

That's a good idea! Put the proxy among the others in voice_engine/channel.cc;
we still need to be able to set the event log on voe::Channel after
construction.

[terelius, 2016-03-10 10:42:32]

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2118: if (call) {
On 2016/03/03 09:25:12, the sun wrote:
> Shouldn't need this condition; add an RTC_DCHECK() in call_w() before it
returns
> instead.

I agree. The Call is created indirectly by CreatePeerConnection function, via
the MediaController constructor. Even though the actual creation is done on the
worker thread, the constructor blocks until creation is done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection.h
File webrtc/api/peerconnection.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.h:335: // Starts recording an Rtc EventLog using the
supplied platform file.
RtcEventLog as one word maybe?

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:222: bool
PeerConnectionFactory::StartRtcEventLog(rtc::PlatformFile file) {
I'm sorry, but I don't see why it is safe to effectively disable these calls
immediately. Won't this mean that the button on the webrtc-internals page will
stop working until Chrome has been updated? If so, I think it would be better to
keep them for now and remove them in the clean-up CL you are planning anyway.

[ivoc, 2016-03-10 13:13:12]

Patchset #3 (id:60001) has been deleted

[ivoc, 2016-03-10 13:15:36]

Please take another look, the CL expanded a bit while addressing the previous
comments.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnection.java:243: public boolean
startRtcEventLog(int file_descriptor) {
On 2016/03/03 09:25:12, the sun wrote:
> as discussed offline, add the max file size here to complete the interface,
even
> though the feature isn't implemented yet.

Done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnection.java:249: public void
StopRtcEventLog() {
On 2016/03/03 09:25:12, the sun wrote:
> lower case 's' in 'stop'

Done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
File webrtc/api/java/src/org/webrtc/PeerConnectionFactory.java (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/java/src/org/w...
webrtc/api/java/src/org/webrtc/PeerConnectionFactory.java:151: public boolean
startRtcEventLog(int file_descriptor) {
On 2016/03/03 09:25:12, the sun wrote:
> Do you need to leave these as dummy methods to avoid the build breaking?

I think these are not used anywhere from the java interface. Chrome does use the
function but calls the C++ function directly.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection.cc
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2105: bool
PeerConnection::StartRtcEventLog(rtc::PlatformFile file) {
On 2016/03/03 09:25:12, the sun wrote:
> Nit: put methods in the order they are declared in the .h

I moved them to a better place, but it's difficult to find the right spot
because the other functions are not in the same order as in the header :-)

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnection.cc:2118: if (call) {
On 2016/03/03 09:25:12, the sun wrote:
> Shouldn't need this condition; add an RTC_DCHECK() in call_w() before it
returns
> instead.

Good idea, done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.cc:222: bool
PeerConnectionFactory::StartRtcEventLog(rtc::PlatformFile file) {
On 2016/03/10 10:42:32, terelius wrote:
> I'm sorry, but I don't see why it is safe to effectively disable these calls
> immediately. Won't this mean that the button on the webrtc-internals page will
> stop working until Chrome has been updated? If so, I think it would be better
to
> keep them for now and remove them in the clean-up CL you are planning anyway.

Note that the functions still exist on PeerConnectionFactoryInterface, they will
just stop working until Chrome is updated. I was planning to make a Chrome CL
and land it soon after landing this one (wait until both are approved before
landing). I couldn't think of a way to keep both these functions working as well
as the new ones in PeerConnectionInterface. Let me know if you have a better
idea for how to handle this.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectionfactory.h:70: bool StartRtcEventLog(rtc::PlatformFile
file) override { return false; }
On 2016/03/03 09:25:12, the sun wrote:
> Add a TODO that these should be removed

Done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.h:55: void SetRtcEventLog(webrtc::RtcEventLog*
event_log);
On 2016/03/03 09:25:12, the sun wrote:
> No need for this - pass it in the ctor instead.

Good point.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:153: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_ =
RtcEventLog::Create();
On 2016/03/03 09:25:13, the sun wrote:
> Is there a thread checker in RtcEventLog::Create()? We want to enforce
> construction/destruction is on the same thread.

Done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:153: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_ =
RtcEventLog::Create();
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> Use std::unique_ptr. I'd also prefer if you create the log in the initializer
> list.
> 

I moved the initialization to the initializer list (although I was under the
impression that this style was preferred now). I think switching to
std::unique_ptr is a good idea, but I would prefer to do that in a separate CL
if that's okay with you.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:223:
congestion_controller_->GetBitrateController()->SetEventLog(event_log_.get());
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> On 2016/03/03 09:25:12, the sun wrote:
> > Could we pass the log* in the CongestionController ctor?
> 
> That would be nicer.

Good point, done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:352: receive_stream->SetRtcEventLog(event_log_.get());
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> Isn't it possible to pass this via the constructor instead?

Done.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:78: RtcEventLog* event_log;
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> Why do we have to make this change? If it really is needed to change this
after
> construction, then I'd prefer to set a proxy here instead and change the
object
> in the proxy instead.

The reason for this change is that we no longer have the RtcEventLog* available
at the place where the VoE Channel is constructed (which is where the RtpRtcp
class is constructed) in the new setup. When constructing a ChannelProxy inside
the AudioReceiveStream, it doesn't actually construct a VoE Channel, but rather
finds the one with the correct channel id. 
I will implement a proxy for the event log like you suggest.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:207: RtcEventLog* event_log_
GUARDED_BY(event_log_crit_);
On 2016/03/03 10:08:25, stefan-webrtc (holmer) wrote:
> On 2016/03/03 09:25:13, the sun wrote:
> > Can you use critical_section_rtcp_sender_ instead?
> 
> Or set it in the constructor and make it const?

This is changed into a pointer to the proxy object, so the critical section is
no longer needed.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:471: RtcEventLog* event_log_
GUARDED_BY(event_log_crit_);
On 2016/03/03 09:25:13, the sun wrote:
> Can you use send_critsect_ instead?

The critical section is no longer needed with the proxy object.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:503: { event_log_->LogAudioPlayout(ssrc); }
On 2016/03/03 09:25:13, the sun wrote:
> Remove the {&nbsp;} on this line.

Hmm, not sure what happened there :-)

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:500: rtc::CriticalSection event_log_lock_;
On 2016/03/03 09:25:13, the sun wrote:
> nit: declare the lock above the members it protects

The lock is gone due to the proxy object.

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/inclu...
File webrtc/voice_engine/include/voe_codec.h (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/inclu...
webrtc/voice_engine/include/voe_codec.h:135: // Get a pointer to the event
logging object associated with this Voice
On 2016/03/03 09:25:13, the sun wrote:
> Remember that here you're changing an interface that is potentially used by
3rd
> parties. How long have we had the GetEventLog() method? Is it reasonable to
> assume nobody relies on it?

It was introduced in Sept 2015, and is not currently in use in Google3 (as far
as I'm aware). It's not a terribly useful function for external users. I suppose
it could be used to start/stop the logging, but there are more convenient
functions on peerconnectionfactory to do that (which will move to peerconnection
in this CL).

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/test/...
File webrtc/voice_engine/test/cmd_test/voe_cmd_test.cc (left):

https://codereview.webrtc.org/1748403002/diff/40001/webrtc/voice_engine/test/...
webrtc/voice_engine/test/cmd_test/voe_cmd_test.cc:806:
codec->GetEventLog()->StartLogging(kDebugFileName, 30000);
On 2016/03/03 09:25:13, the sun wrote:
> Add a test at the PeerConnection level which exercises the whole stack.

This is a little complicated to do, because there is currently no way to open an
rtc::PlatformFile in WebRTC, which is used on the PC interface to pass in the
file. How do you think I should handle this?

[stefan-webrtc, 2016-03-11 10:11:53]

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log.cc:217: int64_t max_size_bytes) {
Can we break this change out to a separate CL, or is the max size depending on
moving to Call?

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_proxy.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.cc:25: if (event_log_) {
Can we DCHECK on this instead so that we know that we won't miss some packets in
the beginning?

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_proxy.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.h:1: /*
Shouldn't this file live in webrtc/audio? It's only used by audio code.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.h:23: RtcEventLogProxy(RtcEventLog* event_log) :
event_log_(event_log) {}
explicit

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:53: RtcEventLog*
event_log);
I think you'll want to keep the old Create method too since this is a
semi-public API. Mark it as deprecated.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:222: if
(event_log_) {
I guess we can remove these ifs now that we always set the event log? For tests
we can pass in a mock.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:78: RtcEventLogProxy* event_log;
I think you can keep this as an RtcEventLog*, the rtp module doesn't need to
know that it might be a proxy.

[the sun, 2016-03-11 10:29:47]

RtcEventLogProxy should even be an internal class in voice_engine/channel.cc. No
need for the lower layers to know about a RtcEventLogProxy - for them it's just
an incarnation of RtcEventLog. voe::Channel just uses it to be able to set the
actual RtcEventLog after construction.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_proxy.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.h:1: /*
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> Shouldn't this file live in webrtc/audio? It's only used by audio code.

This should even be an internal class in voice_engine/channel.cc. No need for
the lower layers to know about a RtcEventLogProxy - for them it's just an
incarnation of RtcEventLog. voe::Channel just uses it to be able to set the
actual RtcEventLog after construction.

[terelius, 2016-03-11 12:30:44]

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log.cc:217: int64_t max_size_bytes) {
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> Can we break this change out to a separate CL, or is the max size depending on
> moving to Call?

I think the total amount of work will decrease by doing it in one CL, and this
CL would not become significantly smaller by doing it separately. The file size
limit change just adds an argument to all the layers between the event log and
PeerConnection, and this CL is rewriting that anyway.

I guess we could do
bool StartLogging(rtc::PlatformFile log_file, int64_t max_size_bytes){
  StartLogging(logfile)
}
to avoid having to change all call sites right now though.

[ivoc, 2016-03-16 17:00:32]

Thanks for the comments! I hid the proxy object inside of VoE::Channel, and
addressed the other comments as well.
Please have another look.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log.cc:217: int64_t max_size_bytes) {
On 2016/03/11 12:30:44, terelius wrote:
> On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> > Can we break this change out to a separate CL, or is the max size depending
on
> > moving to Call?
> 
> I think the total amount of work will decrease by doing it in one CL, and this
> CL would not become significantly smaller by doing it separately. The file
size
> limit change just adds an argument to all the layers between the event log and
> PeerConnection, and this CL is rewriting that anyway.
> 
> I guess we could do
> bool StartLogging(rtc::PlatformFile log_file, int64_t max_size_bytes){
>   StartLogging(logfile)
> }
> to avoid having to change all call sites right now though.

The max size and moving to call are pretty independent, however, since we're
changing the peerconnection API in this CL, I think it makes sense to include it
here as well, to avoid having to change it again soon afterwards. 
I like Bjorn's suggestion, I will implement it like that.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_proxy.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.cc:25: if (event_log_) {
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> Can we DCHECK on this instead so that we know that we won't miss some packets
in
> the beginning?

Good idea.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
File webrtc/call/rtc_event_log_proxy.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.h:1: /*
On 2016/03/11 10:29:47, the sun wrote:
> On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> > Shouldn't this file live in webrtc/audio? It's only used by audio code.
> 
> This should even be an internal class in voice_engine/channel.cc. No need for
> the lower layers to know about a RtcEventLogProxy - for them it's just an
> incarnation of RtcEventLog. voe::Channel just uses it to be able to set the
> actual RtcEventLog after construction.

Excellent idea, I will move it there and hide the proxy from the outside world.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log_proxy.h:23: RtcEventLogProxy(RtcEventLog* event_log) :
event_log_(event_log) {}
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> explicit

I removed this constructor in the new code, so this is no longer needed.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:53: RtcEventLog*
event_log);
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> I think you'll want to keep the old Create method too since this is a
> semi-public API. Mark it as deprecated.

Done.

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/modules/bitrate_co...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:222: if
(event_log_) {
On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> I guess we can remove these ifs now that we always set the event log? For
tests
> we can pass in a mock.

I guess that makes sense, I can add a DCHECK in the constructor

[the sun, 2016-03-21 13:03:09]

A great update. Thanks for doing this; the code is much nicer!

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/mediacontroll...
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/mediacontroll...
webrtc/api/mediacontroller.cc:45: RTC_DCHECK(call_.get());
you need to rebase.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:2119: return
media_controller_->call_w()->RtcEventLog()->StartLogging(
Is Call::RtcEventLog used anywhere but here?

If not, I think we should remove it and instead add a Start/Stop pair of
functions on Call. That will keep the API surface down and the RtcEventLog
interface remains internal business. Exposing the interface like you do right
now gives the client more than it bargained for, and effectively makes
RtcEventLog (which should really be named RtcEventLogInterface btw) an API
available for 3rd parties (assuming Call will be made available for 3rd
parties).

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log.cc:70: ~RtcEventLogImpl();
override

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log_unittest.cc:318: nullptr);  // RtcEventLogProxy*
RtcEventLog

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:80:
BitrateController* BitrateController::CreateBitrateController(
Can you ditch this Create method and just have the one below? In how many places
is it used? If it's a matter of supplying a nullptr in three places, that is
better IMO.

Oh wait, it is used by downstream code? Add a TODO then that it should be
removed.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:29:
SendSideBandwidthEstimation(RtcEventLog* event_log);
explicit

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:93:
RtcEventLog* event_log_;
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:51: class RtcEventLogProxy : public
webrtc::RtcEventLog {
final

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:55: void SetBufferDuration(int64_t
buffer_duration_us) {
"override" here and below

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:62: void StartLogging(const std::string&
file_name, int duration_ms) {
It seems to me this method will never be called on the proxy object. Only the
Log... functions will ever be called from the components the proxy is give to,
right?

If that's correct, remove the code and add RTC_NOTREACHED.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:110: RTC_DCHECK(event_log_);
I don't see why you can DCHECK here but have to use a conditional above. Please
explain?

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:138: static bool ParseRtcEventLog(const
std::string& file_name,
Remove

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:148: RtcEventLog* event_log_ GUARDED_BY(crit_);
RTC_DISALLOW_COPY_AND_ASSIGN

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:950: configuration.event_log = &*event_log_;
nit: add () around *event_log_ for clarity.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:163: void
ChannelProxy::SetRtcEventLog(RtcEventLog* event_log) {
RTC_DCHECK(thread_checker_.CalledOnValidThread());

[stefan-webrtc, 2016-03-21 13:25:48]

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/80001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log.cc:217: int64_t max_size_bytes) {
On 2016/03/16 17:00:32, ivoc wrote:
> On 2016/03/11 12:30:44, terelius wrote:
> > On 2016/03/11 10:11:53, stefan-webrtc (holmer) wrote:
> > > Can we break this change out to a separate CL, or is the max size
depending
> on
> > > moving to Call?
> > 
> > I think the total amount of work will decrease by doing it in one CL, and
this
> > CL would not become significantly smaller by doing it separately. The file
> size
> > limit change just adds an argument to all the layers between the event log
and
> > PeerConnection, and this CL is rewriting that anyway.
> > 
> > I guess we could do
> > bool StartLogging(rtc::PlatformFile log_file, int64_t max_size_bytes){
> >   StartLogging(logfile)
> > }
> > to avoid having to change all call sites right now though.
> 
> The max size and moving to call are pretty independent, however, since we're
> changing the peerconnection API in this CL, I think it makes sense to include
it
> here as well, to avoid having to change it again soon afterwards. 
> I like Bjorn's suggestion, I will implement it like that.

Thanks.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:245: if
(event_log_) {
Remove

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:497: std::unique_ptr<voe::RtcEventLogProxy>
event_log_;
event_log_proxy_

[terelius, 2016-03-21 20:47:46]

Looks good, just a few questions below.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
File webrtc/api/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
webrtc/api/java/src/org/webrtc/PeerConnection.java:241: // the native code. If
an RTC event log is already being recorded, it will be
This is the current behavior, but is that how we want to handle multiple start
messages? At some point, we talked about about ignoring the call and proceeding
with the existing log and that is what I implemented in
https://codereview.webrtc.org/1687703002/. I honestly don't know which is
better.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
webrtc/api/java/src/org/webrtc/PeerConnection.java:248: nativePeerConnection,
file_descriptor, filesize_limit_bytes);
Nit: I believe filesize_limit_bytes is called max_size_bytes in the other
layers. Maybe use the same name everywhere?

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:2119: return
media_controller_->call_w()->RtcEventLog()->StartLogging(
On 2016/03/21 13:03:08, the sun wrote:
> Is Call::RtcEventLog used anywhere but here?
> 
> If not, I think we should remove it and instead add a Start/Stop pair of
> functions on Call. That will keep the API surface down and the RtcEventLog
> interface remains internal business. Exposing the interface like you do right
> now gives the client more than it bargained for, and effectively makes
> RtcEventLog (which should really be named RtcEventLogInterface btw) an API
> available for 3rd parties (assuming Call will be made available for 3rd
> parties).

+1

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/test...
File webrtc/voice_engine/test/auto_test/standard/codec_test.cc (left):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/test...
webrtc/voice_engine/test/auto_test/standard/codec_test.cc:180: TEST_F(CodecTest,
RtcEventLogIntegrationTest) {
Should there be an integration test for the new API too, or is it so simple that
isn't needed?

[ivoc, 2016-03-22 13:44:55]

Thanks again for the comments! See my responses below.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
File webrtc/api/java/src/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
webrtc/api/java/src/org/webrtc/PeerConnection.java:241: // the native code. If
an RTC event log is already being recorded, it will be
On 2016/03/21 20:47:45, terelius wrote:
> This is the current behavior, but is that how we want to handle multiple start
> messages? At some point, we talked about about ignoring the call and
proceeding
> with the existing log and that is what I implemented in
> https://codereview.webrtc.org/1687703002/. I honestly don't know which is
> better.

I think it makes sense for the comments in this CL to match the current
behavior. If that CL changes the behavior, I think it should also update the
comments here to match the new behavior. 
As for which behavior is best, I don't really have a strong opinion either way,
as it's not really something that should be happening anyway (in my opinion). As
long as it's documented, either is fine with me.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/java/src/org/...
webrtc/api/java/src/org/webrtc/PeerConnection.java:248: nativePeerConnection,
file_descriptor, filesize_limit_bytes);
On 2016/03/21 20:47:45, terelius wrote:
> Nit: I believe filesize_limit_bytes is called max_size_bytes in the other
> layers. Maybe use the same name everywhere?

Good point, will update.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/mediacontroll...
File webrtc/api/mediacontroller.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/mediacontroll...
webrtc/api/mediacontroller.cc:45: RTC_DCHECK(call_.get());
On 2016/03/21 13:03:08, the sun wrote:
> you need to rebase.

Done, thanks (I removed this line while rebasing).

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
File webrtc/api/peerconnection.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/api/peerconnectio...
webrtc/api/peerconnection.cc:2119: return
media_controller_->call_w()->RtcEventLog()->StartLogging(
On 2016/03/21 13:03:08, the sun wrote:
> Is Call::RtcEventLog used anywhere but here?
> 
> If not, I think we should remove it and instead add a Start/Stop pair of
> functions on Call. That will keep the API surface down and the RtcEventLog
> interface remains internal business. Exposing the interface like you do right
> now gives the client more than it bargained for, and effectively makes
> RtcEventLog (which should really be named RtcEventLogInterface btw) an API
> available for 3rd parties (assuming Call will be made available for 3rd
> parties).

Good point, I changed it to a start/stop pair.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log.cc:70: ~RtcEventLogImpl();
On 2016/03/21 13:03:08, the sun wrote:
> override

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
File webrtc/call/rtc_event_log_unittest.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/call/rtc_event_lo...
webrtc/call/rtc_event_log_unittest.cc:318: nullptr);  // RtcEventLogProxy*
On 2016/03/21 13:03:08, the sun wrote:
> RtcEventLog

Oops, missed that one, thanks.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:80:
BitrateController* BitrateController::CreateBitrateController(
On 2016/03/21 13:03:08, the sun wrote:
> Can you ditch this Create method and just have the one below? In how many
places
> is it used? If it's a matter of supplying a nullptr in three places, that is
> better IMO.
> 
> Oh wait, it is used by downstream code? Add a TODO then that it should be
> removed.

Stefan suggested that I should mark this one as deprecated and keep it around
for now. I did this in the interface (bitrate_controller.h), I'll add a TODO
there as well.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc
(right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:245: if
(event_log_) {
On 2016/03/21 13:25:48, stefan-webrtc (holmer) wrote:
> Remove

Good catch, done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:29:
SendSideBandwidthEstimation(RtcEventLog* event_log);
On 2016/03/21 13:03:09, the sun wrote:
> explicit 

Thanks, done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.h:93:
RtcEventLog* event_log_;
On 2016/03/21 13:03:09, the sun wrote:
> RTC_DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:51: class RtcEventLogProxy : public
webrtc::RtcEventLog {
On 2016/03/21 13:03:09, the sun wrote:
> final

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:55: void SetBufferDuration(int64_t
buffer_duration_us) {
On 2016/03/21 13:03:09, the sun wrote:
> "override" here and below

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:62: void StartLogging(const std::string&
file_name, int duration_ms) {
On 2016/03/21 13:03:09, the sun wrote:
> It seems to me this method will never be called on the proxy object. Only the
> Log... functions will ever be called from the components the proxy is give to,
> right?
> 
> If that's correct, remove the code and add RTC_NOTREACHED.

You're correct, done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:110: RTC_DCHECK(event_log_);
On 2016/03/21 13:03:09, the sun wrote:
> I don't see why you can DCHECK here but have to use a conditional above.
Please
> explain?

I was thinking about the lifetimes of the AudioReceiveStream compared to the
lifetime of the objects calling the LogX() functions (i.e. VoE Channel). I
thought it might be possible for the LogX functions to be called after the
AudioReceiveStream is destroyed (which replaces the event_log_ with a nullptr),
but before the VoE Channel is destroyed.
Thinking about it a bit more, there should probably be conditionals everywhere
to prevent this issue.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:138: static bool ParseRtcEventLog(const
std::string& file_name,
On 2016/03/21 13:03:09, the sun wrote:
> Remove

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:148: RtcEventLog* event_log_ GUARDED_BY(crit_);
On 2016/03/21 13:03:09, the sun wrote:
> RTC_DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:950: configuration.event_log = &*event_log_;
On 2016/03/21 13:03:09, the sun wrote:
> nit: add () around *event_log_ for clarity.

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.h:497: std::unique_ptr<voe::RtcEventLogProxy>
event_log_;
On 2016/03/21 13:25:48, stefan-webrtc (holmer) wrote:
> event_log_proxy_

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:163: void
ChannelProxy::SetRtcEventLog(RtcEventLog* event_log) {
On 2016/03/21 13:03:09, the sun wrote:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/test...
File webrtc/voice_engine/test/auto_test/standard/codec_test.cc (left):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/test...
webrtc/voice_engine/test/auto_test/standard/codec_test.cc:180: TEST_F(CodecTest,
RtcEventLogIntegrationTest) {
On 2016/03/21 20:47:46, terelius wrote:
> Should there be an integration test for the new API too, or is it so simple
that
> isn't needed?

I would like to add one, but there is currently no support for opening platform
files in WebRTC. It would be a good idea to try to add this in a follow-up CL.

[the sun, 2016-03-23 10:26:45]

lgtm % final set of comments (mostly nits)

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1748403002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel.cc:110: RTC_DCHECK(event_log_);
On 2016/03/22 13:44:55, ivoc wrote:
> On 2016/03/21 13:03:09, the sun wrote:
> > I don't see why you can DCHECK here but have to use a conditional above.
> Please
> > explain?
> 
> I was thinking about the lifetimes of the AudioReceiveStream compared to the
> lifetime of the objects calling the LogX() functions (i.e. VoE Channel). I
> thought it might be possible for the LogX functions to be called after the
> AudioReceiveStream is destroyed (which replaces the event_log_ with a
nullptr),
> but before the VoE Channel is destroyed.
> Thinking about it a bit more, there should probably be conditionals everywhere
> to prevent this issue.

Well, that, and we must also account for the scenario where the Channel is used
without an AudioSend/ReceiveStream.

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectionproxy.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnectionproxy.h:77: PROXY_METHOD2(bool, StartRtcEventLog,
rtc::PlatformFile, int64_t)
nit: add comments in peeroconnectionfactoryproxy.h about removing the methods
there.

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call.h#newcode143
webrtc/call.h:143: 
nit: remove blank line

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:158: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_;
use unique_ptr

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:128: sigslot::signal2<VideoCapturer*, CaptureState>
SignalVideoCaptureStateChange;
bad merge?

[ivoc, 2016-03-23 14:27:26]

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectionproxy.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/api/peerconnectio...
webrtc/api/peerconnectionproxy.h:77: PROXY_METHOD2(bool, StartRtcEventLog,
rtc::PlatformFile, int64_t)
On 2016/03/23 10:26:44, the sun wrote:
> nit: add comments in peeroconnectionfactoryproxy.h about removing the methods
> there.

Done.

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call.h#newcode143
webrtc/call.h:143: 
On 2016/03/23 10:26:45, the sun wrote:
> nit: remove blank line

Done.

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:158: rtc::scoped_ptr<webrtc::RtcEventLog> event_log_;
On 2016/03/23 10:26:45, the sun wrote:
> use unique_ptr

Done.

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/pc/channelmanager.h
File webrtc/pc/channelmanager.h (right):

https://codereview.webrtc.org/1748403002/diff/140001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.h:128: sigslot::signal2<VideoCapturer*, CaptureState>
SignalVideoCaptureStateChange;
On 2016/03/23 10:26:45, the sun wrote:
> bad merge?

Oh, oops, I think this is from the common ancestor (I use 3-way merge when
rebasing). Will remove, good find!

[ivoc, 2016-03-23 15:15:44]

Patchset #8 (id:180001) has been deleted

[ivoc, 2016-03-29 11:38:02]

Patchset #9 (id:220001) has been deleted

[ivoc, 2016-03-29 11:42:41]

Patchset #10 (id:260001) has been deleted

[ivoc, 2016-03-30 15:09:16]

Patchset #9 (id:240001) has been deleted

[ivoc, 2016-03-30 15:19:58]

The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 15:20:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748403002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748403002/280001

[the sun, 2016-03-30 15:20:38]

https://codereview.webrtc.org/1748403002/diff/280001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1748403002/diff/280001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:104: EXPECT_CALL(*channel_proxy_,
SetRtcEventLog(testing::NotNull()))
Could check for argument to be &event_log_ ?

[commit-bot: I haz the power, 2016-03-30 15:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-30 15:24:22]

Dry run: Try jobs failed on following builders:
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8564)

[ivoc, 2016-03-31 09:06:56]

https://codereview.webrtc.org/1748403002/diff/280001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1748403002/diff/280001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:104: EXPECT_CALL(*channel_proxy_,
SetRtcEventLog(testing::NotNull()))
On 2016/03/30 15:20:38, the sun wrote:
> Could check for argument to be &event_log_ ?

Good idea, done.

[ivoc, 2016-03-31 09:19:09]

The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-31 09:19:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748403002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748403002/300001

[commit-bot: I haz the power, 2016-03-31 11:19:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 11:19:41]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[terelius, 2016-04-01 11:26:01]

Nice! LGTM.

[stefan-webrtc, 2016-04-01 11:41:02]

lgtm webrtc/modules/

[tommi, 2016-04-04 14:26:47]

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:98:
channel_proxy_->SetRtcEventLog(event_log);
how is ownership and lifetime of |event_log| managed?

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:59: RtcEventLog*
event_log);
is the ownership/lifetime of |observer| and |event_log| handled the same way?

[ivoc, 2016-04-05 08:02:58]

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:98:
channel_proxy_->SetRtcEventLog(event_log);
On 2016/04/04 14:26:47, tommi-webrtc wrote:
> how is ownership and lifetime of |event_log| managed?

|event_log| is a member of Call, which should outlive its AudioReceiveStreams
(although this relationship is not very explicit in the code). 
I think it's common for raw pointers of Call-members to be passed to the various
send/receive Streams, see the CongestionController* above for another example.

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:59: RtcEventLog*
event_log);
On 2016/04/04 14:26:47, tommi-webrtc wrote:
> is the ownership/lifetime of |observer| and |event_log| handled the same way?

The |observer| is Call itself in this case, and |event_log| is a member of Call,
so they should have the same lifetime.
BitrateController is a member of CongestionController, which is also a member of
Call. So everything is part of the Call family :-)

[tommi, 2016-04-07 13:21:53]

lgtm.  The ownership management can be improved on but OK as is since it sounds
like we're at least not regressing.

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:98:
channel_proxy_->SetRtcEventLog(event_log);
On 2016/04/05 08:02:58, ivoc wrote:
> On 2016/04/04 14:26:47, tommi-webrtc wrote:
> > how is ownership and lifetime of |event_log| managed?
> 
> |event_log| is a member of Call, which should outlive its AudioReceiveStreams
> (although this relationship is not very explicit in the code). 
> I think it's common for raw pointers of Call-members to be passed to the
various
> send/receive Streams, see the CongestionController* above for another example.

As a pattern, I think it's one to avoid if we can - even though we have examples
of it.
If you're sure that we're at least as safe here as we are with the pointer to
CongestionController (i.e. they have the same lifetime), then that's good enough
for now.

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
File webrtc/modules/bitrate_controller/include/bitrate_controller.h (right):

https://codereview.webrtc.org/1748403002/diff/300001/webrtc/modules/bitrate_c...
webrtc/modules/bitrate_controller/include/bitrate_controller.h:59: RtcEventLog*
event_log);
On 2016/04/05 08:02:58, ivoc wrote:
> On 2016/04/04 14:26:47, tommi-webrtc wrote:
> > is the ownership/lifetime of |observer| and |event_log| handled the same
way?
> 
> The |observer| is Call itself in this case, and |event_log| is a member of
Call,
> so they should have the same lifetime.
> BitrateController is a member of CongestionController, which is also a member
of
> Call. So everything is part of the Call family :-)

I see.  Well, that does sound a bit fragile but OK for now.  As we do further
updates, we can improve on object ownership, lifetime and how to make those
things clear in the code.

[stefan-webrtc, 2016-05-08 15:57:02]

Are we ready to land this?

Btw, could you update the description to mention that it also adds a file size
limit to the startRtcEventLog API?

[ivoc, 2016-05-09 07:48:27]

On 2016/05/08 15:57:02, stefan-webrtc (holmer) wrote:
> Are we ready to land this?
> 
> Btw, could you update the description to mention that it also adds a file size
> limit to the startRtcEventLog API?

This CL is ready for landing, however since it will break the Chrome event log
functionality, I want to hold off landing this one until my Chrome CL is ready
for landing as well. Let me know if you have a better idea for handling this. 
I will add the comment about the filesize limit, but note that this only adds
the argument to the API, the actual implementation of that feature is not part
of this CL.

[stefan-webrtc, 2016-05-09 08:21:12]

On 2016/05/09 07:48:27, ivoc wrote:
> On 2016/05/08 15:57:02, stefan-webrtc (holmer) wrote:
> > Are we ready to land this?
> > 
> > Btw, could you update the description to mention that it also adds a file
size
> > limit to the startRtcEventLog API?
> 
> This CL is ready for landing, however since it will break the Chrome event log
> functionality, I want to hold off landing this one until my Chrome CL is ready
> for landing as well. Let me know if you have a better idea for handling this. 
> I will add the comment about the filesize limit, but note that this only adds
> the argument to the API, the actual implementation of that feature is not part
> of this CL.

Thanks for the update. How far off is your Chrome CL? Any chance we can split
this in two and land the file-size limit API change separately?

[ivoc, 2016-05-09 09:31:52]

Description was changed from

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. 
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603
==========

to

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603
==========

[ivoc, 2016-05-09 09:34:27]

On 2016/05/09 08:21:12, stefan-webrtc (holmer) wrote:
> On 2016/05/09 07:48:27, ivoc wrote:
> > On 2016/05/08 15:57:02, stefan-webrtc (holmer) wrote:
> > > Are we ready to land this?
> > > 
> > > Btw, could you update the description to mention that it also adds a file
> size
> > > limit to the startRtcEventLog API?
> > 
> > This CL is ready for landing, however since it will break the Chrome event
log
> > functionality, I want to hold off landing this one until my Chrome CL is
ready
> > for landing as well. Let me know if you have a better idea for handling
this. 
> > I will add the comment about the filesize limit, but note that this only
adds
> > the argument to the API, the actual implementation of that feature is not
part
> > of this CL.
> 
> Thanks for the update. How far off is your Chrome CL? Any chance we can split
> this in two and land the file-size limit API change separately?

The chrome CL is ready for review, but unfortunately it turned into a rather big
and complicated CL, so the review can take a while. I'm hoping I can get it in
before the cut.

[ivoc, 2016-05-09 09:47:39]

On 2016/05/09 08:21:12, stefan-webrtc (holmer) wrote:
> On 2016/05/09 07:48:27, ivoc wrote:
> > On 2016/05/08 15:57:02, stefan-webrtc (holmer) wrote:
> > > Are we ready to land this?
> > > 
> > > Btw, could you update the description to mention that it also adds a file
> size
> > > limit to the startRtcEventLog API?
> > 
> > This CL is ready for landing, however since it will break the Chrome event
log
> > functionality, I want to hold off landing this one until my Chrome CL is
ready
> > for landing as well. Let me know if you have a better idea for handling
this. 
> > I will add the comment about the filesize limit, but note that this only
adds
> > the argument to the API, the actual implementation of that feature is not
part
> > of this CL.
> 
> Thanks for the update. How far off is your Chrome CL? Any chance we can split
> this in two and land the file-size limit API change separately?

About splitting up the file-size limit API into a separate CL: it's definitely
possible, but then when this CL lands any users of this feature will still have
to be updated in addition to Chrome (because the API call still needs to move
from PeerConnectionFactory to PeerConnection). If we want to expose this feature
as soon as possible, this is probably the fastest way to do it. Do you think we
should take this approach?

[stefan-webrtc, 2016-05-09 11:43:03]

On 2016/05/09 09:47:39, ivoc wrote:
> On 2016/05/09 08:21:12, stefan-webrtc (holmer) wrote:
> > On 2016/05/09 07:48:27, ivoc wrote:
> > > On 2016/05/08 15:57:02, stefan-webrtc (holmer) wrote:
> > > > Are we ready to land this?
> > > > 
> > > > Btw, could you update the description to mention that it also adds a
file
> > size
> > > > limit to the startRtcEventLog API?
> > > 
> > > This CL is ready for landing, however since it will break the Chrome event
> log
> > > functionality, I want to hold off landing this one until my Chrome CL is
> ready
> > > for landing as well. Let me know if you have a better idea for handling
> this. 
> > > I will add the comment about the filesize limit, but note that this only
> adds
> > > the argument to the API, the actual implementation of that feature is not
> part
> > > of this CL.
> > 
> > Thanks for the update. How far off is your Chrome CL? Any chance we can
split
> > this in two and land the file-size limit API change separately?
> 
> About splitting up the file-size limit API into a separate CL: it's definitely
> possible, but then when this CL lands any users of this feature will still
have
> to be updated in addition to Chrome (because the API call still needs to move
> from PeerConnectionFactory to PeerConnection). If we want to expose this
feature
> as soon as possible, this is probably the fastest way to do it. Do you think
we
> should take this approach?

I think it would be good to land the file-size limit separately if it's not too
much work.

[ivoc, 2016-06-01 07:31:22]

Description was changed from

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603
==========

to

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603,chromium:609749
==========

[ivoc, 2016-06-15 08:46:56]

The CQ bit was checked by ivoc@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 08:47:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748403002/380001

[commit-bot: I haz the power, 2016-06-15 09:29:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 09:29:49]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-06-29 08:42:59]

lgtm again for bwe related changes.

[tommi, 2016-06-29 09:00:22]

On 2016/06/29 08:42:59, stefan-webrtc (holmer) wrote:
> lgtm again for bwe related changes.

Is this cl still being worked on? Seems like a lot of things might have changed
in the past couple of months and we're close to cutting a release branch. If
this will need time to stabilize, we should probably wait until next week.

[ivoc, 2016-06-29 09:52:29]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-06-29 09:52:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, tommi@webrtc.org, terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1748403002/#ps420001 (title:
"Another rebase and accompanying changes.")

[commit-bot: I haz the power, 2016-06-29 09:52:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-06-29 10:10:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 10:10:27]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14589)

[ivoc, 2016-06-29 11:57:05]

Description was changed from

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603,chromium:609749
==========

to

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603,chromium:609749
R=solenberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1895526c6130e3d0e9b154f95...
==========

[ivoc, 2016-06-29 11:57:08]

Committed patchset #16 (id:420001) manually as
1895526c6130e3d0e9b154f95079b8eda7567016 (presubmit successful).

[commit-bot: I haz the power, 2016-06-29 11:57:10]

Description was changed from

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603,chromium:609749
R=solenberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1895526c6130e3d0e9b154f95...
==========

to

==========
Move RtcEventLog object from inside VoiceEngine to Call.

In addition to moving the logging object itself, this also moves the interface
from PeerConnectionFactory to PeerConnection, which makes more sense for this
functionality. An API parameter to set an upper limit to the size of the logfile
is introduced.
The old interface on PeerConnectionFactory is not removed in this CL, because it
is called from Chrome, it will be removed after Chrome is updated to use the
PeerConnection interface.

BUG=webrtc:4741,webrtc:5603,chromium:609749
R=solenberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/1895526c6130e3d0e9b154f95079b8eda7567016
Cr-Commit-Position: refs/heads/master@{#13321}
==========

[ivoc, 2016-06-30 07:59:20]

A revert of this CL (patchset #16 id:420001) has been created in
https://codereview.webrtc.org/2111813002/ by ivoc@webrtc.org.

The reason for reverting is: Reverting all CLs related to moving the eventlog,
as they break Chromium tests..