Bug 1428098 - Do not reconfigure manually when input resolution changes. r?dminor draft
authorAndreas Pehrson <pehrsons@mozilla.com>
Wed, 03 Jan 2018 10:28:17 +0100
changeset 718538 1a74c512cf5f6de91ab2f5fc9dd587a763050e41
parent 718536 ad2324282192c35e4ec0df337e74d4ad22642806
child 745507 9f4ccd97a87341b9dfc937228592f1edc6617d52
push id94952
push userbmo:apehrson@mozilla.com
push dateWed, 10 Jan 2018 12:39:53 +0000
reviewersdminor
bugs1428098
milestone59.0a1
Bug 1428098 - Do not reconfigure manually when input resolution changes. r?dminor webrtc.org now handles this. MozReview-Commit-ID: 8loJR1L0h1m
media/webrtc/signaling/gtest/videoconduit_unittests.cpp
media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
media/webrtc/signaling/src/media-conduit/VideoConduit.h
--- a/media/webrtc/signaling/gtest/videoconduit_unittests.cpp
+++ b/media/webrtc/signaling/gtest/videoconduit_unittests.cpp
@@ -69,18 +69,20 @@ public:
 class MockVideoSink : public rtc::VideoSinkInterface<webrtc::VideoFrame>
 {
 public:
   ~MockVideoSink() override = default;
 
   void OnFrame(const webrtc::VideoFrame& frame) override
   {
     mVideoFrame = frame;
+    ++mOnFrameCount;
   }
 
+  size_t mOnFrameCount = 0;
   webrtc::VideoFrame mVideoFrame;
 };
 
 class VideoConduitTest : public ::testing::Test {
 public:
 
   VideoConduitTest()
     : mCall(new MockCall())
@@ -980,26 +982,29 @@ TEST_F(VideoConduitTest, TestReconfigure
   rtc::VideoSinkWants wants;
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   mVideoConduit->StartTransmitting();
   SendVideoFrame(1280, 720, 1);
   ASSERT_EQ(sink->mVideoFrame.width(), 1280);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 1000U);
+  ASSERT_EQ(sink->mOnFrameCount, 1U);
 
   SendVideoFrame(640, 360, 2);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 2000U);
+  ASSERT_EQ(sink->mOnFrameCount, 2U);
 
   SendVideoFrame(1920, 1280, 3);
   ASSERT_EQ(sink->mVideoFrame.width(), 960);
   ASSERT_EQ(sink->mVideoFrame.height(), 640);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 3000U);
+  ASSERT_EQ(sink->mOnFrameCount, 3U);
   mVideoConduit->StopTransmitting();
 }
 
 TEST_F(VideoConduitTest, TestReconfigureSendMediaCodecWhileTransmitting)
 {
   MediaConduitErrorCode ec;
   EncodingConstraints constraints;
   VideoCodecConfig::SimulcastEncoding encoding;
@@ -1071,26 +1076,29 @@ TEST_F(VideoConduitTest, TestReconfigure
   UniquePtr<MockVideoSink> sink(new MockVideoSink());
   rtc::VideoSinkWants wants;
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   SendVideoFrame(1280, 720, 1);
   ASSERT_EQ(sink->mVideoFrame.width(), 1280);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 1000U);
+  ASSERT_EQ(sink->mOnFrameCount, 1U);
 
   SendVideoFrame(640, 360, 2);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 2000U);
+  ASSERT_EQ(sink->mOnFrameCount, 2U);
 
   SendVideoFrame(1920, 1280, 3);
   ASSERT_EQ(sink->mVideoFrame.width(), 960);
   ASSERT_EQ(sink->mVideoFrame.height(), 640);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 3000U);
