Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414793002:
Update WebmMuxer for audio component of MediaStream recording. (Closed)
DescriptionUpdate WebmMuxer for audio component of MediaStream recording.
This is the second of ~three CLs for adding the audio component of
MediaStreamRecording. These CLs will be:
1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler
2) Update WebmMuxer to mux Opus output of AudioTrackRecorder
3) Update MediaRecorderHandler to use AudioTrackRecorder
Also refactors some opus header-related constants into a common location.
BUG=528519
Committed: https://crrev.com/528e1355c6c1d16b966d453fbf9df45ecfe144cf
Cr-Commit-Position: refs/heads/master@{#358489}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : address comments #
Total comments: 11
Patch Set 3 : address comments #
Total comments: 4
Patch Set 4 : address miu's comments #
Total comments: 14
Patch Set 5 : address comments #
Total comments: 10
Patch Set 6 : address comments #
Total comments: 4
Patch Set 7 : mcasas@ comments #Patch Set 8 : format #
Messages
Total messages: 41 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
ajose@chromium.org changed reviewers: + mcasas@chromium.org
Think this is ready for an initial review.
Doesn't look too bad, I've got a few comments to get going. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:17: // Comments and constants from media/filters/opus_audio_decoder.cc Please refactor the common constants out of both opus_audio_decoder.cc and this file, in an new file (e.g. media/filters/opus_extradata.h or similar). It's OK to include it from here, but might need .DEPS adaptation -- presubmit will tell. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:83: DCHECK_LE(params.channels(), 8); Extract |8| here and in l.49 to a constant? https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:127: Consider adding DCHECK(has_video_ || has_audio_); https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:159: if (first_frame_timestamp_ == base::TimeTicks()) I'd say if (first_frame_timestamp_.is_null()) https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:207: DCHECK_EQ(video_track_index_, 0u) nit: expected value goes first, DCHECK_EQ(0u, video_track_index_) https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:249: audio_track->set_seek_pre_roll(80000000); Nit: let's define a constant for the magic number, what about: // Matroska recommended SeekPreRoll value "number of nanoseconds that must be // discarded, for that stream, after a seek until the decoded data is valid to // render." http://wiki.xiph.org/MatroskaOpus const uint32_t kMatroskaSeekPreRollNs = 80000000; ? https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:252: DCHECK_EQ((int)audio_track->channels(), params.channels()); No C-style casts. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:259: } nit: no need for {} https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.h:70: // Creates and adds a new audio track. bla bla bla :) https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:21: using ::testing::Values; s/Values/ValuesIn/ and adapt l.189 https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:184: {kCodecVP8, 1, 0}, Nit: consider something like {kCodecVP8, 1 /* num_video_tracks */, 0 /*num_audio_tracks*/ } (perhaps just the first entry). https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:186: {kCodecVP8, 0, 1}, I guess in this case you can s/kCodecVp8/kUnknownVideoCodec/ ? Or maybe it'd hit the DCHECK on WebMuxer()...
Thanks for the comments :) https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:17: // Comments and constants from media/filters/opus_audio_decoder.cc On 2015/10/28 00:42:02, mcasas wrote: > Please refactor the common constants out of both opus_audio_decoder.cc and > this file, in an new file (e.g. media/filters/opus_extradata.h or similar). It's > OK to > include it from here, but might need .DEPS adaptation -- presubmit will tell. Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:83: DCHECK_LE(params.channels(), 8); On 2015/10/28 00:42:02, mcasas wrote: > Extract |8| here and in l.49 to a constant? Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:127: On 2015/10/28 00:42:02, mcasas wrote: > Consider adding DCHECK(has_video_ || has_audio_); Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:159: if (first_frame_timestamp_ == base::TimeTicks()) On 2015/10/28 00:42:02, mcasas wrote: > I'd say > if (first_frame_timestamp_.is_null()) Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:207: DCHECK_EQ(video_track_index_, 0u) On 2015/10/28 00:42:02, mcasas wrote: > nit: expected value goes first, DCHECK_EQ(0u, video_track_index_) Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:249: audio_track->set_seek_pre_roll(80000000); On 2015/10/28 00:42:02, mcasas wrote: > Nit: let's define a constant for the magic number, what about: > > // Matroska recommended SeekPreRoll value "number of nanoseconds that must be > // discarded, for that stream, after a seek until the decoded data is valid to > // render." http://wiki.xiph.org/MatroskaOpus > const uint32_t kMatroskaSeekPreRollNs = 80000000; > > ? Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:252: DCHECK_EQ((int)audio_track->channels(), params.channels()); On 2015/10/28 00:42:02, mcasas wrote: > No C-style casts. Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.cc:259: } On 2015/10/28 00:42:02, mcasas wrote: > nit: no need for {} Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxer.h File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer.h:70: // Creates and adds a new audio track. bla bla bla On 2015/10/28 00:42:02, mcasas wrote: > :) oy vey https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:21: using ::testing::Values; On 2015/10/28 00:42:02, mcasas wrote: > s/Values/ValuesIn/ > and adapt l.189 Need to change anything on 189 still? https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:184: {kCodecVP8, 1, 0}, On 2015/10/28 00:42:02, mcasas wrote: > Nit: consider something like > {kCodecVP8, 1 /* num_video_tracks */, 0 /*num_audio_tracks*/ } > (perhaps just the first entry). Done. https://codereview.chromium.org/1414793002/diff/80001/media/capture/webm_muxe... media/capture/webm_muxer_unittest.cc:186: {kCodecVP8, 0, 1}, On 2015/10/28 00:42:02, mcasas wrote: > I guess in this case you can s/kCodecVp8/kUnknownVideoCodec/ ? > Or maybe it'd hit the DCHECK on WebMuxer()... Hits the dcheck :'(
Description was changed from ========== Update WebmMuxer for audio component of MediaStream recording. This is the second of ~three CLs for adding the audio component of MediaStreamRecording. These CLs will be: 1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler 2) Update WebmMuxer to mux Opus output of AudioTrackRecorder 3) Update MediaRecorderHandler to use AudioTrackRecorder BUG=528519 ========== to ========== Update WebmMuxer for audio component of MediaStream recording. This is the second of ~three CLs for adding the audio component of MediaStreamRecording. These CLs will be: 1) Add AudioTrackRecorder, a MediaStreamAudioSink which will be owned by MediaRecorderHandler 2) Update WebmMuxer to mux Opus output of AudioTrackRecorder 3) Update MediaRecorderHandler to use AudioTrackRecorder Also refactors some opus header-related constants into a common location. BUG=528519 ==========
https://codereview.chromium.org/1414793002/diff/100001/content/renderer/media... File content/renderer/media/media_recorder_handler.cc (right): https://codereview.chromium.org/1414793002/diff/100001/content/renderer/media... content/renderer/media/media_recorder_handler.cc:88: 0 /* no audio for now */, base::Bind(&MediaRecorderHandler::WriteData, Please quote the bug that tracks the implementation, i.e. http://crbug.com/528519. https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux... media/capture/webm_muxer.cc:21: const uint32_t kMatroskaSeekPreRollNs = 80000000; The webm stream we are generating is supposed to be live, i.e., not seekable, so I'm not sure we need this setting. https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux... File media/capture/webm_muxer_unittest.cc (right): https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux... media/capture/webm_muxer_unittest.cc:189: INSTANTIATE_TEST_CASE_P(, WebmMuxerTest, ::testing::ValuesIn(kTestCases)); Yes, here you can s/::testing::ValuesIn/ValuesIn/ after |using ::testing::ValuesIn| in l.21. https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea... File media/filters/opus_header_constants.h (right): https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea... media/filters/opus_header_constants.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. No (c) [1] [1] https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea... media/filters/opus_header_constants.h:41: static const int kMaxChannelsWithDefaultLayout = 2; Creating all this static variables in media might impact Chrome startup time, so it might not fly, in that case you'll need to create a lazy class [1,2] for your static variables. [1] http://www.chromium.org/developers/coding-style/important-abstractions-and-da... [2] https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance... https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea... media/filters/opus_header_constants.h:103: const uint8 kFFmpegChannelDecodingLayouts [OPUS_MAX_VORBIS_CHANNELS] static? s/uint8/uint8_t/
ajose@chromium.org changed reviewers: + miu@chromium.org, tomfinegan@chromium.org
Thanks for the comments! Will wait on switching to a lazy class for now.
tomfinegan@: PTAL at media/capture/webm_muxer.{cc, h}, and let me know if you're
OK with changes to media/filter/opus_audio_decoder.cc
miu@: What do you think about using a lazy class in
media/filters/opus_header_constants.h?
https://codereview.chromium.org/1414793002/diff/100001/content/renderer/media...
File content/renderer/media/media_recorder_handler.cc (right):
https://codereview.chromium.org/1414793002/diff/100001/content/renderer/media...
content/renderer/media/media_recorder_handler.cc:88: 0 /* no audio for now */,
base::Bind(&MediaRecorderHandler::WriteData,
On 2015/11/02 18:29:54, mcasas wrote:
> Please quote the bug that tracks the implementation,
> i.e. http://crbug.com/528519.
Done.
https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux...
File media/capture/webm_muxer.cc (right):
https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux...
media/capture/webm_muxer.cc:21: const uint32_t kMatroskaSeekPreRollNs =
80000000;
On 2015/11/02 18:29:54, mcasas wrote:
> The webm stream we are generating is supposed to
> be live, i.e., not seekable, so I'm not sure we need
> this setting.
Removed for now
https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux...
File media/capture/webm_muxer_unittest.cc (right):
https://codereview.chromium.org/1414793002/diff/100001/media/capture/webm_mux...
media/capture/webm_muxer_unittest.cc:189: INSTANTIATE_TEST_CASE_P(,
WebmMuxerTest, ::testing::ValuesIn(kTestCases));
On 2015/11/02 18:29:54, mcasas wrote:
> Yes, here you can s/::testing::ValuesIn/ValuesIn/ after
> |using ::testing::ValuesIn| in l.21.
Done.
https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea...
File media/filters/opus_header_constants.h (right):
https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea...
media/filters/opus_header_constants.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/11/02 18:29:54, mcasas wrote:
> No (c) [1]
>
> [1] https://www.chromium.org/developers/coding-style#TOC-File-headers
Done.
https://codereview.chromium.org/1414793002/diff/100001/media/filters/opus_hea...
media/filters/opus_header_constants.h:103: const uint8
kFFmpegChannelDecodingLayouts [OPUS_MAX_VORBIS_CHANNELS]
On 2015/11/02 18:29:54, mcasas wrote:
> static?
> s/uint8/uint8_t/
Done.
https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... File media/filters/opus_header_constants.h (right): https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... media/filters/opus_header_constants.h:41: static const int kMaxChannelsWithDefaultLayout = 2; Per discussion on chromium-dev@ (about a month ago), make this an enum instead of a static const int, to allow the compiler to not have to allocate storage for this constant. https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... media/filters/opus_header_constants.h:103: static const uint8_t kFFmpegChannelDecodingLayouts [OPUS_MAX_VORBIS_CHANNELS] The lazy class idea is not needed. There should not be any static-initializer code generated for these arrays. These arrays need their declaration separated from their definition. In the opus_constants.h file (please delete "header" from the name): extern const uint8 kDefaultOpusChannelLayout[kMaxChannelsWithDefaultLayout]; extern const uint8_t kFFmpegChannelDecodingLayouts[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS]; extern const uint8_t kOpusVorbisChannelMap[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS]; In an opus_constants.cc file: namespace media { const uint8 kDefaultOpusChannelLayout[kMaxChannelsWithDefaultLayout] = { 0, 1}; const uint8_t kFFmpegChannelDecodingLayouts[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS] = { ...multiline initializer here... }; const uint8_t kOpusVorbisChannelMap[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS] = { ...multiline initializer here... }; } // namespace media
Great, thanks for the help miu@. https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... File media/filters/opus_header_constants.h (right): https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... media/filters/opus_header_constants.h:41: static const int kMaxChannelsWithDefaultLayout = 2; On 2015/11/03 04:18:29, miu wrote: > Per discussion on chromium-dev@ (about a month ago), make this an enum instead > of a static const int, to allow the compiler to not have to allocate storage for > this constant. Done. https://codereview.chromium.org/1414793002/diff/120001/media/filters/opus_hea... media/filters/opus_header_constants.h:103: static const uint8_t kFFmpegChannelDecodingLayouts [OPUS_MAX_VORBIS_CHANNELS] On 2015/11/03 04:18:28, miu wrote: > The lazy class idea is not needed. There should not be any static-initializer > code generated for these arrays. > > These arrays need their declaration separated from their definition. In the > opus_constants.h file (please delete "header" from the name): > > extern const uint8 kDefaultOpusChannelLayout[kMaxChannelsWithDefaultLayout]; > > extern const uint8_t > kFFmpegChannelDecodingLayouts[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS]; > > extern const uint8_t > kOpusVorbisChannelMap[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS]; > > In an opus_constants.cc file: > > namespace media { > > const uint8 kDefaultOpusChannelLayout[kMaxChannelsWithDefaultLayout] = { 0, > 1}; > > const uint8_t > kFFmpegChannelDecodingLayouts[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS] > = { ...multiline initializer here... }; > > const uint8_t > kOpusVorbisChannelMap[OPUS_MAX_VORBIS_CHANNELS][OPUS_MAX_VORBIS_CHANNELS] = { > ...multiline initializer here... }; > > } // namespace media Done.
lgtm % nits https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.cc:72: size_t num_video_tracks, You take num_XXXXX_tracks as arguments, but this implementation only supports zero or one of each. If you intend to later support multiple tracks, you should replace has_video_ and has_audio_ with num_video_tracks_ and num_audio_tracks_. If you don't intend to support mulitple tracks, these args could just be bools. https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:95: bool has_video_; const bool has_video_; and const bool has_audio_; https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... File media/filters/opus_constants.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:43: nit: Please remove extra newline. https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:99: nit: Please remove extra newline here too.
Looks fine. One nit and a comment re using size_t for track number (just that the max track num in libwebm is 126). No need for a change on the declaration/implementation. Just wanted to make sure you're aware. https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:47: size_t num_audio_tracks, Note that max track number in libwebm is 126, see https://chromium.googlesource.com/webm/libwebm/+/master/mkvmuxer.hpp Add{Audio|Video}Track will fail if track number exceeds 126. https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... File media/filters/opus_constants.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:44: nit: one line of whitespace is probably good here
https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:47: size_t num_audio_tracks, On 2015/11/04 00:28:55, Tom Finegan wrote: > Note that max track number in libwebm is 126, see > https://chromium.googlesource.com/webm/libwebm/+/master/mkvmuxer.hpp > > Add{Audio|Video}Track will fail if track number exceeds 126. And a DCHECK inside constructor + link to file + explanation pretty please?
Thanks for the comments, and for the heads up about track limits. https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.cc:72: size_t num_video_tracks, On 2015/11/04 00:18:27, miu wrote: > You take num_XXXXX_tracks as arguments, but this implementation only supports > zero or one of each. If you intend to later support multiple tracks, you should > replace has_video_ and has_audio_ with num_video_tracks_ and num_audio_tracks_. > If you don't intend to support mulitple tracks, these args could just be bools. Switched to num_{audio|video}_tracks_. Full support for multiple tracks is probably outside the scope of this CL, but coming soon. It's currently tracked by [1]. [1] https://code.google.com/p/chromium/issues/detail?id=528523 https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:47: size_t num_audio_tracks, On 2015/11/04 00:41:40, mcasas wrote: > On 2015/11/04 00:28:55, Tom Finegan wrote: > > Note that max track number in libwebm is 126, see > > https://chromium.googlesource.com/webm/libwebm/+/master/mkvmuxer.hpp > > > > Add{Audio|Video}Track will fail if track number exceeds 126. > > And a DCHECK inside constructor + link to file + explanation > pretty please? Done. https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:47: size_t num_audio_tracks, On 2015/11/04 00:28:55, Tom Finegan wrote: > Note that max track number in libwebm is 126, see > https://chromium.googlesource.com/webm/libwebm/+/master/mkvmuxer.hpp > > Add{Audio|Video}Track will fail if track number exceeds 126. Done. https://codereview.chromium.org/1414793002/diff/140001/media/capture/webm_mux... media/capture/webm_muxer.h:95: bool has_video_; On 2015/11/04 00:18:27, miu wrote: > const bool has_video_; > > and > > const bool has_audio_; Done. https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... File media/filters/opus_constants.h (right): https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:43: On 2015/11/04 00:18:28, miu wrote: > nit: Please remove extra newline. Done. https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:44: On 2015/11/04 00:28:55, Tom Finegan wrote: > nit: one line of whitespace is probably good here Done. https://codereview.chromium.org/1414793002/diff/140001/media/filters/opus_con... media/filters/opus_constants.h:99: On 2015/11/04 00:18:28, miu wrote: > nit: Please remove extra newline here too. Done.
Looking good, few questions. https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.cc:91: DCHECK_LT(num_audio_tracks, LIBWEBM_MAX_TRACK_NUMBER); Use |mkvmuxer::kMaxTrackNumber| instead, and remove the definition of LIBWEBM_MAX_TRACK_NUMBER (although you can bring the comment here). https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.cc:183: video_track_index_ = Hmm I'm not sure everything fits: - WebmMuxer is told on ctor how many video/audio tracks to hold - then it has a |video_track_index_| initialised to 0 - on first video frame arrival, |video_track_index_ == 0| and this method AddVideoTrack() is called, which initialises said |video_track_index_|. - on second video frame arrival _of any video track whatsoever_, |video_track_index_ != 0| and we'll add the frame to |segment_| with the said |video_track_index_|. All incoming video streams would end up labeled with the same track index, is that the desired behaviour? Same for Audio. https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.h:30: // Clients will push encoded VPx video frames one by one via OnEncodedVideo(). Update these lines plz. https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... File media/filters/opus_constants.cc (right): https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... media/filters/opus_constants.cc:5: #include <stdint.h> Needed? https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... File media/filters/opus_constants.h (right): https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... media/filters/opus_constants.h:24: // - N = totel number of streams (8 bits) s/totel/total/
Thanks for the comments! https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... File media/capture/webm_muxer.cc (right): https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.cc:91: DCHECK_LT(num_audio_tracks, LIBWEBM_MAX_TRACK_NUMBER); On 2015/11/05 02:08:20, mcasas wrote: > Use |mkvmuxer::kMaxTrackNumber| instead, > and remove the definition of LIBWEBM_MAX_TRACK_NUMBER > (although you can bring the comment here). Much better. https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.cc:183: video_track_index_ = On 2015/11/05 02:08:20, mcasas wrote: > Hmm I'm not sure everything fits: > - WebmMuxer is told on ctor how many video/audio tracks to hold > - then it has a |video_track_index_| initialised to 0 > - on first video frame arrival, |video_track_index_ == 0| and > this method AddVideoTrack() is called, which initialises > said |video_track_index_|. > - on second video frame arrival _of any video track whatsoever_, > |video_track_index_ != 0| and we'll add the frame to |segment_| > with the said |video_track_index_|. > > All incoming video streams would end up labeled with > the same track index, is that the desired behaviour? Same > for Audio. It's not the desired behavior, AFAIK. But currently WebmMuxer is only meant to support a single video and single audio track, right? Hence [1]. Also added a DCHECK to verify we only have at most one of each for now. [1] https://code.google.com/p/chromium/issues/detail?id=528523 https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/160001/media/capture/webm_mux... media/capture/webm_muxer.h:30: // Clients will push encoded VPx video frames one by one via OnEncodedVideo(). On 2015/11/05 02:08:20, mcasas wrote: > Update these lines plz. Done. https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... File media/filters/opus_constants.cc (right): https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... media/filters/opus_constants.cc:5: #include <stdint.h> On 2015/11/05 02:08:20, mcasas wrote: > Needed? Doesn't know about uint8_t without this https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... File media/filters/opus_constants.h (right): https://codereview.chromium.org/1414793002/diff/160001/media/filters/opus_con... media/filters/opus_constants.h:24: // - N = totel number of streams (8 bits) On 2015/11/05 02:08:20, mcasas wrote: > s/totel/total/ Done.
Patchset #6 (id:180001) has been deleted
Remove the |num_{audio_video}_tracks_| and
you're good to go. With that premise, LGTM.
https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux...
File media/capture/webm_muxer.h (right):
https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux...
media/capture/webm_muxer.h:33: // passed on contructor with the
Reflow l.33-34
https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux...
media/capture/webm_muxer.h:98: const uint8_t num_audio_tracks_;
Since for this CL we don't support
|num_video_tracks_| > 1 or |num_audio_tracks_| > 1,
I'd say
a) remove the members and associated checks
b) leave a TODO in the class if you want (with bug of course)
c) remove the associate constructor params.
IOW remove all the multi-track parts from this CL if
they are not used on ToT :)
On a parallel discussion, I see the utility of supporting
several audio tracks (e.g. stereo or 5:1 audio) but it
might not be too urgent to support several video tracks,
as a matter of fact I've never seen a MediaStream with
more than a video tracks. Even more, have you ever
seen a .webm file with >1 video track? How would we
play it back...?
ajose@chromium.org changed reviewers: + dalecurtis@chromium.org
Thanks for the comments, removed multi-track stuff. dalecurtis@: RS lgtm? https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... File media/capture/webm_muxer.h (right): https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... media/capture/webm_muxer.h:33: // passed on contructor with the On 2015/11/06 02:22:37, mcasas wrote: > Reflow l.33-34 Done. https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... media/capture/webm_muxer.h:98: const uint8_t num_audio_tracks_; On 2015/11/06 02:22:37, mcasas wrote: > Since for this CL we don't support > |num_video_tracks_| > 1 or |num_audio_tracks_| > 1, > I'd say > a) remove the members and associated checks > b) leave a TODO in the class if you want (with bug of course) > c) remove the associate constructor params. > > IOW remove all the multi-track parts from this CL if > they are not used on ToT :) > > On a parallel discussion, I see the utility of supporting > several audio tracks (e.g. stereo or 5:1 audio) but it > might not be too urgent to support several video tracks, > as a matter of fact I've never seen a MediaStream with > more than a video tracks. Even more, have you ever > seen a .webm file with >1 video track? How would we > play it back...? Done.
On 2015/11/06 18:40:52, ajose wrote: > Thanks for the comments, removed multi-track stuff. > > dalecurtis@: RS lgtm? > > https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... > File media/capture/webm_muxer.h (right): > > https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... > media/capture/webm_muxer.h:33: // passed on contructor with the > On 2015/11/06 02:22:37, mcasas wrote: > > Reflow l.33-34 > > Done. > > https://codereview.chromium.org/1414793002/diff/200001/media/capture/webm_mux... > media/capture/webm_muxer.h:98: const uint8_t num_audio_tracks_; > On 2015/11/06 02:22:37, mcasas wrote: > > Since for this CL we don't support > > |num_video_tracks_| > 1 or |num_audio_tracks_| > 1, > > I'd say > > a) remove the members and associated checks > > b) leave a TODO in the class if you want (with bug of course) > > c) remove the associate constructor params. l.46 and l.47 of webm_muxer.h are still there. > > > > IOW remove all the multi-track parts from this CL if > > they are not used on ToT :) > > > > On a parallel discussion, I see the utility of supporting > > several audio tracks (e.g. stereo or 5:1 audio) but it > > might not be too urgent to support several video tracks, > > as a matter of fact I've never seen a MediaStream with > > more than a video tracks. Even more, have you ever > > seen a .webm file with >1 video track? How would we > > play it back...? > > Done.
To avoid issues with writing the WebM header, we need some way of knowing what tracks to expect, so leaving in these params as bools for now.
On 2015/11/06 19:16:48, ajose wrote: > To avoid issues with writing the WebM header, we need some way of knowing what > tracks to expect, so leaving in these params as bools for now. still LGTM
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
lgtm
The CQ bit was checked by ajose@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/1414793002/#ps280001 (title: "mcasas@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414793002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414793002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #7 (id:260001) has been deleted
The CQ bit was checked by ajose@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org, dalecurtis@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/1414793002/#ps300001 (title: "format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414793002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414793002/300001
Message was sent while issue was closed.
Committed patchset #8 (id:300001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/528e1355c6c1d16b966d453fbf9df45ecfe144cf Cr-Commit-Position: refs/heads/master@{#358489} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
