Added support for logging the SSRC corresponding to AudioPlayout events.

Added support for logging the SSRC corresponding to AudioPlayout events.

To do this, the logging of this event was moved from the ACM to
VoiceEngine Channel. A new LogAudioPlayoutEvent function was added on
the RtcEventLog interface, and the LogDebugEvent function was removed
since it is no longer being used.

BUG=webrtc:4741
R=henrik.lundin@webrtc.org, henrikg@webrtc.org, kwiberg@webrtc.org, stefan@webrtc.org, terelius@webrtc.org

Committed: https://crrev.com/ae856f2c9fc358e5cd68d8a595136dcef017ed96
Cr-Commit-Position: refs/heads/master@{#9972}

[ivoc, 2015-09-15 11:21:39]

ivoc@webrtc.org changed reviewers:
+ hlundin@google.com, stefan@webrtc.org, terelius@webrtc.org

[ivoc, 2015-09-15 11:21:39]

ivoc@webrtc.org changed required reviewers:
+ hlundin@google.com, stefan@webrtc.org, terelius@webrtc.org

[ivoc, 2015-09-15 11:23:43]

ivoc@webrtc.org changed reviewers:
+ henrikg@webrtc.org

[ivoc, 2015-09-15 11:23:43]

ivoc@webrtc.org changed required reviewers:
+ henrikg@webrtc.org

[ivoc, 2015-09-15 11:38:08]

Hi guys,

Please have a look at this CL to add support for logging the SSRC corresponding
to AudioPlayout events. This will allow us to figure out which AudioPlayout
events correspond to which channel, which will help with debugging. 

Thanks!

[kwiberg-webrtc, 2015-09-15 11:49:41]

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

[kwiberg-webrtc, 2015-09-15 11:49:42]

LGTM for audio_coding

Nice. It looks like you've found a more natural place for this lof message.

[hlundin-webrtc, 2015-09-15 12:19:07]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org
- hlundin@google.com

[hlundin-webrtc, 2015-09-15 12:19:07]

henrik.lundin@webrtc.org changed required reviewers:
+ henrik.lundin@webrtc.org
- hlundin@google.com

[hlundin-webrtc, 2015-09-15 12:30:52]

Good. LGTM with two nits.

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h
File webrtc/video/rtc_event_log.h (right):

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h#...
webrtc/video/rtc_event_log.h:67: // Log an audio playout event
Nit: Logs...

https://codereview.webrtc.org/1340283002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1340283002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:455: GetLocalSSRC(ssrc);
Since GetLocalSSRC has a return value, I'd argue that you CHECK that it always
returns 0. (Currently, it does by definition, but who knows...)

[terelius, 2015-09-15 14:32:29]

lgtm

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h
File webrtc/video/rtc_event_log.h (right):

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h#...
webrtc/video/rtc_event_log.h:68: virtual void LogAudioPlayout(uint32_t ssrc) =
0;
Since there is no more DebugEvent in the C-interface, I think we should remove
the DebugEvent message from the protobuf too. Not sure if that is best done in
this or a separate CL.

[ivoc, 2015-09-15 14:55:16]

Thanks for the quick reviews! I addressed Henrik's comments, and I propose to
address Bjorn's comment in a follow-up CL.

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h
File webrtc/video/rtc_event_log.h (right):

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h#...
webrtc/video/rtc_event_log.h:67: // Log an audio playout event
On 2015/09/15 12:30:52, hlundin-webrtc wrote:
> Nit: Logs...

Done.

https://codereview.webrtc.org/1340283002/diff/1/webrtc/video/rtc_event_log.h#...
webrtc/video/rtc_event_log.h:68: virtual void LogAudioPlayout(uint32_t ssrc) =
0;
On 2015/09/15 14:32:29, terelius wrote:
> Since there is no more DebugEvent in the C-interface, I think we should remove
> the DebugEvent message from the protobuf too. Not sure if that is best done in
> this or a separate CL.

I agree, let's do that in a follow-up CL.

https://codereview.webrtc.org/1340283002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1340283002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:455: GetLocalSSRC(ssrc);
On 2015/09/15 12:30:52, hlundin-webrtc wrote:
> Since GetLocalSSRC has a return value, I'd argue that you CHECK that it always
> returns 0. (Currently, it does by definition, but who knows...)

Done.

[Henrik Grunell WebRTC, 2015-09-16 10:34:54]

voice_engine lgtm.

[stefan-webrtc, 2015-09-17 09:02:26]

Reviewing video/ files.

Are we removing generic debug events for now as we don't see a need for them? We
have been talking about adding events for when the bandwidth estimator decides
to reduce bandwidth, i.e., a bandwidth reduction event with different properties
such as "due to loss", "due to delay"... We can probably add methods for that
when we get there, but I was mostly wondering if we're going to go with specific
methods for each type of event or go back to a generic method later?

[terelius, 2015-09-17 09:50:03]

On 2015/09/17 09:02:26, stefan-webrtc (holmer) wrote:
> Reviewing video/ files.
> 
> Are we removing generic debug events for now as we don't see a need for them?
We
> have been talking about adding events for when the bandwidth estimator decides
> to reduce bandwidth, i.e., a bandwidth reduction event with different
properties
> such as "due to loss", "due to delay"... We can probably add methods for that
> when we get there, but I was mostly wondering if we're going to go with
specific
> methods for each type of event or go back to a generic method later?

Since the different debug events have different parameters/members, it seems
better to make them separate types rather than to try to squeeze them into some
generic debug event type. There is a small caveat; if we are going to add more
than 7 different debug types but they are only used rarely, it might be more
efficient (in terms of storage) to use a generic debug type. The reason is that
the protobuf field numbers use a more compact encoding if they fit into 4 bits.

[stefan-webrtc, 2015-09-17 10:34:40]

On 2015/09/17 09:50:03, terelius wrote:
> On 2015/09/17 09:02:26, stefan-webrtc (holmer) wrote:
> > Reviewing video/ files.
> > 
> > Are we removing generic debug events for now as we don't see a need for
them?
> We
> > have been talking about adding events for when the bandwidth estimator
decides
> > to reduce bandwidth, i.e., a bandwidth reduction event with different
> properties
> > such as "due to loss", "due to delay"... We can probably add methods for
that
> > when we get there, but I was mostly wondering if we're going to go with
> specific
> > methods for each type of event or go back to a generic method later?
> 
> Since the different debug events have different parameters/members, it seems
> better to make them separate types rather than to try to squeeze them into
some
> generic debug event type. There is a small caveat; if we are going to add more
> than 7 different debug types but they are only used rarely, it might be more
> efficient (in terms of storage) to use a generic debug type. The reason is
that
> the protobuf field numbers use a more compact encoding if they fit into 4
bits.

Thanks. Then this lgtm.

[ivoc, 2015-09-17 11:56:18]

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

[commit-bot: I haz the power, 2015-09-17 11:56:25]

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

[commit-bot: I haz the power, 2015-09-17 13:56:46]

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

[commit-bot: I haz the power, 2015-09-17 13:56:47]

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

[ivoc, 2015-09-17 14:34:21]

Committed patchset #3 (id:40001) manually as
ae856f2c9fc358e5cd68d8a595136dcef017ed96 (presubmit successful).

[commit-bot: I haz the power, 2015-09-17 14:34:24]

Patchset 3 (id:??) landed as
https://crrev.com/ae856f2c9fc358e5cd68d8a595136dcef017ed96
Cr-Commit-Position: refs/heads/master@{#9972}