+  ASSERT_EQ(sink->mOnFrameCount, 3U);
 
   mVideoConduit->StopTransmitting();
 }
 
 TEST_F(VideoConduitTest, TestVideoEncode)
 {
   MediaConduitErrorCode ec;
   EncodingConstraints constraints;
@@ -1106,26 +1114,29 @@ TEST_F(VideoConduitTest, TestVideoEncode
   rtc::VideoSinkWants wants;
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   mVideoConduit->StartTransmitting();
   SendVideoFrame(1280, 720, 1);
   ASSERT_EQ(sink->mVideoFrame.width(), 1280);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 1000U);
+  ASSERT_EQ(sink->mOnFrameCount, 1U);
 
   SendVideoFrame(640, 360, 2);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 2000U);
+  ASSERT_EQ(sink->mOnFrameCount, 2U);
 
   SendVideoFrame(1920, 1280, 3);
   ASSERT_EQ(sink->mVideoFrame.width(), 1920);
   ASSERT_EQ(sink->mVideoFrame.height(), 1280);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 3000U);
+  ASSERT_EQ(sink->mOnFrameCount, 3U);
 
   mVideoConduit->StopTransmitting();
   mVideoConduit->RemoveSink(sink.get());
 }
 
 TEST_F(VideoConduitTest, TestVideoEncodeMaxFs)
 {
   MediaConduitErrorCode ec;
@@ -1143,46 +1154,52 @@ TEST_F(VideoConduitTest, TestVideoEncode
   rtc::VideoSinkWants wants;
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   mVideoConduit->StartTransmitting();
   SendVideoFrame(1280, 720, 1);
   ASSERT_EQ(sink->mVideoFrame.width(), 1280);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 1000U);
+  ASSERT_EQ(sink->mOnFrameCount, 1U);
 
   SendVideoFrame(640, 360, 2);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 2000U);
+  ASSERT_EQ(sink->mOnFrameCount, 2U);
 
   SendVideoFrame(1920, 1280, 3);
   ASSERT_EQ(sink->mVideoFrame.width(), 960);
   ASSERT_EQ(sink->mVideoFrame.height(), 640);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 3000U);
+  ASSERT_EQ(sink->mOnFrameCount, 3U);
 
   // maxFs should not force pixel count above what a sink has requested.
   // We set 3600 macroblocks (16x16 pixels), so we request 3500 here.
   wants.max_pixel_count = rtc::Optional<int>(3500*16*16);
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   SendVideoFrame(1280, 720, 4);
   ASSERT_EQ(sink->mVideoFrame.width(), 960);
   ASSERT_EQ(sink->mVideoFrame.height(), 540);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 4000U);
+  ASSERT_EQ(sink->mOnFrameCount, 4U);
 
   SendVideoFrame(640, 360, 5);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 5000U);
+  ASSERT_EQ(sink->mOnFrameCount, 5U);
 
   SendVideoFrame(1920, 1280, 6);
   ASSERT_EQ(sink->mVideoFrame.width(), 960);
   ASSERT_EQ(sink->mVideoFrame.height(), 640);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 6000U);
+  ASSERT_EQ(sink->mOnFrameCount, 6U);
 
   mVideoConduit->StopTransmitting();
   mVideoConduit->RemoveSink(sink.get());
 }
 
 // Disabled: See Bug 1420493
 TEST_F(VideoConduitTest, DISABLED_TestVideoEncodeMaxWidthAndHeight)
 {
@@ -1202,24 +1219,27 @@ TEST_F(VideoConduitTest, DISABLED_TestVi
   rtc::VideoSinkWants wants;
   mVideoConduit->AddOrUpdateSink(sink.get(), wants);
 
   mVideoConduit->StartTransmitting();
   SendVideoFrame(1280, 720, 1);
   ASSERT_EQ(sink->mVideoFrame.width(), 1280);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 1000U);
+  ASSERT_EQ(sink->mOnFrameCount, 1U);
 
   SendVideoFrame(640, 360, 2);
   ASSERT_EQ(sink->mVideoFrame.width(), 640);
   ASSERT_EQ(sink->mVideoFrame.height(), 360);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 2000U);
