Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1652373004:
MediaRecorder: add and wire read {video,audio}BitsPerSecond props (Closed)
DescriptionMediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
Committed: https://crrev.com/e608797d019863e635cfdcac30d44f6304f81904
Cr-Commit-Position: refs/heads/master@{#373989}
Patch Set 1 #
Total comments: 12
Patch Set 2 : reviewers@ comments #
Total comments: 22
Patch Set 3 : philipj@opera.com comments #
Total comments: 19
Patch Set 4 : philipj@ second round of comments #
Total comments: 5
Patch Set 5 : philipj@ nits #
Total comments: 1
Messages
Total messages: 29 (12 generated)
mcasas@chromium.org changed reviewers: + avayvod@chromium.org
avayvod@ PTAL (peter@ FYI).
lgtm with nits I'm afraid I'm not an OWNER here yet (Peter?) It seems like you'll need to update the public API surface tests which require API OWNERS approval IIRC. Are you going to send an Intent to Ship for these properties to blink-dev? https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:7: // bitrates for video, audio and for both [1]. The first bunch checks that an nit: s/bunch/bunch of tests/ https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:8: // off-the-limit bitrate causes an exception thrown. The following bunch checks nit: s/bunch/tests https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:10: // honored or clamped as expeected, and how the overall bitsPerSecond is divided nit: s/expeected/expected https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:49: unsigned long audioBitsPerSecond() const { return m_audioBitsPerSecond; } what about bitsPerSecond() mentioned in the GitHub issue?
peter@chromium.org changed reviewers: + peter@chromium.org
As far as I can see only audioBitsPerSecond and videoBitsPerSecond are exposed (which makes sense, since bitsPerSecond is only used to calculate their value). You'll indeed need API OWNERS approval since the global interface listings will have to be updated for these new properties, which raises Anton's question. lgtm, but please follow due process :). https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:75: const kTooSmallBps = 1E3; This is an odd way to note these numbers (especially 128E3). Could we just spell them out? Makes it much easier to search for them. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:98: int32_t m_videoBitsPerSecond = 0; It seems to me that we should either initialize all scalar values in-line (like these), or all in the initialization list of the constructor, not a mixture of both.
Description was changed from
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
==========
to
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301
==========
Description was changed from
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301
==========
to
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
==========
mcasas@chromium.org changed reviewers: + philipj@opera.com
philipj@opera.com: PTAL at the added two r/o attributes, following the discussion in the mentioned bug. peter@, whywhat@, I thought for such a tiny modification to the API, which is basically a catch-up with the spec, I would not need to pass by blink-dev. WDYT? https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:7: // bitrates for video, audio and for both [1]. The first bunch checks that an On 2016/02/02 22:19:54, whywhat wrote: > nit: s/bunch/bunch of tests/ Done. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:8: // off-the-limit bitrate causes an exception thrown. The following bunch checks On 2016/02/02 22:19:54, whywhat wrote: > nit: s/bunch/tests Done. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:10: // honored or clamped as expeected, and how the overall bitsPerSecond is divided On 2016/02/02 22:19:54, whywhat wrote: > nit: s/expeected/expected Done. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:75: const kTooSmallBps = 1E3; On 2016/02/02 22:43:51, Peter Beverloo wrote: > This is an odd way to note these numbers (especially 128E3). Could we just spell > them out? Makes it much easier to search for them. Done. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:49: unsigned long audioBitsPerSecond() const { return m_audioBitsPerSecond; } On 2016/02/02 22:19:54, whywhat wrote: > what about bitsPerSecond() mentioned in the GitHub issue? As peter@ says, the idea here is to show the calculated and/or truncated values. |bitsPerSecond| overrides and is divided into audio and video bitrates in a UA dependent way, hence the interest in reading them back etc. https://codereview.chromium.org/1652373004/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:98: int32_t m_videoBitsPerSecond = 0; On 2016/02/02 22:43:51, Peter Beverloo wrote: > It seems to me that we should either initialize all scalar values in-line (like > these), or all in the initialization list of the constructor, not a mixture of > both. Done.
https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:19: assert_throws("NotSupportedError", The spec doesn't allow for NotSupportedError to be thrown for bitrate reasons. You could change the spec, but it seems like an interop hazard to throw exceptions based on unspecified restrictions on bitrate, better to just clamp the bitrate to the supported range of bitrates I'd say. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:45: function makeExpectedBitrateAsyncTest(options, message, expectedVideoBps, expectedAudioBps) { s/message/name/ or testName (in existing test too) https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:45: function makeExpectedBitrateAsyncTest(options, message, expectedVideoBps, expectedAudioBps) { How about making expected a dictionary too, so that it's clearer at the call site what's what? https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:46: return async_test(function(test) { No reason to return the test, it'll run anyway. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:47: const constraints = {video: true, audio: true}; No tests for when either audio or video are not wanted, but a bitrate for them is still requested? https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:54: assert_unreached('Exception while creating MediaRecorder: ' + e); Don't really need this, any exception thrown will fail the test, and the stacktrace will point to where it happened. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:62: const onError = this.step_func(function() { can be simplified to test.unreached_func('Error creating MediaStream.') https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:70: assert_unreached('Exception launching getUserMedia(): ' + e); Dito, can drop try catch https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html:22: assert_equals(recorder.videoBitsPerSecond, 0); That seems weird, why shouldn't this return the default bitrate being targeted, just like mimeType should return the default MIME type chosen? https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:161: if (!m_recorderHandler->initialize(this, stream->descriptor(), contentType.type(), contentType.parameter("codecs"), m_audioBitsPerSecond, m_videoBitsPerSecond)) { Assuming that the internal representation isn't unsigned long (style guide says no), I think you'd do best to keep the local audio/videoBitsPerSecond local variables here, and ASSERT that they are in the correct range before assigning to m_*. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:97: int32_t m_audioBitsPerSecond; Why int32_t here and in WebMediaRecorderHandler? Per https://google.github.io/styleguide/cppguide.html#Integer_Types plain int seems the right thing, and int32_t members are very rare in Blink.
On 2016/02/03 02:49:59, mcasas wrote: > mailto:philipj@opera.com: PTAL at the added two r/o attributes, > following the discussion in the mentioned bug. > > peter@, whywhat@, I thought for such a tiny > modification to the API, which is basically a > catch-up with the spec, I would not need to > pass by blink-dev. WDYT? You are the spec editor and this doesn't seem to be implemented in Gecko, so I wouldn't call it catch-up in the sense that a lot of small changes are. How about sending a message to the existing "Intent to Ship: Media Stream Recording API" thread pointing to the spec (http://w3c.github.io/mediacapture-record/MediaRecorder.html is out of sync, so first sync) and that Mozilla (@mreavy) is on board with this change, and if there are no objections in 2 days, you'll ship these bits too? If you expect there to be a long series of small additions that weren't in the spec when this was last on blink-dev, perhaps an all-encompassing "Intent to Implement and Ship" for that, listing the expected changes, would be good. Your call.
Patchset #3 (id:40001) has been deleted
philipj@opera.com PTAL Still pending blink-dev post OK -- or lack of NOKs :) Will follow up w/ Gecko, but TBH I see this addition as trivial. Updated W3C spec editor's draft [1]. [1] http://w3c.github.io/mediacapture-record/MediaRecorder.html https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:19: assert_throws("NotSupportedError", On 2016/02/03 03:31:04, philipj_UTC7 wrote: > The spec doesn't allow for NotSupportedError to be thrown for bitrate reasons. > You could change the spec, but it seems like an interop hazard to throw > exceptions based on unspecified restrictions on bitrate, better to just clamp > the bitrate to the supported range of bitrates I'd say. Hmm that makes sense. I propose not throwing, ASSERT()ing here and clamping to its maximum value. Sounds good? (I'll remove the expect-throw layouttest). https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:45: function makeExpectedBitrateAsyncTest(options, message, expectedVideoBps, expectedAudioBps) { On 2016/02/03 03:31:04, philipj_UTC7 wrote: > s/message/name/ or testName (in existing test too) Done. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:45: function makeExpectedBitrateAsyncTest(options, message, expectedVideoBps, expectedAudioBps) { On 2016/02/03 03:31:05, philipj_UTC7 wrote: > How about making expected a dictionary too, so that it's clearer at the call > site what's what? Done. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:46: return async_test(function(test) { On 2016/02/03 03:31:05, philipj_UTC7 wrote: > No reason to return the test, it'll run anyway. Done. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:54: assert_unreached('Exception while creating MediaRecorder: ' + e); On 2016/02/03 03:31:05, philipj_UTC7 wrote: > Don't really need this, any exception thrown will fail the test, and the > stacktrace will point to where it happened. Done. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:62: const onError = this.step_func(function() { On 2016/02/03 03:31:04, philipj_UTC7 wrote: > can be simplified to test.unreached_func('Error creating MediaStream.') Nice! Done. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:70: assert_unreached('Exception launching getUserMedia(): ' + e); On 2016/02/03 03:31:04, philipj_UTC7 wrote: > Dito, can drop try catch More succint, done. Took the chance and cleaned up/homogeneise the other LayoutTests. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html:22: assert_equals(recorder.videoBitsPerSecond, 0); On 2016/02/03 03:31:05, philipj_UTC7 wrote: > That seems weird, why shouldn't this return the default bitrate being targeted, > just like mimeType should return the default MIME type chosen? Because the bitrate, when left by JS as undefined, means is left to the encoder. In this case, it basically means 0 to signify that the encoders were left to decide it (and update it as they see fit). Sense? https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.h:97: int32_t m_audioBitsPerSecond; On 2016/02/03 03:31:05, philipj_UTC7 wrote: > Why int32_t here and in WebMediaRecorderHandler? Per > https://google.github.io/styleguide/cppguide.html#Integer_Types plain int seems > the right thing, and int32_t members are very rare in Blink. Replaced throughout.
https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-bitrates.html:19: assert_throws("NotSupportedError", On 2016/02/03 23:50:26, mcasas wrote: > On 2016/02/03 03:31:04, philipj_UTC7 wrote: > > The spec doesn't allow for NotSupportedError to be thrown for bitrate reasons. > > You could change the spec, but it seems like an interop hazard to throw > > exceptions based on unspecified restrictions on bitrate, better to just clamp > > the bitrate to the supported range of bitrates I'd say. > > Hmm that makes sense. I propose not throwing, ASSERT()ing > here and clamping to its maximum value. Sounds good? > (I'll remove the expect-throw layouttest). Clamping sounds good, yep. https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html (right): https://codereview.chromium.org/1652373004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-creation.html:22: assert_equals(recorder.videoBitsPerSecond, 0); On 2016/02/03 23:50:27, mcasas wrote: > On 2016/02/03 03:31:05, philipj_UTC7 wrote: > > That seems weird, why shouldn't this return the default bitrate being > targeted, > > just like mimeType should return the default MIME type chosen? > > Because the bitrate, when left by JS as undefined, > means is left to the encoder. In this case, it basically > means 0 to signify that the encoders were left to > decide it (and update it as they see fit). Sense? Well, the encoder isn't really targeting a zero bitrate, why not set it to the actually targeted bitrate? The reasoning being that creating a new MediaRecorder({ audioBitsPerSecond: other.audioBitsPerSecond, videoBitsPerSecond: other.videoBitsPerSecond }) should result exactly the same bitrate config as other, and it's reasonable to leave zero to mean "minimum bitrate possible." https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:26: async_test(function(test) { Using this as you had done is fine too, IMHO, but so is the new style. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:49: "recorder throws InvalidStateError if requestData() comes in invalid state"); Maybe s/comes in invalid state/while state is not "recording"/, assuming that is what is being tested. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: // TODO(mcasas): This section would no be needed if the bit rates are signed, see https://github.com/w3c/mediacapture-record/issues/48. I like https://github.com/w3c/mediacapture-record/issues/48 want to fix that? Integer overflow and stuff is so un-fun. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: ASSERT(options.bitsPerSecond() < kMaxIntAsUnsigned); 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? https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:62: ASSERT(overallBps != 0 || options.videoBitsPerSecond() < kMaxIntAsUnsigned); These asserts would have to go too. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:95: // Allocate the remaining overall |overallBps|, if any, to video. Can drop "overall" now. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { Look at videoBps here instead? It'd be nice to deal with the options dict at the top only.
philipj@ PTAL https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:26: async_test(function(test) { On 2016/02/04 03:03:10, philipj_UTC7 wrote: > Using this as you had done is fine too, IMHO, but so is the new style. Yeah, it was a homogenisation. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/mediarecorder/MediaRecorder-requestData.html:49: "recorder throws InvalidStateError if requestData() comes in invalid state"); On 2016/02/04 03:03:10, philipj_UTC7 wrote: > Maybe s/comes in invalid state/while state is not "recording"/, assuming that is > what is being tested. Done. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: // TODO(mcasas): This section would no be needed if the bit rates are signed, see https://github.com/w3c/mediacapture-record/issues/48. On 2016/02/04 03:03:10, philipj_UTC7 wrote: > I like https://github.com/w3c/mediacapture-record/issues/48 want to fix that? > Integer overflow and stuff is so un-fun. 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? https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: ASSERT(options.bitsPerSecond() < kMaxIntAsUnsigned); On 2016/02/04 03:03:10, philipj_UTC7 wrote: > 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? Done. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:62: ASSERT(overallBps != 0 || options.videoBitsPerSecond() < kMaxIntAsUnsigned); On 2016/02/04 03:03:10, philipj_UTC7 wrote: > These asserts would have to go too. Done, I think. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:95: // Allocate the remaining overall |overallBps|, if any, to video. On 2016/02/04 03:03:10, philipj_UTC7 wrote: > Can drop "overall" now. Done. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { On 2016/02/04 03:03:10, philipj_UTC7 wrote: > Look at videoBps here instead? It'd be nice to deal with the options dict at the > top only. 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|.
Patchset #4 (id:80001) has been deleted
LGTM % asserts and, if you like, dictionary member default values https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:56: // TODO(mcasas): This section would no be needed if the bit rates are signed, see https://github.com/w3c/mediacapture-record/issues/48. On 2016/02/05 02:39:14, mcasas wrote: > On 2016/02/04 03:03:10, philipj_UTC7 wrote: > > I like https://github.com/w3c/mediacapture-record/issues/48 want to fix that? > > Integer overflow and stuff is so un-fun. > > 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? Sounds good. https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { On 2016/02/05 02:39:14, mcasas wrote: > On 2016/02/04 03:03:10, philipj_UTC7 wrote: > > Look at videoBps here instead? It'd be nice to deal with the options dict at > the > > top only. > > 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|. 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? https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: ASSERT(options.bitsPerSecond() >= 0); This ought to result in a tautological compare warning, but since the bots don't complain I assume that warning is disabled. This chunk of code is assuming that options.*PerSecond() are unsigned and not intended to be more generic, so I would drop the always-true asserts. https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:63: ASSERT(overallBps != 0 || options.videoBitsPerSecond() >= 0); If the trailing "||true" is dropped, note that this assert is also trivial to fail, just call new MediaRecorder({ bitsPerSecond: 0})?
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { On 2016/02/05 07:48:49, philipj_UTC7 wrote: > On 2016/02/05 02:39:14, mcasas wrote: > > On 2016/02/04 03:03:10, philipj_UTC7 wrote: > > > Look at videoBps here instead? It'd be nice to deal with the options dict at > > the > > > top only. > > > > 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|. > > 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? Using hasVideoBitsPerSecond(). https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: ASSERT(options.bitsPerSecond() >= 0); On 2016/02/05 07:48:49, philipj_UTC7 wrote: > This ought to result in a tautological compare warning, but since the bots don't > complain I assume that warning is disabled. This chunk of code is assuming that > options.*PerSecond() are unsigned and not intended to be more generic, so I > would drop the always-true asserts. Done. https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:63: ASSERT(overallBps != 0 || options.videoBitsPerSecond() >= 0); On 2016/02/05 07:48:49, philipj_UTC7 wrote: > If the trailing "||true" is dropped, note that this assert is also trivial to > fail, just call new MediaRecorder({ bitsPerSecond: 0})? These are basically ASSERT(if a then b) implemented with basic operators. In this case it means: if (overallBps == 0) then (options.videoBitsPerSecond() must be X) where X is: "non negative" or "smaller than the max". I removed the first ASSERT, but cannot use |options.hasBitsPerSecond()| since that would not return true when the user has explicitly configured them to be 0. Added a comment to clarify what I'm doing.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, avayvod@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1652373004/#ps140001 (title: "philipj@ nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1652373004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1652373004/140001
Message was sent while issue was closed.
Description was changed from
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
==========
to
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
==========
to
==========
MediaRecorder: add and wire read {video,audio}BitsPerSecond props
This CL follows [1] and adds these two fellas:
readonly attribute unsigned long videoBitsPerSecond;
readonly attribute unsigned long audioBitsPerSecond;
to the MR idl, and wires them to reflect the configured
or calculated bit rates. Added Layout Tests to double
check the current allocation scheme.
[1] https://github.com/w3c/mediacapture-record/issues/43
BUG=575301, 548290
Committed: https://crrev.com/e608797d019863e635cfdcac30d44f6304f81904
Cr-Commit-Position: refs/heads/master@{#373989}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e608797d019863e635cfdcac30d44f6304f81904 Cr-Commit-Position: refs/heads/master@{#373989}
Message was sent while issue was closed.
I have filed https://code.google.com/p/chromium/issues/detail?id=585059 for the failing asserts. https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:59: ASSERT(options.bitsPerSecond() >= 0); On 2016/02/06 00:26:25, mcasas wrote: > On 2016/02/05 07:48:49, philipj_UTC7 wrote: > > This ought to result in a tautological compare warning, but since the bots > don't > > complain I assume that warning is disabled. This chunk of code is assuming > that > > options.*PerSecond() are unsigned and not intended to be more generic, so I > > would drop the always-true asserts. > > Done. You didn't remove it :) https://codereview.chromium.org/1652373004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:64: ASSERT(!options.hasBitsPerSecond() || options.videoBitsPerSecond() < kMaxIntAsUnsigned); I'm afraid this assert fails if you try `{ videoBitsPerSecond : -1 }` or `{ videoBitsPerSecond : 3000000000 }` in MediaRecorder-bitrates.html.
Message was sent while issue was closed.
https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { On 2016/02/06 00:26:25, mcasas wrote: > On 2016/02/05 07:48:49, philipj_UTC7 wrote: > > On 2016/02/05 02:39:14, mcasas wrote: > > > On 2016/02/04 03:03:10, philipj_UTC7 wrote: > > > > Look at videoBps here instead? It'd be nice to deal with the options dict > at > > > the > > > > top only. > > > > > > 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|. > > > > 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? > > Using hasVideoBitsPerSecond(). 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.
Message was sent while issue was closed.
Oops, let's follow this dangling items in https://codereview.chromium.org/1682483002/ https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1652373004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.cpp:99: if ((options.videoBitsPerSecond() > 0 || overallBps > 0) && (videoBps < kSmallestPossibleVpxBitRate)) { On 2016/02/08 09:39:44, philipj_UTC7 wrote: > On 2016/02/06 00:26:25, mcasas wrote: > > On 2016/02/05 07:48:49, philipj_UTC7 wrote: > > > On 2016/02/05 02:39:14, mcasas wrote: > > > > On 2016/02/04 03:03:10, philipj_UTC7 wrote: > > > > > Look at videoBps here instead? It'd be nice to deal with the options > dict > > at > > > > the > > > > > top only. > > > > > > > > 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|. > > > > > > 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? > > > > Using hasVideoBitsPerSecond(). > > 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. 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). |
