MediaRecorder: add and wire read {video,audio}BitsPerSecond props

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"
 #include "modules/EventTargetModules.h"
 #include "modules/mediarecorder/BlobEvent.h"
 #include "platform/ContentType.h"
 #include "platform/NotImplemented.h"
 #include "platform/blob/BlobData.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebMediaStream.h"
+#include <algorithm>
 
 namespace blink {
 
 namespace {
 
 // Boundaries of Opus bitrate from https://www.opus-codec.org/.
 const int kSmallestPossibleOpusBitRate = 6000;
 const int kLargestAutoAllocatedOpusBitRate = 128000;
 
 // Smallest Vpx bitrate that can be requested.
@@ skipping 14 matching lines @@
     return String();
 }
 
 // Allocates the requested bit rates from |bitrateOptions| into the respective
 // |{audio,video}BitsPerSecond| (where a value of zero indicates Platform to use
 // whatever it sees fit). If |options.bitsPerSecond()| is specified, it
 // 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, int32_t* audioBitsPerSecond, int32_t* videoBitsPerSecond)
+void AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionContext* context, const MediaRecorderOptions& options, MediaStream* stream, int* audioBitsPerSecond, int* videoBitsPerSecond)
 {
-    // Throw if any value would overflow an int32_t.
+    // Clamp incoming values to a signed integer's maximum.
     // TODO(mcasas): This section would no be needed if the bit rates are signed, see https://github.com/w3c/mediacapture-record/issues/48.

[philipj_slow, 2016/02/04 03:03:10]

I like https://github.com/w3c/mediacapture-record/issues/48 want to fix that?
Integer overflow and stuff is so un-fun.

[mcasas, 2016/02/05 02:39:14]

I do! Will follow up with Gecko and hopefully deal with all 
this once and for all. For the time being, let's land this?

[philipj_slow, 2016/02/05 07:48:49]

Sounds good.

-    const uint32_t kInt32MaxValueAsULong = std::numeric_limits<int32_t>::max();
-    if (options.bitsPerSecond() >= kInt32MaxValueAsULong) {
-        exceptionState.throwDOMException(NotSupportedError, "Overall bit rate too large " + String::number(options.bitsPerSecond()) + "bps)");
-        return;
-    }
-    if (options.bitsPerSecond() == 0) {
-        if (options.videoBitsPerSecond() >= kInt32MaxValueAsULong) {
-            exceptionState.throwDOMException(NotSupportedError, "Video bit rate too large " + String::number(options.videoBitsPerSecond()) + "bps)");
-            return;
-        }
-        if (options.audioBitsPerSecond() >= kInt32MaxValueAsULong) {
-            exceptionState.throwDOMException(NotSupportedError, "Audio bit rate too large " + String::number(options.audioBitsPerSecond()) + "bps)");
-            return;
-        }
-    }
+    const unsigned kMaxIntAsUnsigned = std::numeric_limits<int>::max();
 
-    int32_t videoBps = options.videoBitsPerSecond();
-    int32_t audioBps = options.audioBitsPerSecond();
-    bool useVideo = stream->getVideoTracks().size() > 0;
-    bool useAudio = stream->getAudioTracks().size() > 0;
+    ASSERT(options.bitsPerSecond() < kMaxIntAsUnsigned);

[philipj_slow, 2016/02/04 03:03:10]

Actually you can't assert this, a script can hit this easily by just passing -1,
which first get converted to 2^32-1 by bindings, and then fails here. So tests
for negative values wouldn't hurt :)

This is another reason to not use unsigned long in the spec, after all why
should -1 wrap and clamp to the maximum value instead of clamping to the minimum
value?

[mcasas, 2016/02/05 02:39:14]

Done.

+    const int overallBps = std::min(options.bitsPerSecond(), kMaxIntAsUnsigned);
+
+    ASSERT(overallBps != 0 || options.videoBitsPerSecond() < kMaxIntAsUnsigned);

[philipj_slow, 2016/02/04 03:03:10]

These asserts would have to go too.

[mcasas, 2016/02/05 02:39:14]

Done, I think.

+    int videoBps = std::min(options.videoBitsPerSecond(), kMaxIntAsUnsigned);
+
+    ASSERT(overallBps != 0 || options.audioBitsPerSecond() < kMaxIntAsUnsigned);
+    int audioBps = std::min(options.audioBitsPerSecond(), kMaxIntAsUnsigned);
+
+    const bool useVideo = stream->getVideoTracks().size() > 0;
+    const bool useAudio = stream->getAudioTracks().size() > 0;
 
     if (useAudio) {
-        // |options.bitsPerSecond()| overrides the specific audio and video bit rates.
-        if (options.bitsPerSecond()) {
+        // |overallBps| overrides the specific audio and video bit rates.
+        if (overallBps) {
             if (useVideo)
-                audioBps = options.bitsPerSecond() / 10;
+                audioBps = overallBps / 10;
             else
-                audioBps = options.bitsPerSecond();
+                audioBps = overallBps;
         }
         // Limit audio bitrate values if set explicitly or calculated.
         if (audioBps) {
             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;
             }
 
             if (audioBps < kSmallestPossibleOpusBitRate) {
                 context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, "Clamping calculated audio bitrate (" + String::number(audioBps) + "bps) to the minimum (" + String::number(kSmallestPossibleOpusBitRate) + "bps)"));
                 audioBps = kSmallestPossibleOpusBitRate;
             }
         }
     } else {
         audioBps = 0;
     }
     if (useVideo) {
-        // Allocate the remaining overall |options.bitsPerSecond()|, if any, to video.
-        if (options.bitsPerSecond())
-            videoBps = options.bitsPerSecond() - audioBps;
+        // Allocate the remaining overall |overallBps|, if any, to video.

[philipj_slow, 2016/02/04 03:03:10]

Can drop "overall" now.

[mcasas, 2016/02/05 02:39:14]

Done.

+        if (overallBps)
+            videoBps = overallBps - audioBps;
         // Clamp the video bit rate if the user has set explicitly the video bit rate or has used the overall bitrate.
-        if ((options.videoBitsPerSecond() > 0 || options.bitsPerSecond() > 0) && (videoBps < kSmallestPossibleVpxBitRate)) {
+        if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) {

[philipj_slow, 2016/02/04 03:03:10]

Look at videoBps here instead? It'd be nice to deal with the options dict at the
top only.

[mcasas, 2016/02/05 02:39:14]

In this case the idea is to check if the original 
options.videoBitsPerSecond() is set, indicating
that the user has explicited the video bitRate.
This is to prevent clamping the |videoBps| to
kSmallestPossibleVpxBitRate when the user 
has passed no explicit |videoBitsPerSecond|.

So, have to look at the original |videoBitsPerSecond|.

[philipj_slow, 2016/02/05 07:48:49]

Oh, I see. If the IDL matched the spec and didn't have default values for
*BitsPerSecond, this would become something involving
options.hasVideoBitsPerSecond() instead. Add a TODO in MediaRecorderOptions.idl
or fix now?

[mcasas, 2016/02/06 00:26:25]

Using hasVideoBitsPerSecond().

[philipj_slow, 2016/02/08 09:39:44]

This bit is still using options.videoBitsPerSecond() in what landed, did you
forget to upload a change? It still seems to me that it should simply look at
videoBps since that is what is being clamped. Is it about avoiding the console
message, perhaps? Since the clamped bitrate can be observed in the MediaRecorder
instance, foregoing the console message completely seems fine to me, it cannot
be preserved if you sync the MediaRecorderOptions.idl with the spec.

[mcasas, 2016/02/08 21:54:40]

Darn, something was missing indeed.
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).

             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 17 matching lines @@
     return recorder;
 }
 
 MediaRecorder::MediaRecorder(ExecutionContext* context, MediaStream* stream, const MediaRecorderOptions& options, ExceptionState& exceptionState)
     : ActiveDOMObject(context)
     , m_stream(stream)
     , m_streamAmountOfTracks(stream->getTracks().size())
     , m_mimeType(options.mimeType())
     , m_stopped(true)
     , m_ignoreMutedMedia(true)