+  ASSERT_EQ(sink->mOnFrameCount, 2U);
 
   SendVideoFrame(1920, 1280, 3);
   ASSERT_EQ(sink->mVideoFrame.width(), 1080);
   ASSERT_EQ(sink->mVideoFrame.height(), 720);
   ASSERT_EQ(sink->mVideoFrame.timestamp_us(), 3000U);
+  ASSERT_EQ(sink->mOnFrameCount, 3U);
 
   mVideoConduit->StopTransmitting();
   mVideoConduit->RemoveSink(sink.get());
 }
 
 } // End namespace test.
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -241,17 +241,16 @@ WebrtcVideoConduit::WebrtcVideoConduit(R
   : mTransportMonitor("WebrtcVideoConduit")
   , mRenderer(nullptr)
   , mVideoAdapter(std::move(aVideoAdapter))
   , mVideoBroadcaster()
   , mEngineTransmitting(false)
   , mEngineReceiving(false)
   , mCapId(-1)
   , mCodecMutex("VideoConduit codec db")
-  , mInReconfig(false)
   , mRecvStream(nullptr)
   , mSendStream(nullptr)
   , mLastWidth(0)
   , mLastHeight(0) // initializing as 0 forces a check for reconfig at start
   , mSendingWidth(0)
   , mSendingHeight(0)
   , mReceivingWidth(0)
   , mReceivingHeight(0)
@@ -1692,22 +1691,19 @@ WebrtcVideoConduit::SelectBitrates(
   }
   out_start = std::min(out_max, std::max(out_start, out_min));
 
   MOZ_ASSERT(mPrefMaxBitrate == 0 || out_max <= mPrefMaxBitrate);
 }
 
 // XXX we need to figure out how to feed back changes in preferred capture
 // resolution to the getUserMedia source.
-// Returns boolean if we've submitted an async change (and took ownership
-// of *frame's data)
-bool
+void
 WebrtcVideoConduit::SelectSendResolution(unsigned short width,
-                                         unsigned short height,
-                                         const webrtc::VideoFrame* frame) // may be null
+                                         unsigned short height)
 {
   mCodecMutex.AssertCurrentThreadOwns();
   // XXX This will do bandwidth-resolution adaptation as well - bug 877954
 
   mLastWidth = width;
   mLastHeight = height;
   // Enforce constraints
   if (mCurSendCodecConfig) {
@@ -1726,105 +1722,37 @@ WebrtcVideoConduit::SelectSendResolution
       if (max_fs > mLastSinkWanted.max_pixel_count.value_or(max_fs)) {
         max_fs = mLastSinkWanted.max_pixel_count.value_or(max_fs);
       }
       mVideoAdapter->OnResolutionRequest(rtc::Optional<int>(max_fs),
                                          rtc::Optional<int>());
     }
   }
 
-  // Adapt to getUserMedia resolution changes
-  // check if we need to reconfigure the sending resolution.
+  // Update on resolution changes
   // NOTE: mSendingWidth != mLastWidth, because of maxwidth/height/etc above
-  bool changed = false;
   if (mSendingWidth != width || mSendingHeight != height) {
     CSFLogDebug(LOGTAG, "%s: resolution changing to %ux%u (from %ux%u)",
                 __FUNCTION__, width, height, mSendingWidth, mSendingHeight);
     // This will avoid us continually retrying this operation if it fails.
     // If the resolution changes, we'll try again.  In the meantime, we'll
     // keep using the old size in the encoder.
     mSendingWidth = width;
     mSendingHeight = height;
-    changed = true;
   }
 
   unsigned int framerate = SelectSendFrameRate(mCurSendCodecConfig,
                                                mSendingFramerate,
                                                mSendingWidth,
                                                mSendingHeight);
   if (mSendingFramerate != framerate) {
     CSFLogDebug(LOGTAG, "%s: framerate changing to %u (from %u)",
                 __FUNCTION__, framerate, mSendingFramerate);
     mSendingFramerate = framerate;
-    changed = true;
   }
