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..15200b5d9ac8f2e56bc764c79b93c39b3ce3fff1 100644
--- a/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
+++ b/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
@@ -56,15 +56,8 @@ void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionCont
     // 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);

[philipj_slow, 2016/02/09 06:31:22]

Can you write test cases that exercise this clamping and would hit the
now-removed assertions?

[mcasas, 2016/02/12 02:11:38]

Done, added a few more LayoutTests.

-
-    // 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();
@@ -97,7 +90,7 @@ void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionCont
         // Allocate the remaining |overallBps|, if any, to video.
         if (overallBps)
             videoBps = overallBps - audioBps;
-        // Clamp the video bit rate if the user has set it explicitly or has used the overall bitrate.
+        // Clamp the video bit rate. Avoid clamping if the user has not set it explicitly.
         if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) {

[mcasas, 2016/02/08 21:55:56]

From https://codereview.chromium.org/1652373004/:
"What I (think I) wrote is that here:
a) cannot use hasVideoBitsPerSecond() since that 
would not give me the case in which the user sets
an explicit videoBitsPerSecond=0
b) the extra |options.videoBitsPerSecond > 0| is
there to prevent clamping the video bit rate to a 
minimum when the user has not set it or has set 
it to zero (meaning: let the platform decide)."

[philipj_slow, 2016/02/09 06:31:22]

The trouble is that because MediaRecorderOptions.idl has default values (0) for
these members (the spec does not) there's no way to distinguish between {} and
the explicit { videoBitsPerSecond: 0 } options dictionaries at this point. This
code only checks for >0 and so isn't affected by this, but it sounds like you
*wanted* to treat explicit videoBitsPerSecond=0 differently? If you want to show
this console message for { videoBitsPerSecond: 0 } just like for {
videoBitsPerSecond: 1 }, then some change will be needed here.

Otherwise, this code will have to be adapted when MediaRecorderOptions.idl is
synced with the spec, so make sure that there are test cases that will assert if
the defaults are removed but this code isn't updated.

[mcasas, 2016/02/12 02:11:38]

I see. I removed the by-default =0 in MediaRecorderOptions
and used has{,Video,Audio}BitsPerSecond() in this code
where it makes sense. I think the micro algorithm is much
clearer in this case.
Added more layout tests and/or comments to make
evident the difference between not-defined values and 
value defined (by the user) to be zero.

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