Move responsibility for RTP header extensions on video receive.
Delete VideoReceiveStream::Config::Rtp::extensions and
FlexfecReceiveStream::Config::rtp_header_extensions.
Instead, let WebRtcSession pass the list of extensions to Call, which
is responsible for the parsing.
BUG=webrtc:7135

This is a somewhat experimental cl, attempting to move responsibility for
received rtp header extensions from video receive streams to Call, with the
intention of later moving it one step more, to RtpTransportController. The idea
is that the extensions map really is a transport-level property, not a stream
property.
I've also been considering the alternative of trying to delete
IdentifyExtensions, and not have any central ownership of the extensions map;
instead each piece of code interesting in some extension would need to know its
configured numeric id, and use RtpPacket::GetRawExtension to access it.
One effect is that receive streams are no longer destroyed and recreated if
header extensions are renegotiated mid-stream, which I guess is an improvement.
Lots of unit tests are broken, I don't expect any fundamental difficulties in
fixing them.
Also rtc eventlogs are broken, since they depend on the model that header
extensions are associated with a stream rather than a transport. Probably needs
more care to get right.
I'd also appreciate advice on which order to do things in. This is related to
https://codereview.webrtc.org/2709723003/. I think it is easiest to land the
latter cl first; then the method to set extensions would naturally belong to the
rtp demuxer, one drawback that we probably have to do the corresonding audio
changes in the same cl.
It's also related to my cl https://codereview.webrtc.org/2794943002/, which
atttempts to delete MediaController. If that is landed first, the WebRtcSession
--> Call signaling in this cl wouldn't need the ugly detour via MediaController.

brandtr

Some quick initial feedback. On 2017/04/21 12:14:37, nisse-webrtc wrote: > This is a somewhat experimental ...

Some quick initial feedback.
On 2017/04/21 12:14:37, nisse-webrtc wrote:
> This is a somewhat experimental cl, attempting to move responsibility for
> received rtp header extensions from video receive streams to Call, with the
> intention of later moving it one step more, to RtpTransportController. The
idea
> is that the extensions map really is a transport-level property, not a stream
> property.
I think this is a good step in the right direction -- the extension maps do not
belong in the streams. But when you say that the extensions map is
"transport-level", how does that work with BUNDLE and a=extmap's on different
"m=" lines? Or is it guaranteed that the a=extmap's always have different id's
on different "m=" lines, when BUNDLE is used?
>
> I've also been considering the alternative of trying to delete
> IdentifyExtensions, and not have any central ownership of the extensions map;
> instead each piece of code interesting in some extension would need to know
its
> configured numeric id, and use RtpPacket::GetRawExtension to access it.
>
> One effect is that receive streams are no longer destroyed and recreated if
> header extensions are renegotiated mid-stream, which I guess is an
improvement.
As long as the locking doesn't introduce any problems, this is definitely a win.
> Lots of unit tests are broken, I don't expect any fundamental difficulties in
> fixing them.
>
> Also rtc eventlogs are broken, since they depend on the model that header
> extensions are associated with a stream rather than a transport. Probably
needs
> more care to get right.
>
> I'd also appreciate advice on which order to do things in. This is related to
> https://codereview.webrtc.org/2709723003/. I think it is easiest to land the
> latter cl first; then the method to set extensions would naturally belong to
the
> rtp demuxer, one drawback that we probably have to do the corresonding audio
> changes in the same cl.
>
> It's also related to my cl https://codereview.webrtc.org/2794943002/, which
> atttempts to delete MediaController. If that is landed first, the
WebRtcSession
> --> Call signaling in this cl wouldn't need the ugly detour via
MediaController.

nisse-webrtc

On 2017/04/27 10:58:30, brandtr wrote: > I think this is a good step in the ...