-
-  if (changed) {
-    // On a resolution change, bounce this to the correct thread to
-    // re-configure (same as used for Init().  Do *not* block the calling
-    // thread since that may be the MSG thread.
-
-    // MUST run on the same thread as Init()/etc
-    if (!NS_IsMainThread()) {
-      // Note: on *initial* config (first frame), best would be to drop
-      // frames until the config is done, then encode the most recent frame
-      // provided and continue from there.  We don't do this, but we do drop
-      // all frames while in the process of a reconfig and then encode the
-      // frame that started the reconfig, which is close.  There may be
-      // barely perceptible glitch in the video due to the dropped frame(s).
-      mInReconfig = true;
-
-      // We can't pass a UniquePtr<> or unique_ptr<> to a lambda directly
-      webrtc::VideoFrame* new_frame = nullptr;
-      if (frame) {
-        // the internal buffer pointer is refcounted, so we don't have 2 copies here
-        new_frame = new webrtc::VideoFrame(*frame);
-      }
-      RefPtr<WebrtcVideoConduit> self(this);
-      RefPtr<Runnable> webrtc_runnable =
-        media::NewRunnableFrom([self, width, height, new_frame]() -> nsresult {
-            UniquePtr<webrtc::VideoFrame> local_frame(new_frame); // Simplify cleanup
-
-            MutexAutoLock lock(self->mCodecMutex);
-            return self->ReconfigureSendCodec(width, height, new_frame);
-          });
-      // new_frame now owned by lambda
-      CSFLogDebug(LOGTAG, "%s: proxying lambda to WebRTC thread for reconfig (width %u/%u, height %u/%u",
-                  __FUNCTION__, width, mLastWidth, height, mLastHeight);
-      NS_DispatchToMainThread(webrtc_runnable.forget());
-      if (new_frame) {
-        return true; // queued it
-      }
-    } else {
-      // already on the right thread
-      ReconfigureSendCodec(width, height, frame);
-    }
-  }
-  return false;
-}
-
-nsresult
-WebrtcVideoConduit::ReconfigureSendCodec(unsigned short width,
-                                         unsigned short height,
-                                         const webrtc::VideoFrame* frame)
-{
-  mCodecMutex.AssertCurrentThreadOwns();
-
-  // Test in case the stream hasn't started yet!  We could get a frame in
-  // before we get around to StartTransmitting(), and that would dispatch a
-  // runnable to call this.
-  mInReconfig = false;
-  if (mSendStream) {
-    mSendStream->ReconfigureVideoEncoder(mEncoderConfig.CopyConfig());
-    if (frame) {
-      mVideoBroadcaster.OnFrame(*frame);
-      CSFLogDebug(LOGTAG, "%s Inserted a frame from reconfig lambda", __FUNCTION__);
-    }
-  }
-  return NS_OK;
 }
 
 unsigned int
 WebrtcVideoConduit::SelectSendFrameRate(const VideoCodecConfig* codecConfig,
                                         unsigned int old_framerate,
                                         unsigned short sending_width,
                                         unsigned short sending_height) const
 {
@@ -1958,38 +1886,32 @@ MediaConduitErrorCode
 WebrtcVideoConduit::SendVideoFrame(const webrtc::VideoFrame& frame)
 {
   // XXX Google uses a "timestamp_aligner" to translate timestamps from the
   // camera via TranslateTimestamp(); we should look at doing the same.  This
   // avoids sampling error when capturing frames, but google had to deal with some
   // broken cameras, include Logitech c920's IIRC.
 
   CSFLogVerbose(LOGTAG, "%s (send SSRC %u (0x%x))", __FUNCTION__,
-              mSendStreamConfig.rtp.ssrcs.front(), mSendStreamConfig.rtp.ssrcs.front());
+                mSendStreamConfig.rtp.ssrcs.front(), mSendStreamConfig.rtp.ssrcs.front());
   // See if we need to recalculate what we're sending.
   // Don't compute mSendingWidth/Height, since those may not be the same as the input.
   {
-    MutexAutoLock lock(mCodecMutex);
-    if (mInReconfig) {
-      // Waiting for it to finish
-      return kMediaConduitNoError;
-    }
     // mLastWidth/Height starts at 0, so we'll never call SelectSendResolution with a 0 size.
     // We in some cases set them back to 0 to force SelectSendResolution to be called again.
     if (frame.width() != mLastWidth || frame.height() != mLastHeight) {
       CSFLogVerbose(LOGTAG, "%s: call SelectSendResolution with %ux%u",
                     __FUNCTION__, frame.width(), frame.height());
       MOZ_ASSERT(frame.width() != 0 && frame.height() != 0);
       // Note coverity will flag this since it thinks they can be 0
-      if (SelectSendResolution(frame.width(), frame.height(), &frame)) {
-        // SelectSendResolution took ownership of the data in i420_frame.
-        // Submit the frame after reconfig is done
-        return kMediaConduitNoError;
-      }
+
+      MutexAutoLock lock(mCodecMutex);
+      SelectSendResolution(frame.width(), frame.height());
     }
+
     // adapt input video to wants of sink
     if (!mVideoBroadcaster.frame_wanted()) {
       return kMediaConduitNoError;
     }
 
     int adapted_width;
     int adapted_height;
     int crop_width;
--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ -166,29 +166,18 @@ public:
                       webrtc::VideoStream& aVideoStream);
 
   /**
    * Function to select and change the encoding resolution based on incoming frame size
    * and current available bandwidth.
    * @param width, height: dimensions of the frame
    * @param frame: optional frame to submit for encoding after reconfig
    */
-  bool SelectSendResolution(unsigned short width,
-                            unsigned short height,
-                            const webrtc::VideoFrame* frame);
-
-  /**
-   * Function to reconfigure the current send codec for a different
-   * width/height/framerate/etc.
-   * @param width, height: dimensions of the frame
-   * @param frame: optional frame to submit for encoding after reconfig
-   */
-  nsresult ReconfigureSendCodec(unsigned short width,
-                                unsigned short height,
-                                const webrtc::VideoFrame* frame);
+  void SelectSendResolution(unsigned short width,
+                            unsigned short height);
 
   /**
    * Function to select and change the encoding frame rate based on incoming frame rate
    * and max-mbps setting.
    * @param current framerate
    * @result new framerate
    */
   unsigned int SelectSendFrameRate(const VideoCodecConfig* codecConfig,
@@ -501,20 +490,19 @@ private:
   // Engine state we are concerned with.
   mozilla::Atomic<bool> mEngineTransmitting; // If true ==> Transmit Subsystem is up and running
   mozilla::Atomic<bool> mEngineReceiving;    // if true ==> Receive Subsystem up and running
 
   int mCapId;   // Capturer for this conduit
   //Local database of currently applied receive codecs
   nsTArray<UniquePtr<VideoCodecConfig>> mRecvCodecList;
 
-  // protects mCurSendCodecConfig, mInReconfig,mVideoSend/RecvStreamStats, mSend/RecvStreams, mSendPacketCounts, mRecvPacketCounts
+  // protects mCurSendCodecConfig, mVideoSend/RecvStreamStats, mSend/RecvStreams, mSendPacketCounts, mRecvPacketCounts
   Mutex mCodecMutex;
   nsAutoPtr<VideoCodecConfig> mCurSendCodecConfig;
-  bool mInReconfig;
   SendStreamStatistics mSendStreamStats;
   ReceiveStreamStatistics mRecvStreamStats;
   webrtc::RtcpPacketTypeCounter mSendPacketCounts;
   webrtc::RtcpPacketTypeCounter mRecvPacketCounts;
 
   // Must call webrtc::Call::DestroyVideoReceive/SendStream to delete these:
   webrtc::VideoReceiveStream* mRecvStream;
   webrtc::VideoSendStream* mSendStream;