Store/restore RTP state for audio streams with same SSRC within a call

Store/restore RTP state for audio streams with same SSRC within a call

This functionality already exists for video streams, so not having it
for audio is unexpected and has lead to problems.

BUG=webrtc:7631
Review-Url: https://codereview.webrtc.org/2887733002
Cr-Commit-Position: refs/heads/master@{#18231}
Committed: https://chromium.googlesource.com/external/webrtc/+/c3d4b48e7e87a3d0ed0a6444cd7f15fa6ef622c7

[ossu, 2017-05-16 14:33:04]

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

[commit-bot: I haz the power, 2017-05-16 14:33:13]

Dry run: 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, 2017-05-16 14:35:21]

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

[commit-bot: I haz the power, 2017-05-16 14:35:21]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21509)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25680)

[ossu, 2017-05-16 14:53:00]

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

[commit-bot: I haz the power, 2017-05-16 14:53:01]

Dry run: 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, 2017-05-16 15:15:48]

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

[commit-bot: I haz the power, 2017-05-16 15:15:49]

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

[ossu, 2017-05-17 12:24:35]

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

[commit-bot: I haz the power, 2017-05-17 12:24:41]

Dry run: 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, 2017-05-17 12:56:50]

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

[commit-bot: I haz the power, 2017-05-17 12:56:51]

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

[ossu, 2017-05-17 13:10:53]

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

[commit-bot: I haz the power, 2017-05-17 13:11:01]

Dry run: 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, 2017-05-17 13:35:10]

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

[commit-bot: I haz the power, 2017-05-17 13:35:11]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-05-17 13:56:45]

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

[ossu, 2017-05-17 13:56:46]

PTAL.

To elaborate on the last patchset: valgrind found that the "uninteresting mock"
printouts were accessing uninitialized data (for some reason), so I turned them
into NiceMocks. It's also nice (no pun intended) to not have to get all those
warnings printed out.

[ossu, 2017-05-18 11:00:55]

ossu@webrtc.org changed reviewers:
+ holmer@chromium.org

[ossu, 2017-05-18 11:00:57]

+stefan to have a look at this one as well.

Trying to match the RTP state restoration functionality that the video side
provides. I believe you reviewed that code at one point. I'm simply getting and
setting the state on the RtpRtcp module. Know of any dark corners I might be
missing that would stop this from working?

[kwiberg-webrtc, 2017-05-19 01:05:10]

lgtm, but wait for Stefan's opinion too

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.h:48: const rtc::Optional<RtpState>&
suspended_rtp_state);
const RtpState* ? That way, you don't force the caller to have an Optional.

Or are all callers expected to have an Optional<RtpState> already?

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:254: typedef std::map<uint32_t, RtpState> RtpStateMap;
using?

[ossu, 2017-05-22 08:46:40]

Friendly ping at stefan@. :)

I need to get this into M60, so before Wednesday. If you're overloaded or if
I've misidentified you for this review, speak up and I'll try to find someone
else to do it.

[ossu, 2017-05-22 14:32:49]

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

[ossu, 2017-05-22 14:32:50]

Hello pbos@,

I believe you implemented this functionality on the video side of things. Just
want someone familiar with saving and restoring the RTP state to have a look at
it. Wanting to get this in before M60.

Thanks,

[pbos-webrtc, 2017-05-22 15:39:50]

lgtm, but please fix my comment. :)

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:255: RtpStateMap suspended_audio_send_ssrcs_;
Can you do GUARDED_BY (configuration_thread_checker_) for these? I recall us
having added thread checker static validation, which is useful now since this is
racy if DestroyAudioSendStream is ever called concurrently. GUARDED_BY enforces
that through the thread check.

Then replace CalledOnValidThread below with invocations of RTC_DCHECK_RUN_ON

[pbos-webrtc, 2017-05-22 15:40:18]

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#oldco...
webrtc/call/call.cc:254: VideoSendStream::RtpStateMap
suspended_video_send_ssrcs_;
Does this mean that there's some VideoSendStream::RtpStateMap that should be
removed?

[ossu, 2017-05-22 17:14:00]

