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

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.

BUG=575301
[1] https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRecorderOptions
[2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/src/src/opus_encoder.c&sq=package:chromium&type=cs&l=169

BUG=575301
Committed: https://crrev.com/8408d85eedde8c47e73ed6aea331dfa28088c16d
Cr-Commit-Position: refs/heads/master@{#370772}

[mcasas, 2016-01-20 05:29:25]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-01-20 05:29:32]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-01-20 05:29:40]

Patchset #1 (id:40001) has been deleted

[mcasas, 2016-01-20 06:15:48]

Patchset #1 (id:60001) has been deleted

[mcasas, 2016-01-20 16:11:50]

Description was changed from

==========
[for bots]

BUG=575301
==========

to

==========
MediaRecorder: wire the bitRate settings in Blink and content

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).

BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

[mcasas, 2016-01-20 16:12:01]

Patchset #1 (id:80001) has been deleted

[mcasas, 2016-01-20 17:17:11]

Description was changed from

==========
MediaRecorder: wire the bitRate settings in Blink and content

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).

BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

to

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
---------------------------------------------------------
Added a LayoutTest checking for throwing when the rates
are too high.



BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

[mcasas, 2016-01-20 17:17:22]

Patchset #1 (id:60002) has been deleted

[mcasas, 2016-01-20 17:18:56]

Description was changed from

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
---------------------------------------------------------
Added a LayoutTest checking for throwing when the rates
are too high.



BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

to

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.



BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

[mcasas, 2016-01-20 17:19:20]

Description was changed from

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.



BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

to

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.


BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

[mcasas, 2016-01-20 17:36:00]

Patchset #1 (id:110001) has been deleted

[mcasas, 2016-01-20 17:38:57]

mcasas@chromium.org changed reviewers:
+ esprehn@chromium.org, miu@chromium.org, peter@chromium.org

[mcasas, 2016-01-20 17:38:58]

(branched off http://crrev.com/1566263002 PS#6 which 
was LGTM'd).

esprehn@, peter@: Moved the bitrate calculation to Blink as instructed :) , also
added a LayoutTest. 

miu@: took your comment from said PS6, the calculation
moved to MediaRecorder.cpp.

PTAL guys :)

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