On 2017/04/27 10:58:30, brandtr wrote:
> I think this is a good step in the right direction -- the extension maps do
not
> belong in the streams. But when you say that the extensions map is
> "transport-level", how does that work with BUNDLE and a=extmap's on different
> "m=" lines? Or is it guaranteed that the a=extmap's always have different id's
> on different "m=" lines, when BUNDLE is used?
My understanding is that different m=lines bundled on the same transport can't
reuse the same
extension id with different meaning.
I'd hope some RTP expert can confirm...
At least, the mid extension (which can tell the receiver which m-line an
unsignalled ssrc belongs to) ought to have the same numeric id for all m-lines.

On 2017/04/27 10:58:30, brandtr wrote:
> Some quick initial feedback.
>
> On 2017/04/21 12:14:37, nisse-webrtc wrote:
> > is that the extensions map really is a transport-level property, not a
stream
> > property.
>
> I think this is a good step in the right direction -- the extension maps do
not
> belong in the streams. But when you say that the extensions map is
> "transport-level", how does that work with BUNDLE and a=extmap's on different
> "m=" lines? Or is it guaranteed that the a=extmap's always have different id's
> on different "m=" lines, when BUNDLE is used?
>
Spec https://tools.ietf.org/html/rfc5285#section-4.1
mention "Each distinct extension MUST have a unique ID."
I interpret it as same ids on different mlines must map to same extension.

On 2017/04/27 11:17:39, nisse-webrtc wrote:
> On 2017/04/27 10:58:30, brandtr wrote:
>
> > I think this is a good step in the right direction -- the extension maps do
> not
> > belong in the streams. But when you say that the extensions map is
> > "transport-level", how does that work with BUNDLE and a=extmap's on
different
> > "m=" lines? Or is it guaranteed that the a=extmap's always have different
id's
> > on different "m=" lines, when BUNDLE is used?
>
> My understanding is that different m=lines bundled on the same transport can't
> reuse the same
> extension id with different meaning.
> I'd hope some RTP expert can confirm...
>
> At least, the mid extension (which can tell the receiver which m-line an
> unsignalled ssrc belongs to) ought to have the same numeric id for all
m-lines.
It wasn't entirely clear from the spec, but I believe Taylor went and chased it
down to make sure it was true. And even if the spec technically said you could
have different IDs, everyone always makes them the same when bundling, so I
think it's fine to reject situations where they aren't the same.

On 2017/04/29 00:17:01, pthatcher1 wrote:
> On 2017/04/27 11:17:39, nisse-webrtc wrote:
> > On 2017/04/27 10:58:30, brandtr wrote:
> >
> > > I think this is a good step in the right direction -- the extension maps
do
> > not
> > > belong in the streams. But when you say that the extensions map is
> > > "transport-level", how does that work with BUNDLE and a=extmap's on
> different
> > > "m=" lines? Or is it guaranteed that the a=extmap's always have different
> id's
> > > on different "m=" lines, when BUNDLE is used?
> >
> > My understanding is that different m=lines bundled on the same transport
can't
> > reuse the same
> > extension id with different meaning.
> > I'd hope some RTP expert can confirm...
> >
> > At least, the mid extension (which can tell the receiver which m-line an
> > unsignalled ssrc belongs to) ought to have the same numeric id for all
> m-lines.
>
> It wasn't entirely clear from the spec, but I believe Taylor went and chased
it
> down to make sure it was true. And even if the spec technically said you
could
> have different IDs, everyone always makes them the same when bundling, so I
> think it's fine to reject situations where they aren't the same.
By the way, it's not true when bundle is not used.

Description was changed from
==========
Move responsibility for RTP header extensions on video receive.
Delete VideoReceiveStream::Config::Rtp::extensions and
FlexfecReceiveStream::Config::rtp_header_extensions.
Instead, let WebRtcSession pass the list of extensions to Call, which
is responsible for the parsing.
BUG=webrtc:7135
==========
to
==========
Move responsibility for RTP header extensions on video receive.
Delete VideoReceiveStream::Config::Rtp::extensions and
FlexfecReceiveStream::Config::rtp_header_extensions.
Instead, let WebRtcSession pass the list of extensions to Call, which
is responsible for the parsing.
BUG=webrtc:7135
==========