Wire up FlexFEC in VideoEngine2.

webrtc/media/engine/fakewebrtccall.h

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 178 matching lines @@
   void Start() override;
   void Stop() override;
 
   webrtc::VideoReceiveStream::Stats GetStats() const override;
 
   webrtc::VideoReceiveStream::Config config_;
   bool receiving_;
   webrtc::VideoReceiveStream::Stats stats_;
 };
 
+class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream {
+ public:
+  explicit FakeFlexfecReceiveStream(
+      const webrtc::FlexfecReceiveStream::Config& config);
+
+  const webrtc::FlexfecReceiveStream::Config& GetConfig();
+
+ private:
+  // webrtc::FlexfecReceiveStream implementation.
+  void Start() override;
+  void Stop() override;
+
+  webrtc::FlexfecReceiveStream::Stats GetStats() const override;
+
+  webrtc::FlexfecReceiveStream::Config config_;
+  bool receiving_;
+};
+
 class FakeCall final : public webrtc::Call, public webrtc::PacketReceiver {
  public:
   explicit FakeCall(const webrtc::Call::Config& config);
   ~FakeCall() override;
 
   webrtc::Call::Config GetConfig() const;
   const std::vector<FakeVideoSendStream*>& GetVideoSendStreams();
   const std::vector<FakeVideoReceiveStream*>& GetVideoReceiveStreams();
 
   const std::vector<FakeAudioSendStream*>& GetAudioSendStreams();
   const FakeAudioSendStream* GetAudioSendStream(uint32_t ssrc);
   const std::vector<FakeAudioReceiveStream*>& GetAudioReceiveStreams();
   const FakeAudioReceiveStream* GetAudioReceiveStream(uint32_t ssrc);
 
+  const std::vector<FakeFlexfecReceiveStream*>& GetFlexfecReceiveStreams();
+
   rtc::SentPacket last_sent_packet() const { return last_sent_packet_; }
 
   // This is useful if we care about the last media packet (with id populated)
   // but not the last ICE packet (with -1 ID).
   int last_sent_nonnegative_packet_id() const {
     return last_sent_nonnegative_packet_id_;
   }
 
   webrtc::NetworkState GetNetworkState(webrtc::MediaType media) const;
   int GetNumCreatedSendStreams() const;
@@ skipping 47 matching lines @@
   webrtc::Call::Config config_;
   webrtc::NetworkState audio_network_state_;
   webrtc::NetworkState video_network_state_;
   rtc::SentPacket last_sent_packet_;
   int last_sent_nonnegative_packet_id_ = -1;
   webrtc::Call::Stats stats_;
   std::vector<FakeVideoSendStream*> video_send_streams_;
   std::vector<FakeAudioSendStream*> audio_send_streams_;
   std::vector<FakeVideoReceiveStream*> video_receive_streams_;
   std::vector<FakeAudioReceiveStream*> audio_receive_streams_;
+  std::vector<FakeFlexfecReceiveStream*> flexfec_receive_streams_;

[magjed_webrtc, 2016/11/18 14:28:00]

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.

[brandtr, 2016/11/21 08:52:03]

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.

[magjed_webrtc, 2016/11/21 13:55:13]

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.

[brandtr, 2016/11/21 16:03:47]

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.

 
   int num_created_send_streams_;
   int num_created_receive_streams_;
 
   int audio_transport_overhead_;
   int video_transport_overhead_;
 };
 
 }  // namespace cricket
 #endif  // WEBRTC_MEDIA_ENGINE_FAKEWEBRTCCALL_H_