Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388303009:
Integrate FlexfecReceiveStream with Call. (Closed)
|
Created:
4 years, 2 months ago by brandtr Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIntegrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
Committed: https://crrev.com/25445d3d4bc6977279451eb48810b5788071db9e
Cr-Commit-Position: refs/heads/master@{#14727}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback response 1. #
Total comments: 15
Patch Set 3 : Feedback response 2. #Patch Set 4 : Rebase. #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 29 (13 generated)
Patchset #1 (id:1) has been deleted
Description was changed from
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
==========
to
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
==========
brandtr@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org
Patchset #1 (id:20001) has been deleted
LG https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:625: // Remove all ssrcs pointing to a receive stream. "...pointing to the FlexFec receive stream to be destroyed", right? https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:650: flexfec_receive_streams_.erase(receive_stream_impl); If flexfec_receive_streams_ own the receive stream and has unique_ptrs then I think we can remove the delete below, or am I missing something?
Patchset #2 (id:60001) has been deleted
Please have a look :) holmer/philipel: general review (thanks holmer, for initial review!) terelius: use of RtcEventLog in Call::DeliverRtp. https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:625: // Remove all ssrcs pointing to a receive stream. On 2016/10/18 18:13:29, stefan-webrtc (holmer) wrote: > "...pointing to the FlexFec receive stream to be destroyed", right? Yep, fixed. https://codereview.webrtc.org/2388303009/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:650: flexfec_receive_streams_.erase(receive_stream_impl); On 2016/10/18 18:13:29, stefan-webrtc (holmer) wrote: > If flexfec_receive_streams_ own the receive stream and has unique_ptrs then I > think we can remove the delete below, or am I missing something? Yes, but Call will not own the created FlexfecReceiveStream. This is the same as how VideoReceiveStream is handled; it is owned by WebRtcVideoChannel2, but the Call functions are responsible for creating and destroying the objects. See e.g.: https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=518 https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=546 https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... I think it would make sense to change all of the streams to use unique_ptr's, in order to make the ownerships clear, but that is probably better to do in a another CL.
https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1019: event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); Where are the recovered packets sent by the flexfec stream? Is there any risk that we log the "same" packet multiple times? E.g. suppose that the sender sends packets A, B, C and a FEC packet D. If the receiver gets A, B and D, will it log A, B, D or A, B, D, C?
https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1019: event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); On 2016/10/19 09:48:51, terelius wrote: > Where are the recovered packets sent by the flexfec stream? Is there any risk > that we log the "same" packet multiple times? E.g. suppose that the sender sends > packets A, B, C and a FEC packet D. If the receiver gets A, B and D, will it log > A, B, D or A, B, D, C? When FlexFEC recovers a media packet, it is returned to the Call layer, which then propagates it down to the OnRecoveredPacket method of the appropriate RtpStreamReceiver. (See https://codereview.webrtc.org/2390823009/.) As far as I can tell, from there on the recovered packet will never enter the Call layer again, i.e. the recovered packet will not be logged to the RtcEventLog. (Do we log packets to the event log anywhere else?) So the answer is, the receiver will log packets A, B, D.
https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:172: std::multimap<uint32_t, FlexfecReceiveStream*> flexfec_receive_ssrcs_media_ Add comment about multiple flex fec streams (possibly) protecting the same media stream. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:622: FlexfecReceiveStream* receive_stream_impl = nullptr; |receive_stream| is the |receive_stream_impl| right? I can't see why you need to "find" the |receive_stream_impl|. Looking at the DCHECK (631) and assignment (632) that must be true right? Remove this variable. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:633: flexfec_receive_ssrcs_media_.erase(media_it++); media_it = flexfec_receive_ssrcs_media_.erase(media_it); same for loop below. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:650: flexfec_receive_streams_.erase(receive_stream_impl); erase(receive_stream) https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1001: auto flexfec_lower_it = flexfec_receive_ssrcs_media_.lower_bound(ssrc); Use equal_range instead: http://en.cppreference.com/w/cpp/container/multimap/equal_range https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1007: if (status == DELIVERY_OK) if (it->second->DeliverRtp(packet, length, packet_time))) { event_log_... return DELIVERY_OK; } Same for line 1018.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:172: std::multimap<uint32_t, FlexfecReceiveStream*> flexfec_receive_ssrcs_media_ On 2016/10/19 12:15:34, philipel wrote: > Add comment about multiple flex fec streams (possibly) protecting the same media > stream. Done. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:622: FlexfecReceiveStream* receive_stream_impl = nullptr; On 2016/10/19 12:15:34, philipel wrote: > |receive_stream| is the |receive_stream_impl| right? I can't see why you need to > "find" the |receive_stream_impl|. > > Looking at the DCHECK (631) and assignment (632) that must be true right? > > Remove this variable. They point to the same object, except that the former points to the base class whereas the latter points to the specific class. Based on offline discussions, I'm keeping this variable since I need to refer to the specific type below. The unnecessary DCHECKs have been removed however. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:633: flexfec_receive_ssrcs_media_.erase(media_it++); On 2016/10/19 12:15:34, philipel wrote: > media_it = flexfec_receive_ssrcs_media_.erase(media_it); > > same for loop below. Done. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:650: flexfec_receive_streams_.erase(receive_stream_impl); On 2016/10/19 12:15:34, philipel wrote: > erase(receive_stream) The containers store the specific type pointers, so need to |receive_stream_impl| here here. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1001: auto flexfec_lower_it = flexfec_receive_ssrcs_media_.lower_bound(ssrc); On 2016/10/19 12:15:34, philipel wrote: > Use equal_range instead: > http://en.cppreference.com/w/cpp/container/multimap/equal_range Done. https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1007: if (status == DELIVERY_OK) On 2016/10/19 12:15:34, philipel wrote: > if (it->second->DeliverRtp(packet, length, packet_time))) { > event_log_... > return DELIVERY_OK; > } > > Same for line 1018. Kept for symmetry with audio/video parts (see above and below). I prefer not to make changes in those parts.
RtcEventLog lgtm
lgtm https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/80001/webrtc/call/call.cc#newco... webrtc/call/call.cc:1007: if (status == DELIVERY_OK) On 2016/10/19 13:53:25, brandtr wrote: > On 2016/10/19 12:15:34, philipel wrote: > > if (it->second->DeliverRtp(packet, length, packet_time))) { > > event_log_... > > return DELIVERY_OK; > > } > > > > Same for line 1018. > > Kept for symmetry with audio/video parts (see above and below). I prefer not to > make changes in those parts. Acknowledged.
Rebase.
Lgtm % nit https://codereview.webrtc.org/2388303009/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2388303009/diff/180001/webrtc/call/call.cc#newc... webrtc/call/call.cc:641: if (prot_it->second == receive_stream_impl) {}, here and above
https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc#ne... webrtc/call/call.cc:641: if (prot_it->second == receive_stream_impl) On 2016/10/20 16:23:26, stefan-webrtc (holmer) wrote: > {}, here and above While I personally like the braces for single-line statements, adding them here does not conform with the local style in this .cc-file, see: https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=131 If I interpret the style guide correctly (see https://google.github.io/styleguide/cppguide.html#Conditionals), it's fine to not have braces when you have single-line if/else statements. But if either of the if or else clauses use braces, the other clause should too. If you still believe I should add the braces, I'll add them on L131 too :)
https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc#ne... webrtc/call/call.cc:641: if (prot_it->second == receive_stream_impl) On 2016/10/21 08:40:26, brandtr wrote: > On 2016/10/20 16:23:26, stefan-webrtc (holmer) wrote: > > {}, here and above > > While I personally like the braces for single-line statements, adding them here > does not conform with the local style in this .cc-file, see: > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=131 > > If I interpret the style guide correctly (see > https://google.github.io/styleguide/cppguide.html#Conditionals), it's fine to > not have braces when you have single-line if/else statements. But if either of > the if or else clauses use braces, the other clause should too. > > If you still believe I should add the braces, I'll add them on L131 too :) My impression has always been that an if-else is a multi-line if statement. The guide isn't very clear about it, so perhaps go with what's common in the repo? :) Whatever you decide, lgtm
https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.chromium.org/2388303009/diff/180001/webrtc/call/call.cc#ne... webrtc/call/call.cc:641: if (prot_it->second == receive_stream_impl) On 2016/10/21 11:07:35, stefan-webrtc (holmer) wrote: > On 2016/10/21 08:40:26, brandtr wrote: > > On 2016/10/20 16:23:26, stefan-webrtc (holmer) wrote: > > > {}, here and above > > > > While I personally like the braces for single-line statements, adding them > here > > does not conform with the local style in this .cc-file, see: > > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=131 > > > > If I interpret the style guide correctly (see > > https://google.github.io/styleguide/cppguide.html#Conditionals), it's fine to > > not have braces when you have single-line if/else statements. But if either of > > the if or else clauses use braces, the other clause should too. > > > > If you still believe I should add the braces, I'll add them on L131 too :) > > My impression has always been that an if-else is a multi-line if statement. The > guide isn't very clear about it, so perhaps go with what's common in the repo? > :) > > Whatever you decide, lgtm Seems fairly common with if/else without braces: https://cs.chromium.org/search/?q=%22++else%22+file:%5Esrc/third_party/webrtc..., so I'll go with that :)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.chromium.org/2388303009/#ps180001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
==========
to
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001)
Message was sent while issue was closed.
Description was changed from
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
==========
to
==========
Integrate FlexfecReceiveStream with Call.
Call demultiplexes received RTP packets and delivers these to the
appropriate {Video,Flexfec}ReceiveStreams. A single video stream
could conceivably be protected by multiple FlexFEC streams.
BUG=webrtc:5654
Committed: https://crrev.com/25445d3d4bc6977279451eb48810b5788071db9e
Cr-Commit-Position: refs/heads/master@{#14727}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/25445d3d4bc6977279451eb48810b5788071db9e Cr-Commit-Position: refs/heads/master@{#14727} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