https://codereview.chromium.org/1610473002/diff/130001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1610473002/diff/130001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:115: const int32_t
bits_per_second_;
nit: all other members have documentation

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:46: //
|{audio,video}BitsPerSecond|, calculating and clamping if needed.
nit: A comment here explaining *why* certain ratios are chosen, e.g. 90% video
and 10% audio, would be useful.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: 
[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.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:58:
*audioBitsPerSecond = options.audioBitsPerSecond();
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.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:72: const
String message = "clamping calculated audio bitrate (" +
String::number(*audioBitsPerSecond) + "bps) to the maximum (" +
String::number(kLargestAutoAllocatedOpusBitRate) + "bps)";
nit: s/clamping/Clamping/. These are sentences.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:90: // Clamp
the video bit rate if there is any configured (explicitly or
nit: it's a bit odd to break this comment given the huge lines that follow it.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:92: if
((*videoBitsPerSecond || options.bitsPerSecond()) && (*videoBitsPerSecond <
kSmallestPossibleVpxBitRate)) {
In which situation is the "(*videoBitsPerSecond || options.bitsPerSecond())"
conditional significant, and does a 0-check not suffice?

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:146: // If bit
rate calculation fails, throw an exception but allow continuing with defaults.
This could do with a few more blank lines for readability.

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

peter@ PTAL

https://codereview.chromium.org/1610473002/diff/130001/content/renderer/media...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1610473002/diff/130001/content/renderer/media...
content/renderer/media/video_track_recorder.cc:115: const int32_t
bits_per_second_;
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> nit: all other members have documentation

Done.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:46: //
|{audio,video}BitsPerSecond|, calculating and clamping if needed.
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> nit: A comment here explaining *why* certain ratios are chosen, e.g. 90% video
> and 10% audio, would be useful.

Commented.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: 
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> [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.
> 

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).

> dictionary MediaRecorderOptions {
>     [EnforceRange] unsigned long audioBitsPerSecond = 0; 
> }
> 
> It also allows this method to be void.

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?

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:58:
*audioBitsPerSecond = options.audioBitsPerSecond();
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> 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.

Done.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:72: const
String message = "clamping calculated audio bitrate (" +
String::number(*audioBitsPerSecond) + "bps) to the maximum (" +
String::number(kLargestAutoAllocatedOpusBitRate) + "bps)";
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> nit: s/clamping/Clamping/. These are sentences.

Done.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:90: // Clamp
the video bit rate if there is any configured (explicitly or
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> nit: it's a bit odd to break this comment given the huge lines that follow it.

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

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:92: if
((*videoBitsPerSecond || options.bitsPerSecond()) && (*videoBitsPerSecond <
kSmallestPossibleVpxBitRate)) {
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> In which situation is the "(*videoBitsPerSecond || options.bitsPerSecond())"
> conditional significant, and does a 0-check not suffice?

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 :(.

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:146: // If bit
rate calculation fails, throw an exception but allow continuing with defaults.
On 2016/01/20 18:35:35, Peter Beverloo wrote:
> This could do with a few more blank lines for readability.

cleaned up.

[miu, 2016-01-21 00:51:05]

lgtm % one thing:

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:55: const
unsigned long kInt32MaxValueAsULong = std::numeric_limits<int32_t>::max();
Please use uint32_t instead of unsigned long.

[esprehn, 2016-01-21 04:00:47]

lgtm

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:51: // This
method throws NotSupportedError.
yay comments, thanks :)

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:52: void
AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionContext*
context, const MediaRecorderOptions& options, bool useAudio, bool useVideo,
int32_t* audioBitsPerSecond, int32_t* videoBitsPerSecond)
take the stream for fewer bools. :)

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:152: const
bool useVideo = stream->getVideoTracks().size() > 0;
remove these, we also don't usually const local bools/ints

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:157:
AllocateVideoAndAudioBitrates(exceptionState, context, options, useAudio,
useVideo, &audioBitsPerSecond, &videoBitsPerSecond);
pass the stream into this instead of the two bools.

[esprehn, 2016-01-21 04:01:06]

Please fix feedback for unsigned long, and passing the stream before landing.

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

lgtm

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: 
On 2016/01/20 21:13:22, mcasas wrote:
> On 2016/01/20 18:35:35, Peter Beverloo wrote:
> > [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.
> > 
> 
> 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).

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:36]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-01-21 18:42:37]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
esprehn@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1610473002/#ps170001
(title: "comments addressed. Added TODO for unsigned->signed change")

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

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: 
On 2016/01/21 11:04:36, Peter Beverloo wrote:
> On 2016/01/20 21:13:22, mcasas wrote:
> > On 2016/01/20 18:35:35, Peter Beverloo wrote:
> > > [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.
> > > 
> > 
> > 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).
> 
> 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?

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.

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:51: // This
method throws NotSupportedError.
On 2016/01/21 04:00:47, esprehn wrote:
> yay comments, thanks :)

Acknowledged.

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:52: void
AllocateVideoAndAudioBitrates(ExceptionState& exceptionState, ExecutionContext*
context, const MediaRecorderOptions& options, bool useAudio, bool useVideo,
int32_t* audioBitsPerSecond, int32_t* videoBitsPerSecond)
On 2016/01/21 04:00:47, esprehn wrote:
> take the stream for fewer bools. :)

Done.

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:55: const
unsigned long kInt32MaxValueAsULong = std::numeric_limits<int32_t>::max();
On 2016/01/21 00:51:05, miu wrote:
> Please use uint32_t instead of unsigned long.

Done.

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:152: const
bool useVideo = stream->getVideoTracks().size() > 0;
On 2016/01/21 04:00:47, esprehn wrote:
> remove these, we also don't usually const local bools/ints

Done.

https://codereview.chromium.org/1610473002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:157:
AllocateVideoAndAudioBitrates(exceptionState, context, options, useAudio,
useVideo, &audioBitsPerSecond, &videoBitsPerSecond);
On 2016/01/21 04:00:47, esprehn wrote:
> pass the stream into this instead of the two bools.

Done.

