MediaRecorder: wire the bitRate settings in Blink and content (2nd go)

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 d92ecf7d97ad00a489c532be4af2d30d2025720c..44dcbd6f651931b98b1ae356a0099655fa7f734e 100644
--- a/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
+++ b/third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp
@@ -7,6 +7,7 @@
 #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"
@@ -19,6 +20,13 @@ 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.
+const int kSmallestPossibleVpxBitRate = 100000;
+
 String stateToString(MediaRecorder::State state)
 {
     switch (state) {
@@ -34,6 +42,64 @@ String stateToString(MediaRecorder::State state)
     return String();
 }
 
+// Allocates the requested bit rates from |bitrateOptions| into the respective
+// |{audio,video}BitsPerSecond|, calculating and clamping if needed.

[Peter Beverloo, 2016/01/20 18:35:35]

nit: A comment here explaining *why* certain ratios are chosen, e.g. 90% video
and 10% audio, would be useful.

[mcasas, 2016/01/20 21:13:22]

Commented.

+bool AllocateVideoAndAudioBitrates(ExecutionContext* context, const MediaRecorderOptions& options, bool useAudio, bool useVideo, int32_t* audioBitsPerSecond, int32_t* videoBitsPerSecond)
+{
+    // Check that |options| don't overflow an int32_t.
+    const unsigned long kInt32MaxValueAsULong = std::numeric_limits<int32_t>::max();
+    if (options.bitsPerSecond() >= kInt32MaxValueAsULong || (options.bitsPerSecond() == 0 && (options.videoBitsPerSecond() >= kInt32MaxValueAsULong || options.audioBitsPerSecond() >= kInt32MaxValueAsULong))) {
+        const String message = "bitrates too large (audio: " + String::number(options.audioBitsPerSecond()) + "bps, video: " + String::number(options.videoBitsPerSecond()) + "bps, overall: " + String::number(options.bitsPerSecond()) + "bps)";
+        context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, ErrorMessageLevel, message));
+        return false;
+    }
+

[Peter Beverloo, 2016/01/20 18:35:35]

[Comment applies to lines 49-55.]

I think this is not super obvious. The conditional is long and the error message
only reflects the given settings, not *why* they're too large.

We support the [EnforceRange] WebIDL annotation that will make sure the value is
in-range and throw a type error when a value is out-of-bounds. If the int32
restriction doesn't just apply to Chrome, you could amend the spec to include
it, too.

dictionary MediaRecorderOptions {
    [EnforceRange] unsigned long audioBitsPerSecond = 0; 
}

It also allows this method to be void.

[mcasas, 2016/01/20 21:13:22]

I'd love to but Gecko had it as ulong beforehand, and we
have to use int32_t for arithmetic reasons. 

Is there a way to EnforceRange as int32_t while the member
is actually ulong? (In case the answer is no, I polished this
area, see below).
Changed the method to void, and instead of logging an
Error, returning false, throwing an Exception from the 
caller site, this method throws directly. 

A bit counter intuitive perhaps, the method throws but
the execution would continue with by-default bit rate
values. Does this make blink-sense?

[Peter Beverloo, 2016/01/21 11:04:36]

It is unfortunate that we can't rely on the standard number types and instead
end up having an unsigned 31-bit integer. Are there really compatibility
concerns here, anything we can measure?

[mcasas, 2016/01/21 18:42:46]

I filed https://github.com/w3c/mediacapture-record/issues/48
to see if we can change these fellas to integer, and added TODOs
here and in MediaRecorderOptions.idl.

+    *videoBitsPerSecond = options.videoBitsPerSecond();
+    *audioBitsPerSecond = options.audioBitsPerSecond();

[Peter Beverloo, 2016/01/20 18:35:35]

nit: There's a lot of stars in the code. Might be worthwhile just storing these
values in a local and only assigning them to the out arguments after their
values are known.

[mcasas, 2016/01/20 21:13:22]

Done.

