Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
Committed: https://crrev.com/429c345b0297d14211c3e14c907d4e5aac726590
Cr-Commit-Position: refs/heads/master@{#11336}

[terelius, 2016-01-11 10:00:58]

Description was changed from

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing a pointer from Channel through
ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
==========

to

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from
Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
==========

[terelius, 2016-01-11 10:00:58]

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

[terelius, 2016-01-11 12:15:45]

Åsa, could you review modules/rtp_rtcp/?
Fredrik, could you review voice_engine/ and call/?

[pbos-webrtc, 2016-01-11 12:16:56]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-01-11 12:16:56]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc#newcode681
webrtc/call/call.cc:681: event_log_->LogRtcpPacket(true, media_type, packet,
length);
Can we change this into an enum that has kIncoming and kOutgoing to make this
mistake harder?

[terelius, 2016-01-11 12:22:52]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc#newcode681
webrtc/call/call.cc:681: event_log_->LogRtcpPacket(true, media_type, packet,
length);
On 2016/01/11 12:16:56, pbos-webrtc wrote:
> Can we change this into an enum that has kIncoming and kOutgoing to make this
> mistake harder?

I suppose we could do that, though I think the origin of this bug is that the
RTCP packet is passed to a SendStream, which on first glance looks as if it is
outgoing.

[the sun, 2016-01-11 12:36:16]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc#newcode681
webrtc/call/call.cc:681: event_log_->LogRtcpPacket(true, media_type, packet,
length);
On 2016/01/11 12:16:56, pbos-webrtc wrote:
> Can we change this into an enum that has kIncoming and kOutgoing to make this
> mistake harder?