[commit-bot: I haz the power, 2016-01-21 18:43:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610473002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610473002/170001

[commit-bot: I haz the power, 2016-01-21 20:30:59]

Description was changed from

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.


BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

to

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.


BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

[commit-bot: I haz the power, 2016-01-21 20:31:00]

Committed patchset #3 (id:170001)

[commit-bot: I haz the power, 2016-01-21 20:32:13]

Description was changed from

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.


BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...
  


BUG=575301
==========

to

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

(This is a refry of https://crrev.com/1566263002 with
the bitrate allocation algorithm in Blink ISO Chrome).

Original description: --------------------------------------
This CL updates MediaRecorderOptions to reflect the bitrate
settings in the spec [1]. Those params are wired all the way
down to {Video,Audio}TrackEncoder where they are used to
configure libvpx and opus, resp.

If param |bitsPerSecond| overrides the other two if specified
and the spec leaves down to the UA to divide its value among
video and audio: this CL allocates 90% to video and 10% to audio.

Also added a DCHECK for opus #channels and sampling rate,
which opus hardcode-limits in a pretty clumsy way (feast
your eyes in [2]).
--------------------------------------------------------------
Diffs with this previous CL:
- Chrome gets just the consolidated audio and video bit rates.
- The bit rate calculation micro algorithm is in MediaRecorder.cpp
- Added a LayoutTest checking for throwing when the rates
are too high.

BUG=575301
[1]
https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...

BUG=575301

Committed: https://crrev.com/8408d85eedde8c47e73ed6aea331dfa28088c16d
Cr-Commit-Position: refs/heads/master@{#370772}
==========

[commit-bot: I haz the power, 2016-01-21 20:32:15]

Patchset 3 (id:??) landed as
https://crrev.com/8408d85eedde8c47e73ed6aea331dfa28088c16d
Cr-Commit-Position: refs/heads/master@{#370772}

[brucedawson, 2016-01-26 18:28:46]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-01-26 18:28:47]

https://codereview.chromium.org/1610473002/diff/170001/content/renderer/media...
File content/renderer/media/audio_track_recorder.cc (right):

https://codereview.chromium.org/1610473002/diff/170001/content/renderer/media...
content/renderer/media/audio_track_recorder.cc:150: DCHECK((params.sample_rate()
!= 48000) || (params.sample_rate() != 24000) ||
This DCHECK is accidentally tautological. There are no values of sample_rate()
that could cause it to trigger.

The "!=" operators should all be changed to "==".

This was found by the VC++ /analyze builder which reported:
audio_track_recorder.cc(150) : warning C6289: Incorrect operator:  mutual
exclusion over || is always a non-zero constant.  Did you intend to use &&
instead?

Note that its suggested fix is incorrect, but it is correct that there is a bug.

Note also that the /analyze builder does *not* automatically report these
warnings due to the very high false positive rate. I look at the new warnings a
few times a week and forward them. So, there is no fyi bot whose output you
missed.

[mcasas, 2016-01-26 18:45:01]

On 2016/01/26 18:28:47, brucedawson wrote:
>
https://codereview.chromium.org/1610473002/diff/170001/content/renderer/media...
> File content/renderer/media/audio_track_recorder.cc (right):
> 
>
https://codereview.chromium.org/1610473002/diff/170001/content/renderer/media...
> content/renderer/media/audio_track_recorder.cc:150:
DCHECK((params.sample_rate()
> != 48000) || (params.sample_rate() != 24000) ||
> This DCHECK is accidentally tautological. There are no values of sample_rate()
> that could cause it to trigger.
> 
> The "!=" operators should all be changed to "==".
> 
> This was found by the VC++ /analyze builder which reported:
> audio_track_recorder.cc(150) : warning C6289: Incorrect operator:  mutual
> exclusion over || is always a non-zero constant.  Did you intend to use &&
> instead?
> 
> Note that its suggested fix is incorrect, but it is correct that there is a
bug.
> 
> Note also that the /analyze builder does *not* automatically report these
> warnings due to the very high false positive rate. I look at the new warnings
a
> few times a week and forward them. So, there is no fyi bot whose output you
> missed.

Thanks! This code is being removed as part of
https://codereview.chromium.org/1579693006/
(under review).