+    , m_audioBitsPerSecond(0)
+    , m_videoBitsPerSecond(0)
     , m_state(State::Inactive)
     , m_dispatchScheduledEventRunner(AsyncMethodRunner<MediaRecorder>::create(this, &MediaRecorder::dispatchScheduledEvent))
 {
     ASSERT(m_stream->getTracks().size());
 
     m_recorderHandler = adoptPtr(Platform::current()->createMediaRecorderHandler());
     ASSERT(m_recorderHandler);
 
     if (!m_recorderHandler) {
         exceptionState.throwDOMException(NotSupportedError, "No MediaRecorder handler can be created.");
         return;
     }
 
-    int32_t audioBitsPerSecond = 0;
-    int32_t videoBitsPerSecond = 0;
-    AllocateVideoAndAudioBitrates(exceptionState, context, options, stream, &audioBitsPerSecond, &videoBitsPerSecond);
+    AllocateVideoAndAudioBitrates(exceptionState, context, options, stream, &m_audioBitsPerSecond, &m_videoBitsPerSecond);
 
     const ContentType contentType(m_mimeType);
-    if (!m_recorderHandler->initialize(this, stream->descriptor(), contentType.type(), contentType.parameter("codecs"), audioBitsPerSecond, videoBitsPerSecond)) {
+    if (!m_recorderHandler->initialize(this, stream->descriptor(), contentType.type(), contentType.parameter("codecs"), m_audioBitsPerSecond, m_videoBitsPerSecond)) {
         exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native MediaRecorder the type provided (" + m_mimeType + ") is not supported.");
         return;
     }
     m_stopped = false;
 }
 
 String MediaRecorder::state() const
 {
     return stateToString(m_state);
 }
@@ skipping 181 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