Change default SSRC for RTCP receiver reports to not collide with video.

Change default SSRC for RTCP receiver reports to not collide with video.

BUG=chromium:547661
TBR=pthatcher@webrtc.org

Committed: https://crrev.com/8093d5442e4c365bfebc07abcf5fb653bd7a1d57
Cr-Commit-Position: refs/heads/master@{#10621}

[the sun, 2015-11-12 10:47:29]

solenberg@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pbos@webrtc.org

[the sun, 2015-11-12 10:47:30]

PTAL, this needs landing before today's roll to chromium.

[the sun, 2015-11-12 10:48:12]

Description was changed from

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=547661
==========

to

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=547661
==========

[pbos-webrtc, 2015-11-12 10:53:36]

On 2015/11/12 10:47:30, the sun wrote:
> PTAL, this needs landing before today's roll to chromium.

This requires bundlefilter to be updated though? It only lets SSRC=1 through as
a special case?

[the sun, 2015-11-12 10:54:48]

On 2015/11/12 10:53:36, pbos-webrtc wrote:
> On 2015/11/12 10:47:30, the sun wrote:
> > PTAL, this needs landing before today's roll to chromium.
> 
> This requires bundlefilter to be updated though? It only lets SSRC=1 through
as
> a special case?

Not for the system to work as before, but yes, BundleFilter needs fixing as
well. See https://code.google.com/p/chromium/issues/detail?id=547661

[pbos-webrtc, 2015-11-12 10:59:27]

On 2015/11/12 10:54:48, the sun wrote:
> On 2015/11/12 10:53:36, pbos-webrtc wrote:
> > On 2015/11/12 10:47:30, the sun wrote:
> > > PTAL, this needs landing before today's roll to chromium.
> > 
> > This requires bundlefilter to be updated though? It only lets SSRC=1 through
> as
> > a special case?
> 
> Not for the system to work as before, but yes, BundleFilter needs fixing as
> well. See https://code.google.com/p/chromium/issues/detail?id=547661

If this gets stuck in BundleFilter (and dropped), what's the point of fixing
what SSRC it contains? Decryption failure vs. dropped in BundleFilter, isn't
that equivalent from a "no packets arriving" perspective? Am I misunderstanding
something.

I'm also wondering whether you need to fix:
https://bugs.chromium.org/p/webrtc/issues/detail?id=4883#c9 but for audio
(switching the receiver SSRC when removing senders, including back to SSRC=2
when all senders are gone).

[pbos-webrtc, 2015-11-12 12:02:47]

Description was changed from

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=547661
==========

to

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=chromium:547661
==========

[the sun, 2015-11-12 12:27:42]

solenberg@webrtc.org changed reviewers:
+ hta@webrtc.org

[hta-webrtc, 2015-11-12 12:41:19]

LGTM.

This is the wrong long-term fix. But the fix itself does not increase our
technical debt.

The real fix is to make recipient of RTCP accept all RTCP packets independent of
SSRC, and figure out from the SSRCs being reported on (within the packet) where
they're supposed to go.

Also, RTCP with Bundle should report on both audio and video streams, not
separate reports for each.

[the sun, 2015-11-12 12:44:37]

On 2015/11/12 10:59:27, pbos-webrtc wrote:
> On 2015/11/12 10:54:48, the sun wrote:
> > On 2015/11/12 10:53:36, pbos-webrtc wrote:
> > > On 2015/11/12 10:47:30, the sun wrote:
> > > > PTAL, this needs landing before today's roll to chromium.
> > > 
> > > This requires bundlefilter to be updated though? It only lets SSRC=1
through
> > as
> > > a special case?
> > 
> > Not for the system to work as before, but yes, BundleFilter needs fixing as
> > well. See https://code.google.com/p/chromium/issues/detail?id=547661
> 
> If this gets stuck in BundleFilter (and dropped), what's the point of fixing
> what SSRC it contains? Decryption failure vs. dropped in BundleFilter, isn't
> that equivalent from a "no packets arriving" perspective? Am I
misunderstanding
> something.

The point is it won't get stuck in BundleFilter since in the test case where the
issue was discovered, we have no receive streams at the sender (BundleFilter
doesn't filter RTCP then).

> I'm also wondering whether you need to fix:
> https://bugs.chromium.org/p/webrtc/issues/detail?id=4883#c9 but for audio
> (switching the receiver SSRC when removing senders, including back to SSRC=2
> when all senders are gone).

It is true that we'll keep sending RRs on a stale SSRC, however that is still
nothing new and should be fixed elsewhere.

[pbos-webrtc, 2015-11-12 12:48:12]

https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in the
RTP/RTCP module.
Why in the RTP/RTCP module?

[the sun, 2015-11-12 12:48:40]

On 2015/11/12 12:48:12, pbos-webrtc wrote:
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in
the
> RTP/RTCP module.
> Why in the RTP/RTCP module?

See Harald's comment.

[pbos-webrtc, 2015-11-12 12:50:05]

On 2015/11/12 12:48:40, the sun wrote:
> On 2015/11/12 12:48:12, pbos-webrtc wrote:
> >
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> > File talk/media/webrtc/webrtcvoiceengine.h (right):
> > 
> >
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> > talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed in
> the
> > RTP/RTCP module.
> > Why in the RTP/RTCP module?
> 
> See Harald's comment.

Probably means that RTCP should be aggregated across Call, can we put that there
instead of within which module the work should be done? We have more than one
RTP/RTCP module, hence the confusion.

[the sun, 2015-11-12 12:50:59]

solenberg@webrtc.org changed reviewers:
- mflodman@webrtc.org

[the sun, 2015-11-12 12:52:40]

On 2015/11/12 12:50:05, pbos-webrtc wrote:
> On 2015/11/12 12:48:40, the sun wrote:
> > On 2015/11/12 12:48:12, pbos-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> > > File talk/media/webrtc/webrtcvoiceengine.h (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1438183002/diff/20001/talk/media/webrtc/webrtcv...
> > > talk/media/webrtc/webrtcvoiceengine.h:321: // This should really be fixed
in
> > the
> > > RTP/RTCP module.
> > > Why in the RTP/RTCP module?
> > 
> > See Harald's comment.
> 
> Probably means that RTCP should be aggregated across Call, can we put that
there
> instead of within which module the work should be done? We have more than one
> RTP/RTCP module, hence the confusion.

There, removed the comment. Linking to the bugs should be enough.

[pbos-webrtc, 2015-11-12 12:55:23]

lgtm, please tbr pthatcher so he's aware of it

[the sun, 2015-11-12 12:56:01]

Description was changed from

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=chromium:547661
==========

to

==========
Change default SSRC for RTCP receiver reports to not collide with video.

BUG=chromium:547661
TBR=pthatcher@webrtc.org
==========

[the sun, 2015-11-12 12:56:08]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-11-12 12:56:08]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1438183002/#ps40001 (title:
"removed comment")

[commit-bot: I haz the power, 2015-11-12 12:56:18]

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

[pbos-webrtc, 2015-11-12 12:58:50]

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

[commit-bot: I haz the power, 2015-11-12 14:02:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-12 14:02:42]

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

[pthatcher1, 2015-11-12 20:46:40]

https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:321: uint32_t receiver_reports_ssrc_ =
0xFA17FA17u;
Sorry for being late to the bug.

Changing the RR SSRC to something other than 1 may cause RRs to not be received
if the SSRCs aren't signalled because the BundleFilter won't know what to do
with them.  I see there is a CL for updating the BundleFilter to let in all RRs,
regardless of SSRC=1, but that's not submitted yet.  Until it has been submitted
and rolled out for a while, I think the fix here to change the SSRC is risky.

[the sun, 2015-11-13 09:31:21]

On 2015/11/12 20:46:40, pthatcher1 wrote:
>
https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1438183002/diff/40001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.h:321: uint32_t receiver_reports_ssrc_ =
> 0xFA17FA17u;
> Sorry for being late to the bug.
> 
> Changing the RR SSRC to something other than 1 may cause RRs to not be
received
> if the SSRCs aren't signalled because the BundleFilter won't know what to do
> with them.  I see there is a CL for updating the BundleFilter to let in all
RRs,
> regardless of SSRC=1, but that's not submitted yet.  Until it has been
submitted
> and rolled out for a while, I think the fix here to change the SSRC is risky.

Back 3 weeks ago, when we had the default send channel in WVoMC, a *random* SSRC
was picked for RRs in the case we're addressing in this isssue. That case being
1 audio/video sender, 1 audio/video receiver. Since there are no streams in the
opposite direction, the sender's BundleFilter has not been populated with any
receive streams and therefore will let through all RTCP.

So the fix provided avoids the SRTCP index collision and allows RRs to flow *as
before*. The result can be seen clearly here:
https://chromeperf.appspot.com/group_report?bug_id=555023, i.e. the stats are
back to nominal levels.

I realize that in a more realistic use case, we still have the same problem as
before: audio RRs will be dropped by BundleFilter (until the mentioned CL is
landed).