MediaRecorder: remove ASSERTS in bitrate clamping.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/mediarecorder/MediaRecorder.h"
 
 #include "bindings/core/v8/Dictionary.h"
 #include "core/events/Event.h"
 #include "core/fileapi/Blob.h"
 #include "core/inspector/ConsoleMessage.h"
@@ skipping 38 matching lines @@
 // overrides any specific bitrate, and the UA is free to allocate as desired:
 // here a 90%/10% video/audio is used. In all cases where a value is explicited
 // or calculated, values are clamped in sane ranges.
 // This method throws NotSupportedError.
 void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionContext* context, const MediaRecorderOptions& options, MediaStream* stream, int* audioBitsPerSecond, int* videoBitsPerSecond)
 {
     // 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);

[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();
     const bool useAudio = !stream->getAudioTracks().isEmpty();
 
     if (useAudio) {
         // |overallBps| overrides the specific audio and video bit rates.
         if (overallBps) {
             if (useVideo)
                 audioBps = overallBps / 10;
@@ skipping 12 matching lines @@
                 audioBps = kSmallestPossibleOpusBitRate;
             }
         }
     } else {
         audioBps = 0;
     }
     if (useVideo) {
         // 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;
         }
     } else {
         videoBps = 0;
     }
 
     *videoBitsPerSecond = videoBps;
     *audioBitsPerSecond = audioBps;
     return;
@@ skipping 237 matching lines @@
 DEFINE_TRACE(MediaRecorder)
 {
     visitor->trace(m_stream);
     visitor->trace(m_dispatchScheduledEventRunner);
     visitor->trace(m_scheduledEvents);
     RefCountedGarbageCollectedEventTargetWithInlineData<MediaRecorder>::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink