MediaRecorder: remove ASSERTS in bitrate clamping.

MediaRecorder: remove ASSERTS in bitrate clamping.

Following up trailing comments in http://crrev.com/1652373004

BUG=585059, 575301, 548290
Committed: https://crrev.com/cad4c9e3b3c9e80bb189059373db528272bca96f
Cr-Commit-Position: refs/heads/master@{#375741}

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

mcasas@chromium.org changed reviewers:
+ philipj@opera.com

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

Philip PTAL.

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (left):

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:101: if
((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps <
kSmallestPossibleVpxBitRate)) {
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]

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (left):

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:101: if
((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps <
kSmallestPossibleVpxBitRate)) {
On 2016/02/08 21:55:56, mcasas wrote:
> 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)."

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.

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: const int
overallBps = std::min(options.bitsPerSecond(), kMaxIntAsUnsigned);
Can you write test cases that exercise this clamping and would hit the
now-removed assertions?

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

PTAL

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (left):

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:101: if
((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps <
kSmallestPossibleVpxBitRate)) {
On 2016/02/09 06:31:22, philipj_OOO_til_Feb15 wrote:
> On 2016/02/08 21:55:56, mcasas wrote:
> > 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)."
> 
> 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? 

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.

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

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.

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1682483002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: const int
overallBps = std::min(options.bitsPerSecond(), kMaxIntAsUnsigned);
On 2016/02/09 06:31:22, philipj_OOO_til_Feb15 wrote:
> Can you write test cases that exercise this clamping and would hit the
> now-removed assertions?

Done, added a few more LayoutTests.

[philipj_slow, 2016-02-15 04:12:28]

lgtm % nits, sorry for the delay (I was OOO Thu-Fri last week)

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html
(right):

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:94:
const kMaxInt32Value = (kMaxUint32Value / 2) - 1;
Just kMaxInt32Value=2147483647 or Math.pow(2,31)-1 since kMaxUint32Value isn't
used?

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:74: if
(options.hasBitsPerSecond()) {
A test for { bitsPerSecond: 0, audioBitsPerSecond: 1000 } to show that this
zeros the audio bitrate would be good.

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:81: if
(options.hasAudioBitsPerSecond() || options.hasBitsPerSecond()) {
It looks like it would be equivalent to keep using audioBps here. Maybe
ASSERT(!audioBps) to show that these things mean the same thing?

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if
((options.hasVideoBitsPerSecond() || options.hasBitsPerSecond()) && (videoBps <
kSmallestPossibleVpxBitRate)) {
If you do change the above issue about audio, can you also turn this into the
same structure, i.e. move the (videoBps < kSmallestPossibleVpxBitRate) into the
{} and ASSERT something in the else branch?

[mcasas, 2016-02-17 00:20:29]

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html
(right):

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:94:
const kMaxInt32Value = (kMaxUint32Value / 2) - 1;
On 2016/02/15 04:12:27, philipj_UTC7 wrote:
> Just kMaxInt32Value=2147483647 or Math.pow(2,31)-1 since kMaxUint32Value isn't
> used? 

Done.

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right):

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:74: if
(options.hasBitsPerSecond()) {
On 2016/02/15 04:12:28, philipj_UTC7 wrote:
> A test for { bitsPerSecond: 0, audioBitsPerSecond: 1000 } to show that this
> zeros the audio bitrate would be good.

Done this and a similar one for video.

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:81: if
(options.hasAudioBitsPerSecond() || options.hasBitsPerSecond()) {
On 2016/02/15 04:12:28, philipj_UTC7 wrote:
> It looks like it would be equivalent to keep using audioBps here. Maybe
> ASSERT(!audioBps) to show that these things mean the same thing?

Done.

https://codereview.chromium.org/1682483002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if
((options.hasVideoBitsPerSecond() || options.hasBitsPerSecond()) && (videoBps <
kSmallestPossibleVpxBitRate)) {
On 2016/02/15 04:12:28, philipj_UTC7 wrote:
> If you do change the above issue about audio, can you also turn this into the
> same structure, i.e. move the (videoBps < kSmallestPossibleVpxBitRate) into
the
> {} and ASSERT something in the else branch?

Done.

[mcasas, 2016-02-17 00:46:30]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-02-17 00:46:31]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1682483002/#ps40001
(title: "philipj@ comments")

[commit-bot: I haz the power, 2016-02-17 00:46:45]

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

[commit-bot: I haz the power, 2016-02-17 01:32:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-17 01:33:13]

Description was changed from

==========
MediaRecorder: remove ASSERTS in bitrate clamping.

Following up trailing comments in http://crrev.com/1652373004

BUG=585059,575301,548290
==========

to

==========
MediaRecorder: remove ASSERTS in bitrate clamping.

Following up trailing comments in http://crrev.com/1652373004

BUG=585059,575301,548290

Committed: https://crrev.com/cad4c9e3b3c9e80bb189059373db528272bca96f
Cr-Commit-Position: refs/heads/master@{#375741}
==========

[commit-bot: I haz the power, 2016-02-17 01:33:14]

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

[philipj_slow, 2016-02-17 02:11:01]

Post-commit LGTM