Bug 1299515 - Clean up MediaEngineRemoteVideoSource::FrameSizeChange. r?jib draft
authorAndreas Pehrson <pehrsons@mozilla.com>
Tue, 28 Nov 2017 11:29:46 +0100
changeset 705124 6fcc0bae37cd11e074e0f3258f2327f314d781a3
parent 705123 acee1bc87dfce46d2c82b049c2c5d2df1496be3b
child 705125 91aab8f5f0761be5bcb3c4a172e4443f5e49a721
push id91361
push userbmo:apehrson@mozilla.com
push dateWed, 29 Nov 2017 13:44:11 +0000
reviewersjib
bugs1299515
milestone59.0a1
Bug 1299515 - Clean up MediaEngineRemoteVideoSource::FrameSizeChange. r?jib When looking for whether MediaEngineRemoteVideoSource would ever set the size to 0x0 I found FrameSizeChange() hard to read. This attempts to clean it up, and adds some invariant checks to see that we're not doing unnecessary work. MozReview-Commit-ID: KPCTNQelh2a
dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
--- a/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
+++ b/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
@@ -1,15 +1,17 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "MediaEngineRemoteVideoSource.h"
 
+#include <limits>
+
 #include "mozilla/RefPtr.h"
 #include "VideoUtils.h"
 #include "nsIPrefService.h"
 #include "MediaTrackConstraints.h"
 #include "CamerasChild.h"
 
 extern mozilla::LogModule* GetMediaManagerLog();
 #define LOG(msg) MOZ_LOG(GetMediaManagerLog(), mozilla::LogLevel::Debug, msg)
@@ -408,45 +410,58 @@ MediaEngineRemoteVideoSource::NotifyPull
   StreamTime delta = aDesiredTime - aSource->GetEndOfAppendedData(aID);
 
   if (delta > 0) {
     AppendToTrack(aSource, mImage, aID, delta, aPrincipalHandle);
   }
 }
 
 void
-MediaEngineRemoteVideoSource::FrameSizeChange(unsigned int w, unsigned int h)
+MediaEngineRemoteVideoSource::FrameSizeChange(unsigned int aWidth, unsigned int aHeight)
 {
-  if ((mWidth < 0) || (mHeight < 0) ||
-      (w !=  (unsigned int) mWidth) || (h != (unsigned int) mHeight)) {
-    LOG(("MediaEngineRemoteVideoSource Video FrameSizeChange: %ux%u was %ux%u", w, h, mWidth, mHeight));
-    mWidth = w;
-    mHeight = h;
+  if (aWidth == static_cast<unsigned int>(mWidth) &&
+      aHeight == static_cast<unsigned int>(mHeight)) {
+    MOZ_ASSERT_UNREACHABLE("Unexpected frame size change that didn't change");
+    return;
+  }
+
+  if (aWidth == 0 || aHeight == 0) {
+    MOZ_ASSERT_UNREACHABLE("We'd rather not get a frame if it's 0 pixels large");
+    return;
+  }
 
-    auto settings = mSettings;
-    NS_DispatchToMainThread(media::NewRunnableFrom([settings, w, h]() mutable {
-      settings->mWidth.Value() = w;
-      settings->mHeight.Value() = h;
-      return NS_OK;
-    }));
-  }
+  MOZ_RELEASE_ASSERT(aWidth <= std::numeric_limits<decltype(mWidth)>::max() &&
+                     aHeight <= std::numeric_limits<decltype(mHeight)>::max(),
+                     "Overflow");
+
+  LOG(("MediaEngineRemoteVideoSource Video FrameSizeChange: %dx%d to %ux%u", mWidth, mHeight, aWidth, aHeight));
+  mWidth = aWidth;
+  mHeight = aHeight;
+
+  NS_DispatchToMainThread(media::NewRunnableFrom([settings = mSettings,
+                                                  aWidth, aHeight]() mutable {
+    settings->mWidth.Value() = aWidth;
+    settings->mHeight.Value() = aHeight;
+    return NS_OK;
+  }));
 }
 
 int
 MediaEngineRemoteVideoSource::DeliverFrame(uint8_t* aBuffer ,
-                                    const camera::VideoFrameProperties& aProps)
+                                           const camera::VideoFrameProperties& aProps)
 {
   // Check for proper state.
   if (mState != kStarted) {
     LOG(("DeliverFrame: video not started"));
     return 0;
   }
 
-  // Update the dimensions
-  FrameSizeChange(aProps.width(), aProps.height());
+  if (aProps.width() != mWidth || aProps.height() != mHeight) {
+    FrameSizeChange(aProps.width(), aProps.height());
+  }
 
   layers::PlanarYCbCrData data;
   RefPtr<layers::PlanarYCbCrImage> image;
   {
     // We grab the lock twice, but don't hold it across the (long) CopyData
     MonitorAutoLock lock(mMonitor);
     if (!mImageContainer) {
       LOG(("DeliverFrame() called after Stop()!"));