+
+    // |options.bitsPerSecond()| overrides the specific audio and video bit
+    // rates, and we are free to allocate as desired.
+    if (useAudio) {
+        if (options.bitsPerSecond()) {
+            if (useVideo)
+                *audioBitsPerSecond = options.bitsPerSecond() / 10;
+            else
+                *audioBitsPerSecond = options.bitsPerSecond();
+        }
+        // Limit audio bitrate values if they are too large or too small.
+        if (*audioBitsPerSecond) {
+            if (*audioBitsPerSecond > kLargestAutoAllocatedOpusBitRate) {
+                const String message = "clamping calculated audio bitrate (" + String::number(*audioBitsPerSecond) + "bps) to the maximum (" + String::number(kLargestAutoAllocatedOpusBitRate) + "bps)";

[Peter Beverloo, 2016/01/20 18:35:35]

nit: s/clamping/Clamping/. These are sentences.

[mcasas, 2016/01/20 21:13:22]

Done.

+                context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
+                *audioBitsPerSecond = kLargestAutoAllocatedOpusBitRate;
+            }
+
+            if (*audioBitsPerSecond < kSmallestPossibleOpusBitRate) {
+                const String message = "clamping calculated audio bitrate (" + String::number(*audioBitsPerSecond) + "bps) to the minimum (" + String::number(kSmallestPossibleOpusBitRate) + "bps)";
+                context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
+                *audioBitsPerSecond = kSmallestPossibleOpusBitRate;
+            }
+        }
+    } else {
+        *audioBitsPerSecond = 0;
+    }
+    // If there is video bit rate allocated it should be larger than a minimum.
+    if (useVideo) {
+        if (options.bitsPerSecond())
+            *videoBitsPerSecond = options.bitsPerSecond() - *audioBitsPerSecond;
+        // Clamp the video bit rate if there is any configured (explicitly or

[Peter Beverloo, 2016/01/20 18:35:35]

nit: it's a bit odd to break this comment given the huge lines that follow it.

[mcasas, 2016/01/20 21:13:22]

Yup, sorry, came from Cr side 80-char limit. Done.

+        // implicitly via |options.bitsPerSecond()|).
+        if ((*videoBitsPerSecond || options.bitsPerSecond()) && (*videoBitsPerSecond < kSmallestPossibleVpxBitRate)) {

[Peter Beverloo, 2016/01/20 18:35:35]

In which situation is the "(*videoBitsPerSecond || options.bitsPerSecond())"
conditional significant, and does a 0-check not suffice?

[mcasas, 2016/01/20 21:13:22]

It's a peculiar case indeed, as miu@ detailed:
- the user does not set the overall bitsPerSecond() nor the 
specific videoBitsPerSecond(), we should not force a 
minimum on it.
- but if the user has set bitsPerSecond() to such a small 
value that most of it goes to audio, we want to clamp the 
video bitrate (and violate bitsPerSecond()).

IOW: clamp if:
- the user has set the video bitrate 
or
- has used the overall bitsPerSecond().

(Will try to explain in the code.)

All these was found during unit testing, and should 
be made explicit in a LayoutTest, sadly will have to 
wait  until the Spec is finished I'm afraid :(.

+            const String message = "clamping calculated video bitrate (" + String::number(*videoBitsPerSecond) + "bps) to the minimum (" + String::number(kSmallestPossibleVpxBitRate) + "bps)";
+            context->addConsoleMessage(ConsoleMessage::create(JSMessageSource, WarningMessageLevel, message));
+            *videoBitsPerSecond = kSmallestPossibleVpxBitRate;
+        }
+    } else {
+        *videoBitsPerSecond = 0;
+    }
+    return true;
+}
+
 } // namespace
 
 MediaRecorder* MediaRecorder::create(ExecutionContext* context, MediaStream* stream, ExceptionState& exceptionState)
@@ -67,14 +133,24 @@ MediaRecorder::MediaRecorder(ExecutionContext* context, MediaStream* stream, con
     m_recorderHandler = adoptPtr(Platform::current()->createMediaRecorderHandler());
     ASSERT(m_recorderHandler);
 
-    // We deviate from the spec by not returning |UnsupportedOption|, see https://github.com/w3c/mediacapture-record/issues/18
     if (!m_recorderHandler) {
         exceptionState.throwDOMException(NotSupportedError, "No MediaRecorder handler can be created.");
         return;
     }
-    ContentType contentType(m_mimeType);
-    if (!m_recorderHandler->initialize(this, stream->descriptor(), contentType.type(), contentType.parameter("codecs"))) {
-        exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native MediaRecorder, the type provided " + m_mimeType + "is unsupported." );
+
+    const ContentType contentType(m_mimeType);
+    const bool useVideo = stream->getVideoTracks().size() > 0;
+    const bool useAudio = stream->getAudioTracks().size() > 0;
+    int32_t audioBitsPerSecond = 0;
+    int32_t videoBitsPerSecond = 0;
+    // If bit rate calculation fails, throw an exception but allow continuing with defaults.

[Peter Beverloo, 2016/01/20 18:35:35]

This could do with a few more blank lines for readability.

[mcasas, 2016/01/20 21:13:22]

cleaned up.

+    if (!AllocateVideoAndAudioBitrates(context, options, useAudio, useVideo, &audioBitsPerSecond, &videoBitsPerSecond)) {
+        const String message = "target bitrates too large (audio: " + String::number(options.audioBitsPerSecond()) + "bps, video: " + String::number(options.videoBitsPerSecond()) + "bps, overall: " + String::number(options.bitsPerSecond()) + "bps)";
+        exceptionState.throwDOMException(NotSupportedError, message);
+    }
+
+    if (!m_recorderHandler->initialize(this, stream->descriptor(), contentType.type(), contentType.parameter("codecs"), audioBitsPerSecond, videoBitsPerSecond)) {
+        exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native MediaRecorder, the bitrates supplied, or the type provided (" + m_mimeType + ") are unsupported.");
         return;
     }
     m_stopped = false;