Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2826263004: Move responsibility for RTP header extensions on video receive. (Closed)

Created:
7 months, 1 week ago by nisse-webrtc
Modified:
1 month, 3 weeks ago
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Crude rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -64 lines) Patch
M call/bitrate_estimator_tests.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M call/call.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M call/call.cc View 1 11 chunks +32 lines, -23 lines 0 comments Download
M call/flexfec_receive_stream.h View 1 1 chunk +0 lines, -3 lines 0 comments Download
M call/flexfec_receive_stream_impl.cc View 1 1 chunk +1 line, -7 lines 0 comments Download
M call/flexfec_receive_stream_unittest.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M call/rampup_tests.cc View 1 2 chunks +7 lines, -1 line 0 comments Download
M call/video_receive_stream.h View 1 1 chunk +0 lines, -3 lines 0 comments Download
M call/video_receive_stream.cc View 1 1 chunk +0 lines, -7 lines 0 comments Download
M logging/rtc_event_log/rtc_event_log_parser.cc View 1 1 chunk +2 lines, -1 line 0 comments Download
M logging/rtc_event_log/rtc_event_log_unittest.cc View 1 2 chunks +2 lines, -0 lines 0 comments Download
M logging/rtc_event_log/rtc_event_log_unittest_helper.cc View 1 2 chunks +2 lines, -0 lines 0 comments Download
M media/base/mediachannel.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M media/engine/fakewebrtccall.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M media/engine/fakewebrtccall.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download
M media/engine/webrtcvideoengine.cc View 1 3 chunks +0 lines, -9 lines 0 comments Download
M media/engine/webrtcvideoengine_unittest.cc View 1 15 chunks +24 lines, -9 lines 0 comments Download
M pc/webrtcsession.cc View 1 1 chunk +15 lines, -0 lines 0 comments Download
M test/call_test.cc View 1 2 chunks +4 lines, -0 lines 0 comments Download
M video/end_to_end_tests.cc View 1 4 chunks +8 lines, -1 line 0 comments Download
M video/rtp_video_stream_receiver.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M video/video_quality_test.cc View 1 3 chunks +4 lines, -0 lines 0 comments Download
M video/video_send_stream_tests.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
Trybot results:

Messages

Total messages: 10 (3 generated)
nisse-webrtc
This is a somewhat experimental cl, attempting to move responsibility for received rtp header extensions ...
7 months, 1 week ago (2017-04-21 12:14:37 UTC) #2
brandtr
Some quick initial feedback. On 2017/04/21 12:14:37, nisse-webrtc wrote: > This is a somewhat experimental ...
7 months ago (2017-04-27 10:58:30 UTC) #3
nisse-webrtc
On 2017/04/27 10:58:30, brandtr wrote: > I think this is a good step in the ...
7 months ago (2017-04-27 11:17:39 UTC) #4
danilchap
On 2017/04/27 10:58:30, brandtr wrote: > Some quick initial feedback. > > On 2017/04/21 12:14:37, ...
7 months ago (2017-04-27 11:20:22 UTC) #5
pthatcher1
On 2017/04/27 11:17:39, nisse-webrtc wrote: > On 2017/04/27 10:58:30, brandtr wrote: > > > I ...
6 months, 4 weeks ago (2017-04-29 00:17:01 UTC) #6
pthatcher1
https://codereview.webrtc.org/2826263004/diff/1/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2826263004/diff/1/webrtc/call/call.h#newcode106 webrtc/call/call.h:106: const std::vector<RtpExtension>& extensions) = 0; It makes sense for ...
6 months, 4 weeks ago (2017-04-29 00:25:19 UTC) #7
pthatcher1
6 months, 4 weeks ago (2017-04-29 00:25:40 UTC) #8
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.

Powered by Google App Engine
This is Rietveld efc10ee0f