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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by nisse-webrtc
Modified:
4 weeks, 1 day 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -67 lines) Patch
M webrtc/call/bitrate_estimator_tests.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/call/call.h View 1 chunk +5 lines, -0 lines 1 comment Download
M webrtc/call/call.cc View 12 chunks +34 lines, -20 lines 0 comments Download
M webrtc/call/flexfec_receive_stream.h View 1 chunk +0 lines, -3 lines 0 comments Download
M webrtc/call/flexfec_receive_stream_impl.cc View 1 chunk +1 line, -7 lines 0 comments Download
M webrtc/call/flexfec_receive_stream_unittest.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/call/rampup_tests.cc View 2 chunks +7 lines, -1 line 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log_parser.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log_unittest.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M webrtc/logging/rtc_event_log/rtc_event_log_unittest_helper.cc View 4 chunks +4 lines, -0 lines 0 comments Download
M webrtc/media/base/mediachannel.h View 1 chunk +2 lines, -0 lines 1 comment Download
M webrtc/media/engine/fakewebrtccall.h View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/media/engine/fakewebrtccall.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.cc View 3 chunks +0 lines, -8 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2_unittest.cc View 15 chunks +24 lines, -14 lines 0 comments Download
M webrtc/pc/fakemediacontroller.h View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/pc/mediacontroller.h View 2 chunks +5 lines, -0 lines 1 comment Download
M webrtc/pc/mediacontroller.cc View 1 chunk +14 lines, -0 lines 0 comments Download
M webrtc/pc/webrtcsession.cc View 1 chunk +10 lines, -0 lines 0 comments Download
M webrtc/test/call_test.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M webrtc/tools/event_log_visualizer/analyzer.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/video/end_to_end_tests.cc View 2 chunks +4 lines, -1 line 0 comments Download
M webrtc/video/rtp_stream_receiver.h View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/video/rtp_stream_receiver.cc View 3 chunks +4 lines, -1 line 0 comments Download
M webrtc/video/video_quality_test.cc View 3 chunks +4 lines, -0 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 chunk +1 line, -9 lines 0 comments Download
M webrtc/video/video_send_stream_tests.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/video_receive_stream.h View 1 chunk +0 lines, -3 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 10 (3 generated)
nisse-webrtc
This is a somewhat experimental cl, attempting to move responsibility for received rtp header extensions ...
1 month, 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 ...
1 month 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 ...
1 month 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, ...
1 month 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 ...
1 month 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 ...
4 weeks, 1 day ago (2017-04-29 00:25:19 UTC) #7
pthatcher1
4 weeks, 1 day 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06