+1

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:77: RtcEventLog* event_log;
= nullptr

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:107: RtcEventLog* event_log_;
* const event_log_ = nullptr
(there's a an implict ctor here which leaves the field uninited)
here, and in more places.

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:208: RtcEventLog* event_log_;
RtcEventLog* const event_log_ = nullptr;

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1067:
event_log_->LogRtpHeader(true, MediaType::ANY, buffer,
Add a test case for this which uses a MockRtcEventLog

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:438: RtcEventLog* const event_log_;
= nullptr

[terelius, 2016-01-11 13:07:20]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc#newcode681
webrtc/call/call.cc:681: event_log_->LogRtcpPacket(true, media_type, packet,
length);
On 2016/01/11 12:36:16, the sun wrote:
> On 2016/01/11 12:16:56, pbos-webrtc wrote:
> > Can we change this into an enum that has kIncoming and kOutgoing to make
this
> > mistake harder?
> 
> +1

There is an enum RTPDirections in common_types.h. Do you think I should reuse it
(for both RTP and RTCP), or should I create a new enum in rtc_event_log.h?

[stefan-webrtc, 2016-01-11 13:12:58]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-01-11 13:12:59]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:28: namespace rtcp {
Empty line above.

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:77: RtcEventLog* event_log;
On 2016/01/11 12:36:16, the sun wrote:
> = nullptr

I'd prefer not to mix "= nullptr" with initializing members in the constructor.

For video code I think we at some point decided against using this feature of
C++11 as it becomes harder to know if a parameter has been initialized or not by
only looking in the initializer list (basically you need to check both the .h
and the .cc).

[terelius, 2016-01-11 13:22:12]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:77: RtcEventLog* event_log;
On 2016/01/11 13:12:59, stefan-webrtc (holmer) wrote:
> On 2016/01/11 12:36:16, the sun wrote:
> > = nullptr
> 
> I'd prefer not to mix "= nullptr" with initializing members in the
constructor.
> 
> For video code I think we at some point decided against using this feature of
> C++11 as it becomes harder to know if a parameter has been initialized or not
by
> only looking in the initializer list (basically you need to check both the .h
> and the .cc).

Yeah, I was just about to ask whether the I should change all the other pointers
too. I think we should be consistent, at least within each file; either we
default all pointers to null or we leave all of them uninitialized.

[the sun, 2016-01-11 13:40:22]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:77: RtcEventLog* event_log;
On 2016/01/11 13:22:11, terelius wrote:
> On 2016/01/11 13:12:59, stefan-webrtc (holmer) wrote:
> > On 2016/01/11 12:36:16, the sun wrote:
> > > = nullptr
> > 
> > I'd prefer not to mix "= nullptr" with initializing members in the
> constructor.
> > 
> > For video code I think we at some point decided against using this feature
of
> > C++11 as it becomes harder to know if a parameter has been initialized or
not
> by
> > only looking in the initializer list (basically you need to check both the
.h
> > and the .cc).

That is already hard since we do not include *all* members in the ctor init list
(aggregate objects with default ctors are generally excluded). If we
consistently used class definition initialization, it would actually be easier
than now. Then, anything in a ctor init list could be assumed to be an override
of the default.

> 
> Yeah, I was just about to ask whether the I should change all the other
pointers
> too. I think we should be consistent, at least within each file; either we
> default all pointers to null or we leave all of them uninitialized.

Agree it's better to be consistent.

If you prefer to use ctor init list, make sure to use
RTC_DISALLOW_IMPLICT_CONSTRUCTORS in the other classes where you added this
member.

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

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:28: namespace rtcp {
On 2016/01/11 13:12:59, stefan-webrtc (holmer) wrote:
> Empty line above.

Done.

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:77: RtcEventLog* event_log;
On 2016/01/11 13:40:22, the sun wrote:
> Agree it's better to be consistent.
> 
> If you prefer to use ctor init list, make sure to use
> RTC_DISALLOW_IMPLICT_CONSTRUCTORS in the other classes where you added this
> member.

Added RTC_DISALLOW_COPY_AND_ASSIGN.

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:107: RtcEventLog* event_log_;
On 2016/01/11 12:36:16, the sun wrote:
> * const event_log_ = nullptr
> (there's a an implict ctor here which leaves the field uninited)
> here, and in more places.

Added RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(PacketContainer) instead

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:208: RtcEventLog* event_log_;
On 2016/01/11 12:36:16, the sun wrote:
> RtcEventLog* const event_log_ = nullptr;

Added RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RTCPSender); instead

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.h:438: RtcEventLog* const event_log_;
On 2016/01/11 12:36:16, the sun wrote:
> = nullptr

Added RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RTPSender) instead.

[the sun, 2016-01-11 15:59:57]

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

https://codereview.webrtc.org/1571283002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:80:
RTC_DISALLOW_COPY_AND_ASSIGN(Configuration);
This is a POD, right? Unless it owns any of these resources it makes sense for
it to be copyable/assignable.

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

https://codereview.webrtc.org/1571283002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:208: RtcEventLog* event_log_;
Still missing * const, or can the event_log pointer change during the object's
life time?

[terelius, 2016-01-11 18:39:47]

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

https://codereview.webrtc.org/1571283002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:80:
RTC_DISALLOW_COPY_AND_ASSIGN(Configuration);
On 2016/01/11 15:59:57, the sun wrote:
> This is a POD, right? Unless it owns any of these resources it makes sense for
> it to be copyable/assignable.

Not sure it is technically POD since the constructor calls
NullObjectReceiveStatistics and NullObjectRtpAudioFeedback. However, for all
practical purposes it should behave as a POD structure and the default copy
constructor should be fine.

I removed the operations mostly because of your
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS suggestion. Copying the configuration struct
used to construct an RTPSender would have a similar effect to copying an
RTPSender. In particular, both senders would use the same bitrate estimator,
RTCP stats and event log etc and there might be race conditions since the
Senders would have separate locks protecting the same data.

I don't have a strong opinion on this, but I think it makes sense to remove copy
and assignment.

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

https://codereview.webrtc.org/1571283002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_sender.h:208: RtcEventLog* event_log_;
On 2016/01/11 15:59:57, the sun wrote:
> Still missing * const, or can the event_log pointer change during the object's
> life time?

Done.

[the sun, 2016-01-12 12:05:21]

lgtm

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

https://codereview.webrtc.org/1571283002/diff/20001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:80:
RTC_DISALLOW_COPY_AND_ASSIGN(Configuration);
On 2016/01/11 18:39:47, terelius wrote:
> On 2016/01/11 15:59:57, the sun wrote:
> > This is a POD, right? Unless it owns any of these resources it makes sense
for
> > it to be copyable/assignable.
> 
> Not sure it is technically POD since the constructor calls
> NullObjectReceiveStatistics and NullObjectRtpAudioFeedback. However, for all
> practical purposes it should behave as a POD structure and the default copy
> constructor should be fine.
> 
> I removed the operations mostly because of your
> RTC_DISALLOW_IMPLICIT_CONSTRUCTORS suggestion. Copying the configuration
struct
> used to construct an RTPSender would have a similar effect to copying an
> RTPSender. In particular, both senders would use the same bitrate estimator,
> RTCP stats and event log etc and there might be race conditions since the
> Senders would have separate locks protecting the same data.
> 
> I don't have a strong opinion on this, but I think it makes sense to remove
copy
> and assignment.

Acknowledged.

[terelius, 2016-01-13 17:08:18]

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/call/call.cc#newcode681
webrtc/call/call.cc:681: event_log_->LogRtcpPacket(true, media_type, packet,
length);
On 2016/01/11 13:07:20, terelius wrote:
> On 2016/01/11 12:36:16, the sun wrote:
> > On 2016/01/11 12:16:56, pbos-webrtc wrote:
> > > Can we change this into an enum that has kIncoming and kOutgoing to make
> this
> > > mistake harder?
> > 
> > +1
> 
> There is an enum RTPDirections in common_types.h. Do you think I should reuse
it
> (for both RTP and RTCP), or should I create a new enum in rtc_event_log.h?

Added an enum in rtc_event_log.h

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1571283002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1067:
event_log_->LogRtpHeader(true, MediaType::ANY, buffer,
On 2016/01/11 12:36:16, the sun wrote:
> Add a test case for this which uses a MockRtcEventLog

Done.

[terelius, 2016-01-14 15:58:55]

I don't have any further commits pending for this CL.

@pbos: Do you want to review the full CL, or was your comment just something you
noticed in passing?

@stefan and åsa: I don't think I need owner approval from both of you. If you're
busy, feel free to decide on one reviewer.

[pbos-webrtc, 2016-01-14 16:05:04]

On 2016/01/14 15:58:55, terelius wrote:
> I don't have any further commits pending for this CL.
> 
> @pbos: Do you want to review the full CL, or was your comment just something
you
> noticed in passing?
> 
> @stefan and åsa: I don't think I need owner approval from both of you. If
you're
> busy, feel free to decide on one reviewer.

Just drive-by, proceed without me please. :)

[stefan-webrtc, 2016-01-18 15:43:30]

stefan@webrtc.org changed reviewers:
- stefan@webrtc.org

[stefan-webrtc, 2016-01-18 15:43:49]

On 2016/01/14 16:05:04, pbos-webrtc wrote:
> On 2016/01/14 15:58:55, terelius wrote:
> > I don't have any further commits pending for this CL.
> > 
> > @pbos: Do you want to review the full CL, or was your comment just something
> you
> > noticed in passing?
> > 
> > @stefan and åsa: I don't think I need owner approval from both of you. If
> you're
> > busy, feel free to decide on one reviewer.
> 
> Just drive-by, proceed without me please. :)

Removed myself as a reviewer :)

