MediaRecorder: remove ASSERTS in bitrate clamping.

third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp

Index: third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
diff --git a/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp b/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
index 32a1901096c08ededa83c0d82267ed95d2305fe5..191a64f5dd8943def04d69dd289614ad7d440be7 100644
--- a/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
+++ b/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
@@ -52,34 +52,33 @@ String stateToString(MediaRecorder::State state)
 // This method throws NotSupportedError.
 void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionContext* context, const MediaRecorderOptions& options, MediaStream* stream, int* audioBitsPerSecond, int* videoBitsPerSecond)
 {
+    const bool useVideo = !stream->getVideoTracks().isEmpty();
+    const bool useAudio = !stream->getAudioTracks().isEmpty();
+
     // Clamp incoming values into a signed integer's range.
     // TODO(mcasas): This section would no be needed if the bit rates are signed or double, see https://github.com/w3c/mediacapture-record/issues/48.
     const unsigned kMaxIntAsUnsigned = std::numeric_limits<int>::max();
 
-    ASSERT(options.bitsPerSecond() >= 0);
-    ASSERT(options.bitsPerSecond() < kMaxIntAsUnsigned);
-    const int overallBps = std::min(options.bitsPerSecond(), kMaxIntAsUnsigned);
-
-    // If options.hasBitsPerSecond() is not present, assert than the individual |{video,audio}BitsPerSecond| are smaller than the maximum.
-    ASSERT(!options.hasBitsPerSecond() || options.videoBitsPerSecond() < kMaxIntAsUnsigned);
-    int videoBps = std::min(options.videoBitsPerSecond(), kMaxIntAsUnsigned);
-
-    ASSERT(!options.hasBitsPerSecond() || options.audioBitsPerSecond() < kMaxIntAsUnsigned);
-    int audioBps = std::min(options.audioBitsPerSecond(), kMaxIntAsUnsigned);
-
-    const bool useVideo = !stream->getVideoTracks().isEmpty();
-    const bool useAudio = !stream->getAudioTracks().isEmpty();
+    int overallBps = 0;
+    if (options.hasBitsPerSecond())
+        overallBps = std::min(options.bitsPerSecond(), kMaxIntAsUnsigned);
+    int videoBps = 0;
+    if (options.hasVideoBitsPerSecond() && useVideo)
+        videoBps = std::min(options.videoBitsPerSecond(), kMaxIntAsUnsigned);
+    int audioBps = 0;
+    if (options.hasAudioBitsPerSecond() && useAudio)
+        audioBps = std::min(options.audioBitsPerSecond(), kMaxIntAsUnsigned);
 
     if (useAudio) {
         // |overallBps| overrides the specific audio and video bit rates.
-        if (overallBps) {
+        if (options.hasBitsPerSecond()) {

[philipj_slow, 2016/02/15 04:12:28]

A test for { bitsPerSecond: 0, audioBitsPerSecond: 1000 } to show that this
zeros the audio bitrate would be good.

[mcasas, 2016/02/17 00:20:28]

Done this and a similar one for video.

             if (useVideo)
                 audioBps = overallBps / 10;
             else
                 audioBps = overallBps;
         }
         // Limit audio bitrate values if set explicitly or calculated.
-        if (audioBps) {
+        if (options.hasAudioBitsPerSecond() || options.hasBitsPerSecond()) {

[philipj_slow, 2016/02/15 04:12:28]

It looks like it would be equivalent to keep using audioBps here. Maybe
ASSERT(!audioBps) to show that these things mean the same thing?

[mcasas, 2016/02/17 00:20:29]

Done.

             if (audioBps > kLargestAutoAllocatedOpusBitRate) {
                 context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Clamping calculated audio bitrate (" + String::number(audioBps) + "bps) to the maximum (" + String::number(kLargestAutoAllocatedOpusBitRate) + "bps)"));
                 audioBps = kLargestAutoAllocatedOpusBitRate;
@@ -90,20 +89,17 @@ void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionCont
                 audioBps = kSmallestPossibleOpusBitRate;
             }
         }
-    } else {
-        audioBps = 0;
     }
+
     if (useVideo) {
         // Allocate the remaining |overallBps|, if any, to video.
-        if (overallBps)
+        if (options.hasBitsPerSecond())
             videoBps = overallBps - audioBps;
-        // Clamp the video bit rate if the user has set it explicitly or has used the overall bitrate.
-        if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) {
+        // Clamp the video bit rate. Avoid clamping if the user has not set it explicitly.
+        if ((options.hasVideoBitsPerSecond() || options.hasBitsPerSecond()) && (videoBps < kSmallestPossibleVpxBitRate)) {

[philipj_slow, 2016/02/15 04:12:28]

If you do change the above issue about audio, can you also turn this into the
same structure, i.e. move the (videoBps < kSmallestPossibleVpxBitRate) into the
{} and ASSERT something in the else branch?

[mcasas, 2016/02/17 00:20:28]

Done.

             context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Clamping calculated video bitrate (" + String::number(videoBps) + "bps) to the minimum (" + String::number(kSmallestPossibleVpxBitRate) + "bps)"));
             videoBps = kSmallestPossibleVpxBitRate;
         }
-    } else {
-        videoBps = 0;
     }
 
     *videoBitsPerSecond = videoBps;