Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511703002:
Wire up FlexFEC in VideoEngine2. (Closed)
|
Created:
4 years, 1 month ago by brandtr Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, the sun, perkj_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWire up FlexFEC in VideoEngine2.
This CL interfaces the SDP information (payload types and
SSRCs) about FlexFEC with the corresponding configs at the
Call layer. It also adds a field trial, which when active
will expose FlexFEC in the default codec list, thus showing
up in the default SDP.
BUG=webrtc:5654
R=magjed@webrtc.org, stefan@webrtc.org
CC=perkj@webrtc.org
Committed: https://crrev.com/468da7c0747e078f689eb02cd86ec31cdb5be903
Cr-Commit-Position: refs/heads/master@{#15184}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Feedback response 1. #Patch Set 3 : Feedback response 2. #Patch Set 4 : Rebase on top of magjed's CL + corresponding fixes. #
Total comments: 13
Patch Set 5 : Rebase on top of relanded CL. #Patch Set 6 : Feedback response 3. #
Total comments: 2
Patch Set 7 : Feedback response 4. #
Total comments: 2
Patch Set 8 : Rebase. #
Messages
Total messages: 30 (12 generated)
Patchset #1 (id:1) has been deleted
[Same message for three related CLs.] Please take a look at this change :) To make it easier to review, I have split a set of related changes into three dependent CLs. The CLs have different reviewers, but you are CC'd on the ones where I didn't set you as a reviewer. Review == holmer: config.h magjed: media/ perkj: pc/ Overview == This set of CLs add support for FlexFEC (see https://tools.ietf.org/html/draft-ietf-payload-flexible-fec-scheme-03) in MediaSession, StreamParams, and VideoEngine2. After these changes, FlexFEC can be exposed in the SDP and the corresponding config will be propagated down to the Call level. This is not turned on by default, but instead hidden between a field trial. Changes == StreamParams: Adds support for the FEC-FR grouping semantics. MediaSession: Whenever FlexFEC is added to the list of codecs, a FlexFEC SSRC will be generated, together with a FEC-FR grouping associating it with the primary SSRC. See https://tools.ietf.org/html/rfc5956 for a description of the FEC-FR grouping semantics. Simulcast is not yet supported. VideoEngine2: Interfaces the SDP information with the Call level objects. The FlexFEC payload type is picked up when the Send/Receive parameters are set. The FlexFEC SSRC is picked up when a Send/Receive stream is added, if the FEC-FR information is provided. The parameters are propagated down to a Call level, where the corresponding VideoSendStream config is updated, and possibly a FlexfecReceiveStream object is created.
lgtm for config.h/.cc, but I have reviewed it all. :) https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:481: ++num_created_receive_streams_; num_created_flexfec_receive_streams_.... https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.h:299: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; std::vector<unique_ptr<...>>? https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:474: flexfec_codec.SetParam(kFlexfecFmtpRepairWindow, "10000000"); Comment on how this window size was chosen. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1136: class WebRtcVideoChannel2FlexfecTest : public WebRtcVideoChannel2Test { Move this down to where it's used.
Patchset #2 (id:40001) has been deleted
Thanks :) https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:481: ++num_created_receive_streams_; On 2016/11/17 12:48:59, stefan-webrtc (holmer) wrote: > num_created_flexfec_receive_streams_.... Don't think so, this variable is shared between video and audio: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/fakewebr... https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/fakewebr... https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.h:299: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; On 2016/11/17 12:48:59, stefan-webrtc (holmer) wrote: > std::vector<unique_ptr<...>>? That would be nice. I'll do that in another CL though, so I can change all the vectors above too. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:474: flexfec_codec.SetParam(kFlexfecFmtpRepairWindow, "10000000"); On 2016/11/17 12:48:59, stefan-webrtc (holmer) wrote: > Comment on how this window size was chosen. Done. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:1136: class WebRtcVideoChannel2FlexfecTest : public WebRtcVideoChannel2Test { On 2016/11/17 12:48:59, stefan-webrtc (holmer) wrote: > Move this down to where it's used. Done.
I would like to land this CL before yours: https://codereview.webrtc.org/2493133002/. There will some conflicts, but not too bad. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/config.cc File webrtc/config.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/config.cc#newcode54 webrtc/config.cc:54: if (flexfec_payload_type < 0) Isn't the valid payload type range 96-127 for this? https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.cc:298: webrtc::FlexfecReceiveStream::Config config) I think this should be: FakeFlexfecReceiveStream(const webrtc::FlexfecReceiveStream::Config&) If you want a move ctor, do it explicitly: FakeFlexfecReceiveStream(webrtc::FlexfecReceiveStream::Config&&) But this is a fake class, so I don't think we care that much about performance. I would just go with a const-ref ctor. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2223: webrtc::FlexfecConfig flexfec_config) Make this const-ref instead of by-value. https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2563: flexfec.flexfec_payload_type == other.flexfec.flexfec_payload_type && Maybe you should implement operator== for FlexfecConfig? https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:2616: RTC_DCHECK(flexfec_payload_type == -1); Use RTC_DCHECK_EQ(-1, flexfec_payload_type) instead.
Rebase on top of magjed's CL + corresponding fixes.
Rebase on top of magjed's CL + corresponding fixes.
Patchset #4 (id:100001) has been deleted
Thanks for quick feedback!
I've responded and rebased in different patchsets. The failed bot is due to a
patch failure, happening due to the revert of the upstream CL :(
holmer: can you check my new changes to config.{h,cc}?
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/config.cc
File webrtc/config.cc (right):
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/config.cc#newcode54
webrtc/config.cc:54: if (flexfec_payload_type < 0)
On 2016/11/17 13:37:19, magjed_webrtc wrote:
> Isn't the valid payload type range 96-127 for this?
Yes, you are right. My intention with this function is however to check if we
have enough information to enable FlexFEC/if we should enable FlexFEC. The way
to signal that we do not want to enable FlexFEC is to send a payload type of -1,
hence this check. To make my intentions clearer, I will rename this function and
remove the 127 check.
(The validity of the payload type values is checked in VideoSendStream. There we
however allow anything in the range 0-127. I'll check with danilchap if we
should tighten that.)
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew...
File webrtc/media/engine/fakewebrtccall.cc (right):
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew...
webrtc/media/engine/fakewebrtccall.cc:298: webrtc::FlexfecReceiveStream::Config
config)
On 2016/11/17 13:37:19, magjed_webrtc wrote:
> I think this should be:
> FakeFlexfecReceiveStream(const webrtc::FlexfecReceiveStream::Config&)
> If you want a move ctor, do it explicitly:
> FakeFlexfecReceiveStream(webrtc::FlexfecReceiveStream::Config&&)
> But this is a fake class, so I don't think we care that much about
performance.
> I would just go with a const-ref ctor.
Yes, this was sloppy. Changed to const ref ctor!
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2223: webrtc::FlexfecConfig
flexfec_config)
On 2016/11/17 13:37:19, magjed_webrtc wrote:
> Make this const-ref instead of by-value.
Done.
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2563: flexfec.flexfec_payload_type ==
other.flexfec.flexfec_payload_type &&
On 2016/11/17 13:37:19, magjed_webrtc wrote:
> Maybe you should implement operator== for FlexfecConfig?
Done. Also implemented it for UlpfecConfig, to further clean up this function.
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:2616: RTC_DCHECK(flexfec_payload_type
== -1);
On 2016/11/17 13:37:19, magjed_webrtc wrote:
> Use RTC_DCHECK_EQ(-1, flexfec_payload_type) instead.
Done.
https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):
https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:657: // not respect the
ScopedFieldTrials that we use in the tests.
magjed: Is this OK?
Further, checking the field trial in the InternalEncoderFactory ctor may not be
a good idea if the application wants to control the field trial on a per-call
basis.
Thanks for rebasing on top of my CL! I have relanded my CL now and it seems to stick this time. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/config.cc File webrtc/config.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/config.cc#newcode67 webrtc/config.cc:67: if (protected_media_ssrcs.size() != 1) I think you will need to write 1u to avoid warnings about comparing integers of different sign. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:480: webrtc::FlexfecReceiveStream::Config config) { I would like this to be a const-ref as well (and/or webrtc::FlexfecReceiveStream::Config&& if you want to have move functionality). https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.h:300: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; I see that you have just followed the example of how this has been done previously, but it would be nice to have some better lifetime management. As I understand it, these vectors actually own their elements. I think std::list<FakeFlexfecReceiveStream> would be a more suitable container (std::list so that pointers are kept valid when you erase iterators), and by-value so that ownership is obvious. The return type for GetFlexfecReceiveStreams() would be std::list<FakeFlexfecReceiveStream>&. You don't need to update the old vectors because I believe that will be a bit of work and outside the scope of this CL, but it would be nice to set a better example of how it should be done. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:657: // not respect the ScopedFieldTrials that we use in the tests. On 2016/11/17 17:28:58, brandtr wrote: > magjed: Is this OK? > > Further, checking the field trial in the InternalEncoderFactory ctor may not be > a good idea if the application wants to control the field trial on a per-call > basis. This won't work because the payload won't be assigned (it will be 0). I have relanded my CL and removed the singleton pattern from InternalEncoderFactory so that you can put the field trial in the ctor. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.h:440: webrtc::FlexfecReceiveStream* flexfec_stream_; Can you comment who owns |flexfec_stream_|?
Rebase.
Patchset #6 (id:160001) has been deleted
https://codereview.webrtc.org/2511703002/diff/120001/webrtc/config.cc File webrtc/config.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/config.cc#newcode67 webrtc/config.cc:67: if (protected_media_ssrcs.size() != 1) On 2016/11/18 14:28:00, magjed_webrtc wrote: > I think you will need to write 1u to avoid warnings about comparing integers of > different sign. I didn't receive any compiler warnings here, but might as well add the suffix. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:480: webrtc::FlexfecReceiveStream::Config config) { On 2016/11/18 14:28:00, magjed_webrtc wrote: > I would like this to be a const-ref as well (and/or > webrtc::FlexfecReceiveStream::Config&& if you want to have move functionality). Can't do it here alone, because FakeCall is a webrtc::Call, which has this signature: https://cs.chromium.org/chromium/src/third_party/webrtc/call.h?l=122. (Note that the signature of CreateFlexfecReceiveStream is inspired by CreateVideoReceiveStream. Now I noticed that CreateAudioReceiveStream takes a const ref parameter however...) I will make an AI to update the signature of CreateFlexfecReceiveStream (both in call.h and here) to take a const ref instead. I don't think move functionality is needed. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.h:300: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; On 2016/11/18 14:28:00, magjed_webrtc wrote: > I see that you have just followed the example of how this has been done > previously, but it would be nice to have some better lifetime management. As I > understand it, these vectors actually own their elements. I think > std::list<FakeFlexfecReceiveStream> would be a more suitable container > (std::list so that pointers are kept valid when you erase iterators), and > by-value so that ownership is obvious. The return type for > GetFlexfecReceiveStreams() would be std::list<FakeFlexfecReceiveStream>&. > > You don't need to update the old vectors because I believe that will be a bit of > work and outside the scope of this CL, but it would be nice to set a better > example of how it should be done. I thought about this and realized why we have these raw (but owning) pointers here: this is how webrtc::Call works, and FakeCall is a webrtc::Call. (See https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=170.) In the production code, webrtc::internal::Call (the implementation of webrtc::Call) will create a stream object when its method Create{Audio,Video}{Receive,Send}Stream is called. It will then return a raw base pointer of the created object to the caller, who will own the created object. When it's time to destroy the created object, the original caller will call webrtc::Call::Destroy{Audio,Video}{Receive,Send}Stream with the owning raw base pointer, and webrtc::internal::Call will delete the object which is pointed to by a raw derived pointer. The raw derived pointer is stored internally in webrtc::internal::Call. So although I like your idea of trying to make the lifetime management clear (which it definitely is not right now), I think the conclusion is that even though I could change the type of |flexfec_receive_streams_|, that would make the implementation of FakeCall::CreateFlexfecReceiveStream and FakeCall::DestroyFlexfecReceiveStream very convoluted. Recall that I can't change the signature of those methods, as they are overriding corresponding methods in webrtc::Call. I don't know who designed the creation/deletion interface and logic in webrtc::Call in the first place, but maybe we should have a discussion about simplifying it. That would involve several PSAs and non-breaking changes however, as this is clearly a public API. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.cc:657: // not respect the ScopedFieldTrials that we use in the tests. On 2016/11/18 14:28:00, magjed_webrtc wrote: > On 2016/11/17 17:28:58, brandtr wrote: > > magjed: Is this OK? > > > > Further, checking the field trial in the InternalEncoderFactory ctor may not > be > > a good idea if the application wants to control the field trial on a per-call > > basis. > > This won't work because the payload won't be assigned (it will be 0). > I have relanded my CL and removed the singleton pattern from > InternalEncoderFactory so that you can put the field trial in the ctor. I see, moving to the ctor. (Thanks for changing your CL to accomodate this!) Btw, should I check in the tests that the generated payload type is valid? https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.h:440: webrtc::FlexfecReceiveStream* flexfec_stream_; On 2016/11/18 14:28:00, magjed_webrtc wrote: > Can you comment who owns |flexfec_stream_|? Done.
https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/20001/webrtc/media/engine/fakew... webrtc/media/engine/fakewebrtccall.h:299: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; On 2016/11/17 13:23:36, brandtr wrote: > On 2016/11/17 12:48:59, stefan-webrtc (holmer) wrote: > > std::vector<unique_ptr<...>>? > > That would be nice. I'll do that in another CL though, so I can change all the > vectors above too. See discussion here: https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake...
https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.h:300: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; On 2016/11/21 08:52:03, brandtr wrote: > On 2016/11/18 14:28:00, magjed_webrtc wrote: > > I see that you have just followed the example of how this has been done > > previously, but it would be nice to have some better lifetime management. As I > > understand it, these vectors actually own their elements. I think > > std::list<FakeFlexfecReceiveStream> would be a more suitable container > > (std::list so that pointers are kept valid when you erase iterators), and > > by-value so that ownership is obvious. The return type for > > GetFlexfecReceiveStreams() would be std::list<FakeFlexfecReceiveStream>&. > > > > You don't need to update the old vectors because I believe that will be a bit > of > > work and outside the scope of this CL, but it would be nice to set a better > > example of how it should be done. > > I thought about this and realized why we have these raw (but owning) pointers > here: this is how webrtc::Call works, and FakeCall is a webrtc::Call. (See > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=170.) > > In the production code, webrtc::internal::Call (the implementation of > webrtc::Call) will create a stream object when its method > Create{Audio,Video}{Receive,Send}Stream is called. It will then return a raw > base pointer of the created object to the caller, who will own the created > object. When it's time to destroy the created object, the original caller will > call webrtc::Call::Destroy{Audio,Video}{Receive,Send}Stream with the owning raw > base pointer, and webrtc::internal::Call will delete the object which is pointed > to by a raw derived pointer. The raw derived pointer is stored internally in > webrtc::internal::Call. > > So although I like your idea of trying to make the lifetime management clear > (which it definitely is not right now), I think the conclusion is that even > though I could change the type of |flexfec_receive_streams_|, that would make > the implementation of FakeCall::CreateFlexfecReceiveStream and > FakeCall::DestroyFlexfecReceiveStream very convoluted. Recall that I can't > change the signature of those methods, as they are overriding corresponding > methods in webrtc::Call. > > I don't know who designed the creation/deletion interface and logic in > webrtc::Call in the first place, but maybe we should have a discussion about > simplifying it. That would involve several PSAs and non-breaking changes > however, as this is clearly a public API. Yeah, it would probably be a good idea to discuss how to simplify the ownership model and add more type safety in webrtc::Call. Changing the public API is of course out of the scope for this CL, but I still think you can simplify ownership in this fake class without touching anything else. Using std::list<FakeFlexfecReceiveStream>, the implementation of FakeCall::CreateFlexfecReceiveStream and FakeCall::DestroyFlexfecReceiveStream would look like this: webrtc::FlexfecReceiveStream* FakeCall::CreateFlexfecReceiveStream( webrtc::FlexfecReceiveStream::Config config) { flexfec_receive_streams_.push_back( FakeFlexfecReceiveStream(std::move(config))); ++num_created_receive_streams_; return &flexfec_receive_streams_.back(); } void FakeCall::DestroyFlexfecReceiveStream( webrtc::FlexfecReceiveStream* receive_stream) { for (auto it = flexfec_receive_streams_.begin(); it != flexfec_receive_streams_.end(); ++it) { if (&*it == receive_stream) { flexfec_receive_streams_.erase(it); return; } } ADD_FAILURE() << "DestroyFlexfecReceiveStream called with unknown parameter."; } I don't think that's more convoluted than what we have now. Having a std::vector<std::unique_ptr<FakeFlexfecReceiveStream>> like Stefan suggests is very similar to a std::list<FakeFlexfecReceiveStream>, but I don't see why we need the extra indirection, but I'm fine with both alternatives, and they would both be a big improvement over the current raw pointers. https://codereview.webrtc.org/2511703002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2511703002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.h:436: // Both |stream_| and |flexfec_stream_| are owned by |this|. I'm still slightly confused, because |call_| actually owns these so that's why we can't put them into unique_ptrs. Can you clarify even more, something like: Both |stream_| and |flexfec_stream_| are managed by |this|, and are destroyed by calling call_->DestroyVideoReceiveStream and call_->DestroyFlexfecReceiveStream respectively.
Tests pass locally. Will rebase in later patchset to fix the bot patch failure. https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.h (right): https://codereview.webrtc.org/2511703002/diff/120001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.h:300: std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_; On 2016/11/21 13:55:13, magjed_webrtc wrote: > On 2016/11/21 08:52:03, brandtr wrote: > > On 2016/11/18 14:28:00, magjed_webrtc wrote: > > > I see that you have just followed the example of how this has been done > > > previously, but it would be nice to have some better lifetime management. As > I > > > understand it, these vectors actually own their elements. I think > > > std::list<FakeFlexfecReceiveStream> would be a more suitable container > > > (std::list so that pointers are kept valid when you erase iterators), and > > > by-value so that ownership is obvious. The return type for > > > GetFlexfecReceiveStreams() would be std::list<FakeFlexfecReceiveStream>&. > > > > > > You don't need to update the old vectors because I believe that will be a > bit > > of > > > work and outside the scope of this CL, but it would be nice to set a better > > > example of how it should be done. > > > > I thought about this and realized why we have these raw (but owning) pointers > > here: this is how webrtc::Call works, and FakeCall is a webrtc::Call. (See > > https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?l=170.) > > > > In the production code, webrtc::internal::Call (the implementation of > > webrtc::Call) will create a stream object when its method > > Create{Audio,Video}{Receive,Send}Stream is called. It will then return a raw > > base pointer of the created object to the caller, who will own the created > > object. When it's time to destroy the created object, the original caller will > > call webrtc::Call::Destroy{Audio,Video}{Receive,Send}Stream with the owning > raw > > base pointer, and webrtc::internal::Call will delete the object which is > pointed > > to by a raw derived pointer. The raw derived pointer is stored internally in > > webrtc::internal::Call. > > > > So although I like your idea of trying to make the lifetime management clear > > (which it definitely is not right now), I think the conclusion is that even > > though I could change the type of |flexfec_receive_streams_|, that would make > > the implementation of FakeCall::CreateFlexfecReceiveStream and > > FakeCall::DestroyFlexfecReceiveStream very convoluted. Recall that I can't > > change the signature of those methods, as they are overriding corresponding > > methods in webrtc::Call. > > > > I don't know who designed the creation/deletion interface and logic in > > webrtc::Call in the first place, but maybe we should have a discussion about > > simplifying it. That would involve several PSAs and non-breaking changes > > however, as this is clearly a public API. > > Yeah, it would probably be a good idea to discuss how to simplify the ownership > model and add more type safety in webrtc::Call. Changing the public API is of > course out of the scope for this CL, but I still think you can simplify > ownership in this fake class without touching anything else. Using > std::list<FakeFlexfecReceiveStream>, the implementation of > FakeCall::CreateFlexfecReceiveStream and FakeCall::DestroyFlexfecReceiveStream > would look like this: > > webrtc::FlexfecReceiveStream* FakeCall::CreateFlexfecReceiveStream( > webrtc::FlexfecReceiveStream::Config config) { > flexfec_receive_streams_.push_back( > FakeFlexfecReceiveStream(std::move(config))); > ++num_created_receive_streams_; > return &flexfec_receive_streams_.back(); > } > > void FakeCall::DestroyFlexfecReceiveStream( > webrtc::FlexfecReceiveStream* receive_stream) { > for (auto it = flexfec_receive_streams_.begin(); > it != flexfec_receive_streams_.end(); ++it) { > if (&*it == receive_stream) { > flexfec_receive_streams_.erase(it); > return; > } > } > ADD_FAILURE() << "DestroyFlexfecReceiveStream called with unknown parameter."; > } > > I don't think that's more convoluted than what we have now. Having a > std::vector<std::unique_ptr<FakeFlexfecReceiveStream>> like Stefan suggests is > very similar to a std::list<FakeFlexfecReceiveStream>, but I don't see why we > need the extra indirection, but I'm fine with both alternatives, and they would > both be a big improvement over the current raw pointers. Ok, I agree that is not more convoluted than the current implementation. (Claming that it was "very convoluted" was clearly an exaggeration, sorry for that...) I have changed to your suggestion :) Further agree that we do not need the extra indirection of the unique_ptr's, although in the code that I usually touch it is quite common to see that pattern. https://codereview.webrtc.org/2511703002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2.h (right): https://codereview.webrtc.org/2511703002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2.h:436: // Both |stream_| and |flexfec_stream_| are owned by |this|. On 2016/11/21 13:55:13, magjed_webrtc wrote: > I'm still slightly confused, because |call_| actually owns these so that's why > we can't put them into unique_ptrs. Can you clarify even more, something like: > Both |stream_| and |flexfec_stream_| are managed by |this|, and are destroyed by > calling call_->DestroyVideoReceiveStream and call_->DestroyFlexfecReceiveStream > respectively. My original thinking was that the caller owns the streams, not Call, since it is up to the caller to decide when the streams should be created and destroyed. During the method calls, the ownership is then temporarily moved between the caller and Call through the raw pointers. But this thinking breaks down with your suggested implementation of the Create/Destroy methods. So I think your suggested comment takes makes sense here. https://codereview.webrtc.org/2511703002/diff/200001/webrtc/media/engine/fake... File webrtc/media/engine/fakewebrtccall.cc (right): https://codereview.webrtc.org/2511703002/diff/200001/webrtc/media/engine/fake... webrtc/media/engine/fakewebrtccall.cc:486: These changes are clearly not "very convoluted", so sorry for claiming that :) https://codereview.webrtc.org/2511703002/diff/200001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2511703002/diff/200001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvideoengine2_unittest.cc:2877: ASSERT_TRUE(channel_->SetSendParameters(send_parameters)); A crucial line was missing here.
lgtm
Rebase.
The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2511703002/#ps220001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1479809702508730,
"parent_rev": "b166000799adf4948a356b47c0bf415c6fec36bd", "commit_rev":
"3f3c782ba328ceb8cdb241b6b4eb36444ea9f0b1"}
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Wire up FlexFEC in VideoEngine2. This CL interfaces the SDP information (payload types and SSRCs) about FlexFEC with the corresponding configs at the Call layer. It also adds a field trial, which when active will expose FlexFEC in the default codec list, thus showing up in the default SDP. BUG=webrtc:5654 R=magjed@webrtc.org, stefan@webrtc.org CC=perkj@webrtc.org ========== to ========== Wire up FlexFEC in VideoEngine2. This CL interfaces the SDP information (payload types and SSRCs) about FlexFEC with the corresponding configs at the Call layer. It also adds a field trial, which when active will expose FlexFEC in the default codec list, thus showing up in the default SDP. BUG=webrtc:5654 R=magjed@webrtc.org, stefan@webrtc.org CC=perkj@webrtc.org Committed: https://crrev.com/468da7c0747e078f689eb02cd86ec31cdb5be903 Cr-Commit-Position: refs/heads/master@{#15184} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/468da7c0747e078f689eb02cd86ec31cdb5be903 Cr-Commit-Position: refs/heads/master@{#15184} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