[åsapersson, 2016-01-19 16:04:15]

lgtm

[ivoc, 2016-01-19 16:31:16]

After our offline discussion yesterday: lgtm.

[terelius, 2016-01-21 12:58:30]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-01-21 12:58:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, ivoc@webrtc.org, asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1571283002/#ps180001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-01-21 12:58:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1571283002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1571283002/180001

[commit-bot: I haz the power, 2016-01-21 13:02:44]

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

[commit-bot: I haz the power, 2016-01-21 13:02:45]

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

[stefan-webrtc, 2016-01-21 13:29:23]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-01-21 13:29:24]

lgtm

[terelius, 2016-01-21 13:30:44]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-01-21 13:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1571283002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1571283002/180001

[commit-bot: I haz the power, 2016-01-21 13:42:07]

Description was changed from

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from
Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
==========

to

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from
Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
==========

[commit-bot: I haz the power, 2016-01-21 13:42:08]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-21 13:42:15]

Description was changed from

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from
Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741
==========

to

==========
Fixes a bug which incorrectly logs incoming RTCP as outgoing.

Adds logging to RTPSender and RTCPSender, pushing an event log pointer from
Channel through ModuleRtpRtcpImpl to the Sender objects.

BUG=webrtc:4741

Committed: https://crrev.com/429c345b0297d14211c3e14c907d4e5aac726590
Cr-Commit-Position: refs/heads/master@{#11336}
==========

[commit-bot: I haz the power, 2016-01-21 13:42:16]

Patchset 10 (id:??) landed as
https://crrev.com/429c345b0297d14211c3e14c907d4e5aac726590
Cr-Commit-Position: refs/heads/master@{#11336}