Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11416367:
Add Opus decode wrapper to media. (Closed)
|
Created:
8 years ago by Tom Finegan Modified:
8 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd wrapper class to media for support of Opus audio, and add a command line flag to enable the support.
This initial version of the wrapper provides support for decoding Opus audio in WebM container files, and is disabled by default.
New flag added: --enable-opus-playback
BUG=166094
TEST=Opus audio in WebM containers plays back in <video> elements when --enable-opus-playback is specified on the command line.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173663
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments. #Patch Set 3 : Move opus support behind a flag, and add canPlayType support. #
Total comments: 50
Patch Set 4 : Rebased to stay synced with 11468018 (libvpx wrapper CL). Sorry for the noise! #
Total comments: 2
Patch Set 5 : Address comments. #
Total comments: 15
Patch Set 6 : Address comments. #
Total comments: 19
Patch Set 7 : Rebased (changed machines) Address most comments. Added TODO for Opus buffering question. #
Total comments: 33
Patch Set 8 : Address most comments and remove media source stuff. #
Total comments: 4
Patch Set 9 : Revert mime_util changes. #
Total comments: 13
Patch Set 10 : Address remaining nits from media reviewers. #
Total comments: 2
Patch Set 11 : Address Dale's comment. #Patch Set 12 : Make Opus sample size handling more compatible with pending ffmpeg_common changes. #
Total comments: 2
Patch Set 13 : Address comment. #Patch Set 14 : Rebased. #Patch Set 15 : Final pre-CQ rebase. #Patch Set 16 : Rebase on updates to webkit/media/filter_helpers.cc #
Messages
Total messages: 36 (0 generated)
First pass/works on my machine. Note: There's a hack in ffmpeg_common to get around (I think) lack of opus support in libavcodec.
+dalecurtis as he is more familiar with audio decoders than me. first round of comments. I haven't reviewed the real OpusAudioDecoder implementation yet. https://codereview.chromium.org/11416367/diff/1/media/base/audio_decoder_conf... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/1/media/base/audio_decoder_conf... media/base/audio_decoder_config.h:33: kCodecOpus, This is ChromeOS specific? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:26: return base::ByteSwapToLE16(static_cast<uint16>(*(data + read_offset))); Is this correct? *(data + read_offset) is still a uint8 value. We cast it to uint16 then do a byteswap? Shouldn't we read two bytes out? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:113: 0, 1, 0, 0, 0, 0, 0, 0 }; Could you please add a comment explain what this is? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:121: num_coupled(0) { *stream_map = *kDefaultOpusStreamMap; } Do you mean memcpy(stream_map, .... ? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:145: const int kChannelsOffset = 9; hmm, can we have these constants in one place, preferably close to line 90? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:155: if (data_size >= kOpusHeaderSize) { shouldn't this been covered by line 142 already? https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:163: const int kMappingRequiredSize = kOpusHeaderSize + 2 + header->channels; what's this magic number 2? Since this contains header->channels, it doesn't look like a real constant... https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:191: } What do you think of reordering the logic of line 164-191 like this? if (header->channel_mapping) { if (data_size < mapping_required_size) { LOG(ERROR)... return false; } normal case here }
Addressed the first round of comments, and: - added command line flag - added canPlayType support The flag is for vp9 and opus. It's defined in content: Should I move it to media_switches? https://codereview.chromium.org/11416367/diff/1/media/base/audio_decoder_conf... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/1/media/base/audio_decoder_conf... media/base/audio_decoder_config.h:33: kCodecOpus, On 2012/12/12 22:16:10, xhwang wrote: > This is ChromeOS specific? Oops, missed that the section I dropped this in was CrOS only. Thanks! https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:26: return base::ByteSwapToLE16(static_cast<uint16>(*(data + read_offset))); On 2012/12/12 22:16:10, xhwang wrote: > Is this correct? *(data + read_offset) is still a uint8 value. We cast it to > uint16 then do a byteswap? Shouldn't we read two bytes out? Yeah, fixed. Deref'ing a uint8* and casting the value to uint16 doesn't magically read 2 bytes. Oops. :) https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:113: 0, 1, 0, 0, 0, 0, 0, 0 }; On 2012/12/12 22:16:10, xhwang wrote: > Could you please add a comment explain what this is? Done. https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:121: num_coupled(0) { *stream_map = *kDefaultOpusStreamMap; } On 2012/12/12 22:16:10, xhwang wrote: > Do you mean memcpy(stream_map, .... ? Done. https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:145: const int kChannelsOffset = 9; On 2012/12/12 22:16:10, xhwang wrote: > hmm, can we have these constants in one place, preferably close to line 90? Done. https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:155: if (data_size >= kOpusHeaderSize) { On 2012/12/12 22:16:10, xhwang wrote: > shouldn't this been covered by line 142 already? Done. https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:163: const int kMappingRequiredSize = kOpusHeaderSize + 2 + header->channels; On 2012/12/12 22:16:10, xhwang wrote: > what's this magic number 2? Since this contains header->channels, it doesn't > look like a real constant... It's not a real constant... it changes based on channel count. Changed the name, and added a constant for bytes per channel (the magic 2). https://codereview.chromium.org/11416367/diff/1/media/filters/opus_audio_deco... media/filters/opus_audio_decoder.cc:191: } On 2012/12/12 22:16:10, xhwang wrote: > What do you think of reordering the logic of line 164-191 like this? > if (header->channel_mapping) { > if (data_size < mapping_required_size) { > LOG(ERROR)... > return false; > } > > normal case here > } Moved things around, but not exactly as requested above. PTAL/WDYT?
more comments https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags... chrome/browser/about_flags.cc:639: "enable-vp9-opus-playback", does it make sense to have two flags? https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... media/base/audio_decoder_config.h:24: kCodecOpus, I am not sure if this is okay given the comments in line 16-18 https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:27: *reinterpret_cast<const uint16*>((data + read_offset))); This is not safe: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_pa... A way to fix it is to use memcpy: https://codereview.chromium.org/11304010/ https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:53: static const int kBytesPerChannel = kRequiredSampleSize / 2; why 2? It's not obvious to me... https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:56: static const int kMaxOpusOutputPacketSizeSamples = 960 * 6; Could you add a link to the spec? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:117: 0, 1, 0, 0, 0, 0, 0, 0 }; what are these values? are they indices into kVorbisChannelLayouts? If so, add comments? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:138: memcpy(&stream_map[0], &kDefaultOpusChannelLayout[0], kMaxVorbisChannels); can this be memcpy(stream_map, kDefaultOpusChannelLayout, kMax....)? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:196: header->num_streams = *(data + kOpusHeaderSize); Can we actually have kOpusHeaderNumStreamsOffset, kOpusHeaderNumCoupledOffset and kOpusHeadStreamMapOffset? If so, we can also consolidate the comments here with comments on line 101. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:210: return true; nit: I like return early and avoid large if/else blocks. Also I'd like not to check one condition twice (e.g. if (header->channel_mapping). How about: if (!header->channel_mapping) { if (header->channels > 2) { LOG(ERROR) << "ParseOpusHeader(): Invalid header, missing stream map."; return false; } header->num_streams = 1; header->num_coupled = (ChannelLayoutToChannelCount(config.channel_layout()) > 1) ? 1 : 0; return true; } // Move the block in if (header->channel_mapping) here https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:229: message_loop_->PostTask(FROM_HERE, base::Bind( FYI, the 2013 fashion trend shows that we are removing this pattern: https://codereview.chromium.org/11428095/ The new pattern is that we make sure the caller calls on the right thread, decoders only do DCHECK here to make sure this method is actually called on the right thread. You don't need to make the changes right now as we are still making the changes. Just FYI. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:450: samples_per_second_ != config.samples_per_second())) { hmm, I wonder what config change we support? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:484: &kDefaultOpusChannelLayout[kMaxVorbisChannels - 1]); Range constructor of std::vector takes [first, last) so that "last" is not included. Is this what you expect? Also would this work? std::vector<uint8> channel_mapping( kDefaultOpusChannelLayout, kDefaultOpusChannelLayout + kMaxVorbisChannels); https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:498: int status = 0; looks like OPUS_OK is 0 (http://dxr.mozilla.org/mozilla-central/media/libopus/include/opus_defines.h.h...). can we initialize to something that's not OPUS_OK? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:503: &channel_mapping[0], hmm, if we always use &channel_mapping[0], why do we need a vector? Can we use a scoped_array? or just an uint8[kMaxVorbisChannels] on the stack? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:504: &status); do we want to check if status == OPUS_OK ? https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:544: int16* output_buffer = reinterpret_cast<int16*>(&output_buffer_[0]); This may also not be safe for type punning. It depends on how opus_multistream_decode uses |output_buffer|. Why not define output_buffer_ to be a int16 array in the first place? line 584 should be safe as we know it will only do a memcpy. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:577: decoded_audio_data += dropped_size; wondering if dropped_size is always a multiple of 2 (so that it's only dropping samples).... https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:597: queued_audio_.push_back(queue_entry); queued_audio_ is used in FFmepgAudioDecoder because one input could possibly result in multiple output DataBuffers. In this case, it seems one input can only generate one output. If that's true, we don't need a queued_audio_ and can just fire the read_cb after decoding. https://codereview.chromium.org/11416367/diff/7001/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): https://codereview.chromium.org/11416367/diff/7001/webkit/media/filter_helper... webkit/media/filter_helpers.cc:9: #include "content/public/common/content_switches.h" media code cannot depend on content code. You'll probably need to define the flag in media_switches.
https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags... chrome/browser/about_flags.cc:639: "enable-vp9-opus-playback", On 2012/12/13 08:33:13, xhwang wrote: > does it make sense to have two flags? I wasn't sure about that. But I didn't want to add more flags than needed. I think most clients will add support for both around the same time. https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... media/base/audio_decoder_config.h:24: kCodecOpus, On 2012/12/13 08:33:13, xhwang wrote: > I am not sure if this is okay given the comments in line 16-18 I think you are right. I think this needs to go after kCodecPCM_S24BE. Probably with a new comment above it. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:27: *reinterpret_cast<const uint16*>((data + read_offset))); On 2012/12/13 08:33:13, xhwang wrote: > This is not safe: > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_pa... > > A way to fix it is to use memcpy: https://codereview.chromium.org/11304010/ This should be fine as long as the size of the data is <= read_offset + sizeof(uint16) and Tom has a DCheck for this. I would not want to add extra memcpys. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:484: &kDefaultOpusChannelLayout[kMaxVorbisChannels - 1]); On 2012/12/13 08:33:13, xhwang wrote: > Range constructor of std::vector takes [first, last) so that "last" is not > included. Is this what you expect? > > Also would this work? > std::vector<uint8> channel_mapping( > kDefaultOpusChannelLayout, > kDefaultOpusChannelLayout + kMaxVorbisChannels); That should work. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:487: // Remap channels from Vorbis order to FFmpeg order (which I what I think which is what... https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:511: // TODO(tomfinegan): The OPUS_GET_LOOKAHEAD ctl fails with a not implemented opus dec just sets 80 milliseconds, which is what Opus recommends. They are still deciding on how to represent this in Matroska. For now add a TODO to handle the delay. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:544: int16* output_buffer = reinterpret_cast<int16*>(&output_buffer_[0]); On 2012/12/13 08:33:13, xhwang wrote: > This may also not be safe for type punning. It depends on how > opus_multistream_decode uses |output_buffer|. Why not define output_buffer_ to > be a int16 array in the first place? line 584 should be safe as we know it will > only do a memcpy. Again this should be safe. But if you can change output_buffer_ to short then that is up to you. https://codereview.chromium.org/11416367/diff/15001/media/filters/opus_audio_... File media/filters/opus_audio_decoder.h (right): https://codereview.chromium.org/11416367/diff/15001/media/filters/opus_audio_... media/filters/opus_audio_decoder.h:89: int delay_; You could remove this if we are not using it yet.
I think I got everything... apologies in advance if I missed a comment or two. PTAL, thanks! https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags... chrome/browser/about_flags.cc:639: "enable-vp9-opus-playback", On 2012/12/13 08:33:13, xhwang wrote: > does it make sense to have two flags? I don't think so-- I don't think there's any benefit to enabling the new WebM codecs individually. We can always use audio or video only files if one the wrappers need to be isolated for testing. https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/7001/media/base/audio_decoder_c... media/base/audio_decoder_config.h:24: kCodecOpus, On 2012/12/13 08:33:13, xhwang wrote: > I am not sure if this is okay given the comments in line 16-18 Heh. I should have actually read the comments in here before making changes. :) Thanks. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:27: *reinterpret_cast<const uint16*>((data + read_offset))); On 2012/12/13 08:33:13, xhwang wrote: > This is not safe: > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_pa... > > A way to fix it is to use memcpy: https://codereview.chromium.org/11304010/ Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:53: static const int kBytesPerChannel = kRequiredSampleSize / 2; On 2012/12/13 08:33:13, xhwang wrote: > why 2? It's not obvious to me... Sleep deprived... 2's the result I want, should have been an 8. Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:56: static const int kMaxOpusOutputPacketSizeSamples = 960 * 6; On 2012/12/13 08:33:13, xhwang wrote: > Could you add a link to the spec? Done, but it's up a little higher in the file. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:117: 0, 1, 0, 0, 0, 0, 0, 0 }; On 2012/12/13 08:33:13, xhwang wrote: > what are these values? are they indices into kVorbisChannelLayouts? If so, add > comments? The values are what the comment says: The default channel layout used to initialize stream_map. :) It's only valid for mono and stereo because streams with > 2 channels require a stream map. Added more to the comment. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:138: memcpy(&stream_map[0], &kDefaultOpusChannelLayout[0], kMaxVorbisChannels); On 2012/12/13 08:33:13, xhwang wrote: > can this be memcpy(stream_map, kDefaultOpusChannelLayout, kMax....)? Done. Was just being over explicit, I guess. :) https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:196: header->num_streams = *(data + kOpusHeaderSize); On 2012/12/13 08:33:13, xhwang wrote: > Can we actually have kOpusHeaderNumStreamsOffset, kOpusHeaderNumCoupledOffset > and kOpusHeadStreamMapOffset? > > If so, we can also consolidate the comments here with comments on line 101. Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:210: return true; On 2012/12/13 08:33:13, xhwang wrote: > nit: I like return early and avoid large if/else blocks. Also I'd like not to > check one condition twice (e.g. if (header->channel_mapping). How about: > > if (!header->channel_mapping) { > if (header->channels > 2) { > LOG(ERROR) << "ParseOpusHeader(): Invalid header, missing stream map."; > return false; > } > > header->num_streams = 1; > header->num_coupled = > (ChannelLayoutToChannelCount(config.channel_layout()) > 1) ? 1 : 0; > return true; > } > > // Move the block in if (header->channel_mapping) here Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:229: message_loop_->PostTask(FROM_HERE, base::Bind( On 2012/12/13 08:33:13, xhwang wrote: > FYI, the 2013 fashion trend shows that we are removing this pattern: > https://codereview.chromium.org/11428095/ > > The new pattern is that we make sure the caller calls on the right thread, > decoders only do DCHECK here to make sure this method is actually called on the > right thread. > > You don't need to make the changes right now as we are still making the changes. > Just FYI. Ok. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:450: samples_per_second_ != config.samples_per_second())) { On 2012/12/13 08:33:13, xhwang wrote: > hmm, I wonder what config change we support? I don't know-- this is copied from the FFmpeg decoder. When in Rome... :) https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:484: &kDefaultOpusChannelLayout[kMaxVorbisChannels - 1]); On 2012/12/13 08:33:13, xhwang wrote: > Range constructor of std::vector takes [first, last) so that "last" is not > included. Is this what you expect? > > Also would this work? > std::vector<uint8> channel_mapping( > kDefaultOpusChannelLayout, > kDefaultOpusChannelLayout + kMaxVorbisChannels); Removed vector. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:487: // Remap channels from Vorbis order to FFmpeg order (which I what I think On 2012/12/13 22:30:14, fgalligan1 wrote: > which is what... Removed comment here, and added one up in RemapOpusChannelLayout() before the reordering loop explaining that I'm matching FFmpeg to meet pipeline expectations. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:498: int status = 0; On 2012/12/13 08:33:13, xhwang wrote: > looks like OPUS_OK is 0 > (http://dxr.mozilla.org/mozilla-central/media/libopus/include/opus_defines.h.h...). > can we initialize to something that's not OPUS_OK? Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:503: &channel_mapping[0], On 2012/12/13 08:33:13, xhwang wrote: > hmm, if we always use &channel_mapping[0], why do we need a vector? Can we use a > scoped_array? or just an uint8[kMaxVorbisChannels] on the stack? Why do I try to over complicate things... Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:504: &status); On 2012/12/13 08:33:13, xhwang wrote: > do we want to check if status == OPUS_OK ? Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:511: // TODO(tomfinegan): The OPUS_GET_LOOKAHEAD ctl fails with a not implemented On 2012/12/13 22:30:14, fgalligan1 wrote: > opus dec just sets 80 milliseconds, which is what Opus recommends. They are > still deciding on how to represent this in Matroska. For now add a TODO to > handle the delay. Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:544: int16* output_buffer = reinterpret_cast<int16*>(&output_buffer_[0]); On 2012/12/13 08:33:13, xhwang wrote: > This may also not be safe for type punning. It depends on how > opus_multistream_decode uses |output_buffer|. Why not define output_buffer_ to > be a int16 array in the first place? line 584 should be safe as we know it will > only do a memcpy. Done. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:577: decoded_audio_data += dropped_size; On 2012/12/13 08:33:13, xhwang wrote: > wondering if dropped_size is always a multiple of 2 (so that it's only dropping > samples).... Added a DCHECK. https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:597: queued_audio_.push_back(queue_entry); On 2012/12/13 08:33:13, xhwang wrote: > queued_audio_ is used in FFmepgAudioDecoder because one input could possibly > result in multiple output DataBuffers. In this case, it seems one input can only > generate one output. If that's true, we don't need a queued_audio_ and can just > fire the read_cb after decoding. Ok. Wasn't sure if the pipeline consumed all output, or if the queue was always needed so that output_buffer_ could be reused while some audio hung around in the queue. Removed queue stuff. https://codereview.chromium.org/11416367/diff/7001/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): https://codereview.chromium.org/11416367/diff/7001/webkit/media/filter_helper... webkit/media/filter_helpers.cc:9: #include "content/public/common/content_switches.h" On 2012/12/13 08:33:13, xhwang wrote: > media code cannot depend on content code. You'll probably need to define the > flag in media_switches. Done. https://codereview.chromium.org/11416367/diff/15001/media/filters/opus_audio_... File media/filters/opus_audio_decoder.h (right): https://codereview.chromium.org/11416367/diff/15001/media/filters/opus_audio_... media/filters/opus_audio_decoder.h:89: int delay_; On 2012/12/13 22:30:14, fgalligan1 wrote: > You could remove this if we are not using it yet. Done.
Thanks for the update. A few more comments. https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/11416367/diff/7001/chrome/browser/about_flags... chrome/browser/about_flags.cc:639: "enable-vp9-opus-playback", On 2012/12/13 23:20:00, Tom Finegan wrote: > On 2012/12/13 08:33:13, xhwang wrote: > > does it make sense to have two flags? > > I don't think so-- I don't think there's any benefit to enabling the new WebM > codecs individually. We can always use audio or video only files if one the > wrappers need to be isolated for testing. sg https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:27: *reinterpret_cast<const uint16*>((data + read_offset))); On 2012/12/13 22:30:14, fgalligan1 wrote: > On 2012/12/13 08:33:13, xhwang wrote: > > This is not safe: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/base/basictypes.h&exact_pa... > > > > A way to fix it is to use memcpy: https://codereview.chromium.org/11304010/ > This should be fine as long as the size of the data is <= read_offset + > sizeof(uint16) and Tom has a DCheck for this. I would not want to add extra > memcpys. > Type punning is about "holding an object in memory of one type and reading its bits back using a different type", which is exactly what we are doing. I still think this needs to be fixed... https://codereview.chromium.org/11416367/diff/7001/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:450: samples_per_second_ != config.samples_per_second())) { On 2012/12/13 23:20:00, Tom Finegan wrote: > On 2012/12/13 08:33:13, xhwang wrote: > > hmm, I wonder what config change we support? > > I don't know-- this is copied from the FFmpeg decoder. When in Rome... :) ok, thanks for letting me know. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:51: static const int kBytesPerChannel = kRequiredSampleSize / 8; Thanks. I still don't fully get it. Is kRequiredSampleSize in bits? Is it the same as bits_per_channel? If so, can we make it more explicit? https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:119: 0, 1, 0, 0, 0, 0, 0, 0 }; hmm, I still don't get it. What does a 0 or 1 channel layout mean? If we only use the first two items in this list, why not just define a kDefaultOpusChannelLayout[kMaxChannelsWithDefaultChannelLayout], where the value of kMaxChannelsWithDefaultChannelLayout is 2? https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:188: if (header->channels > 2) { s/2/kMaxChannelsWithDefaultChannelLayout if you follow the above comment https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:307: // read request. This comment is not necessary. Most decoders (e.g. VideoDecoders) don't queue buffers. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:457: memcpy(&channel_mapping, &kDefaultOpusChannelLayout, kMaxVorbisChannels); why do you need "&"? https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:458: if (channel_count > 2) { ditto about 2 https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:566: base::ResetAndReturn(&read_cb_).Run(kOk, output); Can we keep the read_cb_ firing in one function. The success path is here but the fail path is in line 382, which is hard to track.
https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:51: static const int kBytesPerChannel = kRequiredSampleSize / 8; On 2012/12/14 01:19:21, xhwang wrote: > Thanks. I still don't fully get it. Is kRequiredSampleSize in bits? Is it the > same as bits_per_channel? If so, can we make it more explicit? kRequiredSampleSize looks to be bits_per_sample to me
PTAL, thanks! https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:51: static const int kBytesPerChannel = kRequiredSampleSize / 8; On 2012/12/14 01:19:21, xhwang wrote: > Thanks. I still don't fully get it. Is kRequiredSampleSize in bits? Is it the > same as bits_per_channel? If so, can we make it more explicit? Renamed to kBitsPerChannel. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:119: 0, 1, 0, 0, 0, 0, 0, 0 }; On 2012/12/14 01:19:21, xhwang wrote: > hmm, I still don't get it. What does a 0 or 1 channel layout mean? > These are not layouts: They are channel values. Only valid for 1 or 2 channels, so: > If we only use the first two items in this list, why not just define a > kDefaultOpusChannelLayout[kMaxChannelsWithDefaultChannelLayout], where the value > of kMaxChannelsWithDefaultChannelLayout is 2? Done. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:188: if (header->channels > 2) { On 2012/12/14 01:19:21, xhwang wrote: > s/2/kMaxChannelsWithDefaultChannelLayout if you follow the above comment Done. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:307: // read request. On 2012/12/14 01:19:21, xhwang wrote: > This comment is not necessary. Most decoders (e.g. VideoDecoders) don't queue > buffers. Done. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:457: memcpy(&channel_mapping, &kDefaultOpusChannelLayout, kMaxVorbisChannels); On 2012/12/14 01:19:21, xhwang wrote: > why do you need "&"? Forgot to remove, thanks. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:458: if (channel_count > 2) { On 2012/12/14 01:19:21, xhwang wrote: > ditto about 2 Done. https://codereview.chromium.org/11416367/diff/6008/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:566: base::ResetAndReturn(&read_cb_).Run(kOk, output); On 2012/12/14 01:19:21, xhwang wrote: > Can we keep the read_cb_ firing in one function. The success path is here but > the fail path is in line 382, which is hard to track. Done. Not sure if the wrapper function is more readable then duping the code, though.
mostly looking good. I don't quite like RunReadCBOrReadFromDemuxerStream() but I am not sure what the best way to do it. Another question, have you run this in media pipeline with real Opus audio? https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:160: uint8 stream_map[kMaxVorbisChannels]; should this be stream_map[kMaxChannelsWithDefaultLayout] ? https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:335: // to output any remaining data still in the decoder. I wonder does opus buffers any data inside? https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:379: } Is this if/else block equivalent to? if (last_input_timestamp_ != kNoTimestamp() && input->GetTimestamp() != kNoTimestamp() && input->GetTimestamp() < last_input_timestamp_) { DVLOG(1) << ... base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); return; } last_input_timestamp_ = input->GetTimestamp(); https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:414: if (config.bits_per_channel() != kBitsPerChannel) { +1, this makes more sense now! https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:543: DCHECK_EQ(dropped_size % 2, 0); s/2/kBytesPerChannel? Correct me if I am wrong. https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:571: void OpusAudioDecoder::RunReadCBOrReadFromDemuxerStream() { hmm, this seems overkill. Also this function is doing many different things. I think we have two easier options here: 1) Fire read_cb_ in this Decode() function for both success and failure pathes. Here's an example: http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/decrypting_v... 2) You can have an output parameter for the decoded buffer, then we always fire read_cb_ in DoDecoderBuffer. In either cases, we don't need a class member variable for the decoded buffer. https://codereview.chromium.org/11416367/diff/7003/media/webm/webm_tracks_par... File media/webm/webm_tracks_parser.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/webm/webm_tracks_par... media/webm/webm_tracks_parser.cc:142: str != "V_VP8") { nit: if (id == kWebMIdCodecID && str != "A_VORBIS" && str != "A_OPUS" && str != "V_VP8") { https://codereview.chromium.org/11416367/diff/7003/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/11416367/diff/7003/net/base/mime_util.cc#newc... net/base/mime_util.cc:816: "audio/opus", it seems there are ordered, fix the order? https://codereview.chromium.org/11416367/diff/7003/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): https://codereview.chromium.org/11416367/diff/7003/webkit/media/filter_helper... webkit/media/filter_helpers.cc:42: } media pipeline tries each audio decoder in the order they are pushed until an audio decoder that can decode certain input stream is found. since in most cases audio is not opus (and I assume FFmpegAudioDecoder cannot decode opus?), can we move this block to line 47?
PTAL, thanks! https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:160: uint8 stream_map[kMaxVorbisChannels]; On 2012/12/14 08:41:48, xhwang wrote: > should this be stream_map[kMaxChannelsWithDefaultLayout] ? No. This supports up to 8 channels. https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:335: // to output any remaining data still in the decoder. On 2012/12/14 08:41:48, xhwang wrote: > I wonder does opus buffers any data inside? I'm not certain, but I don't think this harms anything (outside of the additional code to read). Added TODO. https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:379: } On 2012/12/14 08:41:48, xhwang wrote: > Is this if/else block equivalent to? > > if (last_input_timestamp_ != kNoTimestamp() && > input->GetTimestamp() != kNoTimestamp() && > input->GetTimestamp() < last_input_timestamp_) { > DVLOG(1) << ... > base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL); > return; > } > > last_input_timestamp_ = input->GetTimestamp(); Changed to this way, since it's simpler. Might be applicable to FFmpegAudioDecoder, which is where the original came from (though what happens here is simpler because we don't need to handle negative vorbis timestamps) https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:543: DCHECK_EQ(dropped_size % 2, 0); On 2012/12/14 08:41:48, xhwang wrote: > s/2/kBytesPerChannel? Correct me if I am wrong. Done. https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:571: void OpusAudioDecoder::RunReadCBOrReadFromDemuxerStream() { On 2012/12/14 08:41:48, xhwang wrote: > hmm, this seems overkill. Also this function is doing many different things. I > think we have two easier options here: > 1) Fire read_cb_ in this Decode() function for both success and failure pathes. > Here's an example: > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/decrypting_v... > > 2) You can have an output parameter for the decoded buffer, then we always fire > read_cb_ in DoDecoderBuffer. > > In either cases, we don't need a class member variable for the decoded buffer. Went with option 2. https://codereview.chromium.org/11416367/diff/7003/media/webm/webm_tracks_par... File media/webm/webm_tracks_parser.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/webm/webm_tracks_par... media/webm/webm_tracks_parser.cc:142: str != "V_VP8") { On 2012/12/14 08:41:48, xhwang wrote: > nit: > > if (id == kWebMIdCodecID && > str != "A_VORBIS" && str != "A_OPUS" && str != "V_VP8") { Done. https://codereview.chromium.org/11416367/diff/7003/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/11416367/diff/7003/net/base/mime_util.cc#newc... net/base/mime_util.cc:816: "audio/opus", On 2012/12/14 08:41:48, xhwang wrote: > it seems there are ordered, fix the order? Done. https://codereview.chromium.org/11416367/diff/7003/webkit/media/filter_helper... File webkit/media/filter_helpers.cc (right): https://codereview.chromium.org/11416367/diff/7003/webkit/media/filter_helper... webkit/media/filter_helpers.cc:42: } On 2012/12/14 08:41:48, xhwang wrote: > media pipeline tries each audio decoder in the order they are pushed until an > audio decoder that can decode certain input stream is found. since in most cases > audio is not opus (and I assume FFmpegAudioDecoder cannot decode opus?), can we > move this block to line 47? Done.
looking pretty good! mostly some code organization / high level comments https://codereview.chromium.org/11416367/diff/18002/media/base/audio_decoder_... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/18002/media/base/audio_decoder_... media/base/audio_decoder_config.h:33: // Additional codecs for all platforms. nit: this comment will likely become invalid at some point :) can you remove this comment as well as the above "ChromiumOS" and "ChromeOS" commnets? https://codereview.chromium.org/11416367/diff/18002/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/11416367/diff/18002/media/base/media_switches... media/base/media_switches.h:52: MEDIA_EXPORT extern const char kEnableVp9OpusPlayback[]; should this just be --enable-opus-playback? for example... what if we want to release opus support to stable sooner than vp9? https://codereview.chromium.org/11416367/diff/18002/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:241: // |OpusAudioDecoder| is the only provider of Opus support. nit: drop || around class names (OpusAudioDecoder) https://codereview.chromium.org/11416367/diff/18002/media/filters/chunk_demux... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:55: &kOpusCodecInfo, does this add support for opus to chunk demuxer / media source? https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:62: DCHECK(channel_layout); you handle having a null channel_layout on line 64 is it ever valid to have null channel_layout? if not -- remove the check from line 64 https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:63: DCHECK_LE(num_channels, kMaxVorbisChannels); ditto for this check -- you handle it gracefully on the next line if this is something that shouldn't be valid, just keep the DCHECKs and make this function void https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:114: // OpusHeader, and passed to |opus_multistream_decoder_create()| when the nit: function names don't need || around them https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:171: DCHECK_GE(data_size, kOpusHeaderSize); ditto for all these dchecks try to aim for having void functions that mandate proper arguments passed in and that crash if you don't https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:180: LOG(ERROR) << "ParseOpusHeader(): invalid channel count in header " nit: remove function names from all of these LOG messages the LOG macros spit out the line number, meaning you really don't need to duplicate the information as to what function it was https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:237: if (!message_loop_->BelongsToCurrentThread()) { replace this trampoline with a DCHECK(BelongsToCurrentThread()) (everything runs on the same thread) https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:243: DoInitialize(stream, status_cb, statistics_cb); inline this method into Initialize() https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:247: // Complete operation asynchronously on different stack of execution as per add a DCHECK(BelongsToCurrentThread(), inline the body of DoRead() and re-bind read_cb with BindToCurrentLoop() check out ffmpeg_video_decoder.cc for reference (ffmpeg_audio_decoder.cc is a bit behind the times!) https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:266: message_loop_->PostTask(FROM_HERE, base::Bind( add a DCHECK(BelongsToCurrentThread(), inline the body of DoReset() and re-bind closure with BindToCurrentLoop() check out ffmpeg_video_decoder.cc for reference (ffmpeg_audio_decoder.cc is a bit behind the times!) https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:316: if (!message_loop_->BelongsToCurrentThread()) { replace this trampoline with a DCHECK(BelongsToCurrentThread()) (everything runs on the same thread) https://codereview.chromium.org/11416367/diff/18002/media/webm/webm_tracks_pa... File media/webm/webm_tracks_parser.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/webm/webm_tracks_pa... media/webm/webm_tracks_parser.cc:142: str != "A_VORBIS" && str != "A_OPUS" && str != "V_VP8") { will this make it work with media source? https://codereview.chromium.org/11416367/diff/18002/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/11416367/diff/18002/net/base/mime_util.cc#new... net/base/mime_util.cc:311: "opus", this should be added if the flag is enabled perhaps remove it from this list and check the command line flag + add it around line 478? https://codereview.chromium.org/11416367/diff/18002/net/base/mime_util.cc#new... net/base/mime_util.cc:425: { "video/webm", "vorbis,opus,vp8,vp8.0" }, ditto https://codereview.chromium.org/11416367/diff/18002/net/base/mime_util.cc#new... net/base/mime_util.cc:815: "audio/opus", is this really a mime type? this list is typically for containers -- we use the codecs= mime parameter for specifying codecs inside containers
Most comments addressed with the following exceptions" - backed out the trampoline related comments and inlining of Do* methods - not sure what to do about checking a media flag from net (mime_util comments) https://codereview.chromium.org/11416367/diff/18002/media/base/audio_decoder_... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/11416367/diff/18002/media/base/audio_decoder_... media/base/audio_decoder_config.h:33: // Additional codecs for all platforms. On 2012/12/14 21:13:34, scherkus wrote: > nit: this comment will likely become invalid at some point :) > > can you remove this comment as well as the above "ChromiumOS" and "ChromeOS" > commnets? Done. https://codereview.chromium.org/11416367/diff/18002/media/base/media_switches.h File media/base/media_switches.h (right): https://codereview.chromium.org/11416367/diff/18002/media/base/media_switches... media/base/media_switches.h:52: MEDIA_EXPORT extern const char kEnableVp9OpusPlayback[]; On 2012/12/14 21:13:34, scherkus wrote: > should this just be --enable-opus-playback? > > for example... what if we want to release opus support to stable sooner than > vp9? Done. https://codereview.chromium.org/11416367/diff/18002/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:241: // |OpusAudioDecoder| is the only provider of Opus support. On 2012/12/14 21:13:34, scherkus wrote: > nit: drop || around class names (OpusAudioDecoder) Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/chunk_demux... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:55: &kOpusCodecInfo, On 2012/12/14 21:13:34, scherkus wrote: > does this add support for opus to chunk demuxer / media source? It does not work; removed changes in this file. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:62: DCHECK(channel_layout); On 2012/12/14 21:13:34, scherkus wrote: > you handle having a null channel_layout on line 64 > > is it ever valid to have null channel_layout? if not -- remove the check from > line 64 Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:63: DCHECK_LE(num_channels, kMaxVorbisChannels); On 2012/12/14 21:13:34, scherkus wrote: > ditto for this check -- you handle it gracefully on the next line > > if this is something that shouldn't be valid, just keep the DCHECKs and make > this function void Function returns void now. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:114: // OpusHeader, and passed to |opus_multistream_decoder_create()| when the On 2012/12/14 21:13:34, scherkus wrote: > nit: function names don't need || around them Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:171: DCHECK_GE(data_size, kOpusHeaderSize); On 2012/12/14 21:13:34, scherkus wrote: > ditto for all these dchecks > > try to aim for having void functions that mandate proper arguments passed in and > that crash if you don't Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:180: LOG(ERROR) << "ParseOpusHeader(): invalid channel count in header " On 2012/12/14 21:13:34, scherkus wrote: > nit: remove function names from all of these LOG messages > > the LOG macros spit out the line number, meaning you really don't need to > duplicate the information as to what function it was Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:237: if (!message_loop_->BelongsToCurrentThread()) { On 2012/12/14 21:13:34, scherkus wrote: > replace this trampoline with a DCHECK(BelongsToCurrentThread()) (everything runs > on the same thread) Done, but reverted; discussed offline. Applies to remaining trampoline comments and inlining of Do* method bodies. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:243: DoInitialize(stream, status_cb, statistics_cb); On 2012/12/14 21:13:34, scherkus wrote: > inline this method into Initialize() Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:247: // Complete operation asynchronously on different stack of execution as per On 2012/12/14 21:13:34, scherkus wrote: > add a DCHECK(BelongsToCurrentThread(), inline the body of DoRead() and re-bind > read_cb with BindToCurrentLoop() > > check out ffmpeg_video_decoder.cc for reference (ffmpeg_audio_decoder.cc is a > bit behind the times!) Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:266: message_loop_->PostTask(FROM_HERE, base::Bind( On 2012/12/14 21:13:34, scherkus wrote: > add a DCHECK(BelongsToCurrentThread(), inline the body of DoReset() and re-bind > closure with BindToCurrentLoop() > > check out ffmpeg_video_decoder.cc for reference (ffmpeg_audio_decoder.cc is a > bit behind the times!) Done. https://codereview.chromium.org/11416367/diff/18002/media/filters/opus_audio_... media/filters/opus_audio_decoder.cc:316: if (!message_loop_->BelongsToCurrentThread()) { On 2012/12/14 21:13:34, scherkus wrote: > replace this trampoline with a DCHECK(BelongsToCurrentThread()) (everything runs > on the same thread) Done. https://codereview.chromium.org/11416367/diff/18002/media/webm/webm_tracks_pa... File media/webm/webm_tracks_parser.cc (right): https://codereview.chromium.org/11416367/diff/18002/media/webm/webm_tracks_pa... media/webm/webm_tracks_parser.cc:142: str != "A_VORBIS" && str != "A_OPUS" && str != "V_VP8") { On 2012/12/14 21:13:34, scherkus wrote: > will this make it work with media source? Media source is not working with opus.
Hopefully the media stuff is now ready, PTAL. Now to find reviewers for the stuff in src/chrome/...
lgtm to me You'll also need scherkus's review. Be sure to add unit tests soon! https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:160: uint8 stream_map[kMaxVorbisChannels]; On 2012/12/14 20:21:34, Tom Finegan wrote: > On 2012/12/14 08:41:48, xhwang wrote: > > should this be stream_map[kMaxChannelsWithDefaultLayout] ? > > No. This supports up to 8 channels. ok. but we only copied two bytes. should we set the rest 6 bytes to zero? https://codereview.chromium.org/11416367/diff/8028/media/base/media_switches.cc File media/base/media_switches.cc (right): https://codereview.chromium.org/11416367/diff/8028/media/base/media_switches.... media/base/media_switches.cc:64: // Ensable VP9 and Opus playback on <video> elements. nit: remove VP9 https://codereview.chromium.org/11416367/diff/8028/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/8028/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:340: } return
+cpu cpu, Please do an OWNERS review of generated_resources.grd. I've added strings for support of a command line flag that enables Opus audio decode support in video elements.
few more nits https://codereview.chromium.org/11416367/diff/6014/media/base/media_switches.cc File media/base/media_switches.cc (right): https://codereview.chromium.org/11416367/diff/6014/media/base/media_switches.... media/base/media_switches.cc:64: // Ensable VP9 and Opus playback on <video> elements. s/Ensable/Enable/ s/VP9 and// s/<video>/media/ https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:32: static inline bool IsEndOfStream(int decoded_size, Buffer* input) { this function doesn't appear to be used https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:61: DCHECK(opus_mapping); nit: these check-for-null DCHECK()s aren't very helpful as we'll simply crash when attempting to access them (just remove them) https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:164: DCHECK(data); nit: these check-for-null DCHECK()s aren't very helpful as we'll simply crash when attempting to access them (just remove them) https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:170: DCHECK(header->channels > 0 && header->channels <= kMaxVorbisChannels) if this is file parsing code you may opt for CHECK()ing and seeing if we need run-time graceful fallback in the future https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:188: const int mapping_required_size = nit: I don't see this used -- perhaps roll the addition into the DCHECK? https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.h (right): https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.h:8: #include <list> are you using <list>?
Nits addressed. https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/7003/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:160: uint8 stream_map[kMaxVorbisChannels]; On 2012/12/14 23:35:27, xhwang wrote: > On 2012/12/14 20:21:34, Tom Finegan wrote: > > On 2012/12/14 08:41:48, xhwang wrote: > > > should this be stream_map[kMaxChannelsWithDefaultLayout] ? > > > > No. This supports up to 8 channels. > > ok. but we only copied two bytes. should we set the rest 6 bytes to zero? I was previously copying all 8 bytes, but I changed it because you pointed out that is doesn't matter if I'm always writing the layout values for channel counts > 2. That's what I was using the default layout for ({ 0, 1, 0, 0, 0, 0, 0, 0 }). It doesn't matter though, anything > 2 gets rewritten. https://codereview.chromium.org/11416367/diff/8028/media/base/media_switches.cc File media/base/media_switches.cc (right): https://codereview.chromium.org/11416367/diff/8028/media/base/media_switches.... media/base/media_switches.cc:64: // Ensable VP9 and Opus playback on <video> elements. On 2012/12/14 23:35:27, xhwang wrote: > nit: remove VP9 Done. https://codereview.chromium.org/11416367/diff/8028/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/8028/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:340: } On 2012/12/14 23:35:27, xhwang wrote: > return Done. https://codereview.chromium.org/11416367/diff/6014/media/base/media_switches.cc File media/base/media_switches.cc (right): https://codereview.chromium.org/11416367/diff/6014/media/base/media_switches.... media/base/media_switches.cc:64: // Ensable VP9 and Opus playback on <video> elements. On 2012/12/14 23:39:28, scherkus wrote: > s/Ensable/Enable/ > > s/VP9 and// > > s/<video>/media/ Done. https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.cc (right): https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:32: static inline bool IsEndOfStream(int decoded_size, Buffer* input) { On 2012/12/14 23:39:28, scherkus wrote: > this function doesn't appear to be used It's used in one place (at the end of Decode()). https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:61: DCHECK(opus_mapping); On 2012/12/14 23:39:28, scherkus wrote: > nit: these check-for-null DCHECK()s aren't very helpful as we'll simply crash > when attempting to access them (just remove them) Done. https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:164: DCHECK(data); On 2012/12/14 23:39:28, scherkus wrote: > nit: these check-for-null DCHECK()s aren't very helpful as we'll simply crash > when attempting to access them (just remove them) Done. https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.cc:170: DCHECK(header->channels > 0 && header->channels <= kMaxVorbisChannels) On 2012/12/14 23:39:28, scherkus wrote: > if this is file parsing code you may opt for CHECK()ing and seeing if we need > run-time graceful fallback in the future Used CHECK per offline discussion (this is file parsing code, and these really are errors). https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... File media/filters/opus_audio_decoder.h (right): https://codereview.chromium.org/11416367/diff/6014/media/filters/opus_audio_d... media/filters/opus_audio_decoder.h:8: #include <list> On 2012/12/14 23:39:28, scherkus wrote: > are you using <list>? Done.
lgtm
Ben: Please do OWNERS reviews of: - chrome/browser/about_flags.cc - content/browser/renderer_host/render_process_host_impl.cc These are very small changes to support a flag that enables/disables Opus audio playback support in media elements. Thanks!
cpu, Please do an OWNERS review of generated_resources.grd. I've added strings for support of a command line flag that enables Opus audio decode support in video elements.
I haven't looked through the decoder, but can you add at least a quick PipelineIntegration unittest which decodes and opus file? https://codereview.chromium.org/11416367/diff/16020/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/16020/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:237: if (codec == kCodecOpus) { I suspect this is going to conflict with https://codereview.chromium.org/11280301/ which now uses SampleFormat to initialize the decoder config. Can you move this check below CodecIDToAudioCodec() and change it to be: codec_context->sample_fmt = AV_SAMPLE_FMT_S16? bytes_per_channel will get updated appropriately through the normal path then.
Dale, PTAL: > I haven't looked through the decoder, but can you add at least a quick > PipelineIntegration unittest which decodes and opus file? Can that _please_ wait until next week? I want to make the branch cut, and I still need to land the libvpx wrapper. https://codereview.chromium.org/11416367/diff/16020/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/16020/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:237: if (codec == kCodecOpus) { On 2012/12/15 00:13:47, DaleCurtis wrote: > I suspect this is going to conflict with > https://codereview.chromium.org/11280301/ which now uses SampleFormat to > initialize the decoder config. > > Can you move this check below CodecIDToAudioCodec() and change it to be: > codec_context->sample_fmt = AV_SAMPLE_FMT_S16? bytes_per_channel will get > updated appropriately through the normal path then. I can't update the value in codec_context without changing the function: codec_context is const. I moved the check to just below the bytes_per_channel declaration.
Per offline discussion with Dale I tweaked the way sample size is handled in ffmpeg_common for Opus.
As discussed offline test is okay for next week. https://codereview.chromium.org/11416367/diff/19008/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/19008/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:233: AVSampleFormat sample_format = codec_context->sample_fmt; I meant you should do this above line 231 and replace the "codec_context->sample_fmt" there with just "sample_fmt" that way you don't have two pieces of code calling av_get_bytes_per_sample().
https://codereview.chromium.org/11416367/diff/19008/media/ffmpeg/ffmpeg_commo... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11416367/diff/19008/media/ffmpeg/ffmpeg_commo... media/ffmpeg/ffmpeg_common.cc:233: AVSampleFormat sample_format = codec_context->sample_fmt; On 2012/12/15 01:13:22, DaleCurtis wrote: > I meant you should do this above line 231 and replace the > "codec_context->sample_fmt" there with just "sample_fmt" that way you don't have > two pieces of code calling av_get_bytes_per_sample(). Done.
lgtm
+brettw for OWNERS review of command line flag changes. PTAL Thanks!
owners lgtm rubberstamp
Made the description more informative.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11416367/36002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11416367/11050
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11416367/11050
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11416367/11050
Failed to apply patch for webkit/media/filter_helpers.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file webkit/media/filter_helpers.cc
Hunk #1 succeeded at 5 with fuzz 2.
Hunk #2 FAILED at 30.
1 out of 2 hunks FAILED -- saving rejects to file
webkit/media/filter_helpers.cc.rej
Patch: webkit/media/filter_helpers.cc
Index: webkit/media/filter_helpers.cc
diff --git a/webkit/media/filter_helpers.cc b/webkit/media/filter_helpers.cc
index
362810c10ff0559c82be9307a1c36d5b7d4dd5b8..b52adc07358eecea5c9765d3ff49b2513d1b2f71
100644
--- a/webkit/media/filter_helpers.cc
+++ b/webkit/media/filter_helpers.cc
@@ -5,12 +5,15 @@
#include "webkit/media/filter_helpers.h"
#include "base/bind.h"
+#include "base/command_line.h"
#include "media/base/filter_collection.h"
+#include "media/base/media_switches.h"
#include "media/filters/chunk_demuxer.h"
#include "media/filters/dummy_demuxer.h"
#include "media/filters/ffmpeg_audio_decoder.h"
#include "media/filters/ffmpeg_demuxer.h"
#include "media/filters/ffmpeg_video_decoder.h"
+#include "media/filters/opus_audio_decoder.h"
#include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebURL.h"
#include "webkit/media/crypto/proxy_decryptor.h"
#include "webkit/media/media_stream_client.h"
@@ -27,10 +30,18 @@ static void AddDefaultDecodersToCollection(
const scoped_refptr<base::MessageLoopProxy>& message_loop,
media::FilterCollection* filter_collection,
ProxyDecryptor* proxy_decryptor) {
+
scoped_refptr<media::FFmpegAudioDecoder> ffmpeg_audio_decoder =
new media::FFmpegAudioDecoder(message_loop);
filter_collection->GetAudioDecoders()->push_back(ffmpeg_audio_decoder);
+ const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+ if (cmd_line->HasSwitch(switches::kEnableOpusPlayback)) {
+ scoped_refptr<media::OpusAudioDecoder> opus_audio_decoder =
+ new media::OpusAudioDecoder(message_loop);
+ filter_collection->GetAudioDecoders()->push_back(opus_audio_decoder);
+ }
+
scoped_refptr<media::FFmpegVideoDecoder> ffmpeg_video_decoder =
new media::FFmpegVideoDecoder(message_loop, proxy_decryptor);
filter_collection->GetVideoDecoders()->push_back(ffmpeg_video_decoder);
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