Thanks for your comments, both of you! I'll put up a new patch set shortly.

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.h:48: const rtc::Optional<RtpState>&
suspended_rtp_state);
On 2017/05/19 01:05:09, kwiberg-webrtc wrote:
> const RtpState* ? That way, you don't force the caller to have an Optional.
> 
> Or are all callers expected to have an Optional<RtpState> already?

I think the Optional better communicates the usage, so I'll keep it for now,

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (left):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#oldco...
webrtc/call/call.cc:254: VideoSendStream::RtpStateMap
suspended_video_send_ssrcs_;
On 2017/05/22 15:40:18, pbos-webrtc wrote:
> Does this mean that there's some VideoSendStream::RtpStateMap that should be
> removed?

I chose to let RtpStateMap be a shorthand for std::map<uint32_t, RtpState> in
both Call and VideoSendStream, rather than find a common place to put it.

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:254: typedef std::map<uint32_t, RtpState> RtpStateMap;
On 2017/05/19 01:05:09, kwiberg-webrtc wrote:
> using?

Alright!

https://codereview.webrtc.org/2887733002/diff/60001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:255: RtpStateMap suspended_audio_send_ssrcs_;
On 2017/05/22 15:39:50, pbos-webrtc wrote:
> Can you do GUARDED_BY (configuration_thread_checker_) for these? I recall us
> having added thread checker static validation, which is useful now since this
is
> racy if DestroyAudioSendStream is ever called concurrently. GUARDED_BY
enforces
> that through the thread check.
> 
> Then replace CalledOnValidThread below with invocations of RTC_DCHECK_RUN_ON

Alright!

[ossu, 2017-05-23 11:02:42]

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

[commit-bot: I haz the power, 2017-05-23 11:02:48]

Dry run: 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, 2017-05-23 11:05:38]

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

[commit-bot: I haz the power, 2017-05-23 11:05:38]

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

[ossu, 2017-05-23 11:46:33]

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

[commit-bot: I haz the power, 2017-05-23 11:46:44]

Dry run: 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, 2017-05-23 13:01:02]

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

[commit-bot: I haz the power, 2017-05-23 13:01:03]

Dry run: This issue passed the CQ dry run.

[ossu, 2017-05-23 13:05:13]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2017-05-23 13:05:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2887733002/#ps100001 (title:
"Rebasement Jaxx")

[commit-bot: I haz the power, 2017-05-23 13:05:19]

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, 2017-05-23 13:07:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495544713369690,
"parent_rev": "23ac8b49f4f8f5cbf709ddefcb2c721bab2505de", "commit_rev":
"c3d4b48e7e87a3d0ed0a6444cd7f15fa6ef622c7"}

[commit-bot: I haz the power, 2017-05-23 13:07:17]

Description was changed from

==========
Store/restore RTP state for audio streams with same SSRC within a call

This functionality already exists for video streams, so not having it
for audio is unexpected and has lead to problems.

BUG=webrtc:7631
==========

to

==========
Store/restore RTP state for audio streams with same SSRC within a call

This functionality already exists for video streams, so not having it
for audio is unexpected and has lead to problems.

BUG=webrtc:7631

Review-Url: https://codereview.webrtc.org/2887733002
Cr-Commit-Position: refs/heads/master@{#18231}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c3d4b48e7e87a3d0ed0a6444c...
==========

[commit-bot: I haz the power, 2017-05-23 13:07:18]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/c3d4b48e7e87a3d0ed0a6444c...

[stefan-webrtc, 2017-05-23 17:24:34]

On 2017/05/22 08:46:40, ossu wrote:
> Friendly ping at stefan@. :)
> 
> I need to get this into M60, so before Wednesday. If you're overloaded or if
> I've misidentified you for this review, speak up and I'll try to find someone
> else to do it.

Sorry for this, i was a bit overloaded and dropped the ball. Feel free to ping
me on chat next time I'm late responding :)

[ossu, 2017-05-24 09:10:08]

On 2017/05/23 17:24:34, stefan-webrtc wrote:
> On 2017/05/22 08:46:40, ossu wrote:
> > Friendly ping at stefan@. :)
> > 
> > I need to get this into M60, so before Wednesday. If you're overloaded or if
> > I've misidentified you for this review, speak up and I'll try to find
someone
> > else to do it.
> 
> Sorry for this, i was a bit overloaded and dropped the ball. Feel free to ping
> me on chat next time I'm late responding :)

Alright, no worries! :)