Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752323002:
Support Opus Ambisonics playback (Closed)
|
Created:
3 years, 9 months ago by flim-chromium Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport Opus Ambisonics playback
Opus Ambisonics does not have a prescribed layout, so CHANNEL_LAYOUT_DISCRETE is used. In this case, the audio decoders and renderer also need to know the number of channels.
BUG=693745
Review-Url: https://codereview.chromium.org/2752323002
Cr-Commit-Position: refs/heads/master@{#475035}
Committed: https://chromium.googlesource.com/chromium/src/+/162236743c74fa040e822ea4d846215e4543791e
Patch Set 1 #Patch Set 2 : FFmpeg test + enable upmix of audio with discrete channel layout #Patch Set 3 : +tests #
Total comments: 15
Patch Set 4 : Address Dale's comments #Patch Set 5 : git cl format #Patch Set 6 : rebase #Patch Set 7 : Fix channel layout after rebase and ensure channels > 8 for discrete layout #Patch Set 8 : If WebAudio is not used and the input channel layout is discrete, default to the hardware layout #
Total comments: 14
Patch Set 9 : +pipeline integration tests, +test media, minor fixes #
Total comments: 6
Patch Set 10 : add webaudio check before rendering discrete + address comments #Patch Set 11 : rebase #Patch Set 12 : actually add config_ change #
Total comments: 6
Patch Set 13 : Replace IsWebAudioSource with just DISCRETE layout #Patch Set 14 : rebase #Patch Set 15 : Fix issues from rebase #
Total comments: 14
Patch Set 16 : Address comments #
Total comments: 8
Patch Set 17 : 8ch comments + kClockless tests #Patch Set 18 : rebase to get updated DEPS from CL 2901383003 #Patch Set 19 : another fix for unreliable ffmpeg channels vs layout #Messages
Total messages: 57 (14 generated)
Description was changed from ========== Support Opus Ambisonics playback Opus Ambisonics does not have a prescribed layout, so CHANNEL_LAYOUT_DISCRETE is used. In this case, the audio decoders and renderer also need to know the number of channels. Use CHANNEL_LAYOUT_DISCRETE for Opus Ambisonics file BUG= ========== to ========== Support Opus Ambisonics playback Opus Ambisonics does not have a prescribed layout, so CHANNEL_LAYOUT_DISCRETE is used. In this case, the audio decoders and renderer also need to know the number of channels. BUG=693745 ==========
flim@chromium.org changed reviewers: + dalecurtis@chromium.org, hongchan@chromium.org
https://codereview.chromium.org/2752323002/diff/40001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2752323002/diff/40001/media/base/audio_decode... media/base/audio_decoder_config.h:66: void set_channels_for_discrete(int channels) { Should be SetChannelsForDiscrete(), since hacker_style() is only allowed for simple (single-value, no dcheck) setters. https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... media/ffmpeg/ffmpeg_common.cc:328: int is_opus_ambisonics = 0; bool https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... media/ffmpeg/ffmpeg_common.cc:334: ? CHANNEL_LAYOUT_DISCRETE This should also include channels > kMaxConcurrentChannels; otherwise an existing layout should be mapped. https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:88: int is_opus_ambisonics = 0; bool. https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:93: ChannelLayout channel_layout = is_opus_ambisonics && channels > 8. https://codereview.chromium.org/2752323002/diff/40001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:976: const int input_channel_count = Just always use last_decoded_channels_ ?
https://codereview.chromium.org/2752323002/diff/40001/media/base/audio_decode... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2752323002/diff/40001/media/base/audio_decode... media/base/audio_decoder_config.h:66: void set_channels_for_discrete(int channels) { On 2017/03/20 17:38:58, DaleCurtis wrote: > Should be SetChannelsForDiscrete(), since hacker_style() is only allowed for > simple (single-value, no dcheck) setters. Done. https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... media/ffmpeg/ffmpeg_common.cc:328: int is_opus_ambisonics = 0; On 2017/03/20 17:38:58, DaleCurtis wrote: > bool Done. https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... media/ffmpeg/ffmpeg_common.cc:328: int is_opus_ambisonics = 0; Also renamed to is_opus_discrete to remove focus from ambisonics since it may also make sense to make other Opus channel mappings discrete (e.g. 255?) https://codereview.chromium.org/2752323002/diff/40001/media/ffmpeg/ffmpeg_com... media/ffmpeg/ffmpeg_common.cc:334: ? CHANNEL_LAYOUT_DISCRETE On 2017/03/20 17:38:58, DaleCurtis wrote: > This should also include channels > kMaxConcurrentChannels; otherwise an > existing layout should be mapped. Done. Refactored slightly to avoid having to redefine kMaxConcurrentChannels here. https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:88: int is_opus_ambisonics = 0; On 2017/03/20 17:38:58, DaleCurtis wrote: > bool. Done. https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:93: ChannelLayout channel_layout = is_opus_ambisonics On 2017/03/20 17:38:58, DaleCurtis wrote: > && channels > 8. Ambisonics signals could also be < 8 channels. Or do you mean even if it's not opus ambisonics signal? https://codereview.chromium.org/2752323002/diff/40001/media/renderers/audio_r... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/renderers/audio_r... media/renderers/audio_renderer_impl.cc:976: const int input_channel_count = On 2017/03/20 17:38:58, DaleCurtis wrote: > Just always use last_decoded_channels_ ? Done.
https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:93: ChannelLayout channel_layout = is_opus_ambisonics On 2017/03/22 at 06:20:51, flim-chromium wrote: > On 2017/03/20 17:38:58, DaleCurtis wrote: > > && channels > 8. > > Ambisonics signals could also be < 8 channels. Or do you mean even if it's not opus ambisonics signal? I mean I don't think DISCRETE will be passed or work correctly all through out the stack if < 8 channels; specifically OSX may have troubles. You'll need to do some further research at all the places we convert from channel count to layout and use DISCRETE today.
Thanks for the review so far. The latest patch (8) also ensures that if the input channel layout is discrete, we just fall back to the hardware layout and truncate playout to the hardware channel count. Does this behaviour make sense? https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/40001/media/filters/ffmpeg_au... media/filters/ffmpeg_audio_decoder.cc:93: ChannelLayout channel_layout = is_opus_ambisonics On 2017/03/22 18:56:30, DaleCurtis wrote: > On 2017/03/22 at 06:20:51, flim-chromium wrote: > > On 2017/03/20 17:38:58, DaleCurtis wrote: > > > && channels > 8. > > > > Ambisonics signals could also be < 8 channels. Or do you mean even if it's not > opus ambisonics signal? > > I mean I don't think DISCRETE will be passed or work correctly all through out > the stack if < 8 channels; specifically OSX may have troubles. You'll need to do > some further research at all the places we convert from channel count to layout > and use DISCRETE today. Thanks for the explanation. I've added '&& channels > 8'.
Most of files live in /media and I am not familiar with the code base. I defer my review to Dale. If there is any issue related to WebAudio, feel free to ping me again.
https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is Is this necessary here? I would have thought the browser side would handle it appropriately. https://codereview.chromium.org/2752323002/diff/140001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:337: if (is_opus_discrete || channel_layout == CHANNEL_LAYOUT_UNSUPPORTED) Do we need this || clause? Seems like we might allow playback of unexpected things (potentially leading to security issues). https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; Isn't this information already in the AudioDecoderConfig?
Can you add a media/test/pipeline_integration_test for this? Overall this seems fine.
Sure, the pipeline tests have been added. https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is On 2017/03/29 22:46:55, DaleCurtis wrote: > Is this necessary here? I would have thought the browser side would handle it > appropriately. Without this check, I get the browser failing with an "Aw, Snap!" page crash. Should this be handled somewhere else instead? https://codereview.chromium.org/2752323002/diff/140001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:337: if (is_opus_discrete || channel_layout == CHANNEL_LAYOUT_UNSUPPORTED) On 2017/03/29 22:46:55, DaleCurtis wrote: > Do we need this || clause? Seems like we might allow playback of unexpected > things (potentially leading to security issues). Sorry, I misunderstood your original comment. Fixed now. https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; On 2017/03/29 22:46:55, DaleCurtis wrote: > Isn't this information already in the AudioDecoderConfig? True, but this is a static function and the non-static config_ can't be accessed here?
https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is On 2017/03/30 at 19:35:10, flim-chromium wrote: > On 2017/03/29 22:46:55, DaleCurtis wrote: > > Is this necessary here? I would have thought the browser side would handle it > > appropriately. > > Without this check, I get the browser failing with an "Aw, Snap!" page crash. Should this be handled somewhere else instead? What's crashing? https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; On 2017/03/30 at 19:35:10, flim-chromium wrote: > On 2017/03/29 22:46:55, DaleCurtis wrote: > > Isn't this information already in the AudioDecoderConfig? > > True, but this is a static function and the non-static config_ can't be accessed here? Ah right, this isn't like FFmpegVideoDecoder where we have a class level method. Is it enough to just check OPUS && channels > 8?
https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is On 2017/03/30 20:22:21, DaleCurtis wrote: > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > Is this necessary here? I would have thought the browser side would handle > it > > > appropriately. > > > > Without this check, I get the browser failing with an "Aw, Snap!" page crash. > Should this be handled somewhere else instead? > > What's crashing? The stack trace is: [1:12:0330/163613.993344:FATAL:audio_converter.cc(35)] Check failed: input_params.IsValid(). #0 0x7f6c917eca6b base::debug::StackTrace::StackTrace() #1 0x7f6c917eb76c base::debug::StackTrace::StackTrace() #2 0x7f6c9185496f logging::LogMessage::~LogMessage() #3 0x7f6c88445ff0 media::AudioConverter::AudioConverter() #4 0x7f6c8844ecf2 media::AudioRendererMixer::AudioRendererMixer() #5 0x7f6c8c0a5485 content::AudioRendererMixerManager::GetMixer() #6 0x7f6c88454507 media::AudioRendererMixerInput::Start() #7 0x7f6c7ca40e1f media::WebAudioSourceProviderImpl::Start() #8 0x7f6c8832cd15 media::AudioRendererImpl::OnAudioBufferStreamInitialized() ... where input_params in audio_converter.cc(35) has DISCRETE layout but 0 channels. https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; On 2017/03/30 20:22:21, DaleCurtis wrote: > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > Isn't this information already in the AudioDecoderConfig? > > > > True, but this is a static function and the non-static config_ can't be > accessed here? > > Ah right, this isn't like FFmpegVideoDecoder where we have a class level method. > Is it enough to just check OPUS && channels > 8? I believe so - it includes the additional check that the Opus packet was encoded specifically with channel mapping 2. This mirrors the check in media/ffmpeg/ffmpeg_common.cc. I'd renamed to |is_opus_discrete| in both places because we'd want to extend this in the future to include mapping_family == 3 (and maybe 255 if that makes sense)
https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is On 2017/03/31 at 00:12:14, flim-chromium wrote: > On 2017/03/30 20:22:21, DaleCurtis wrote: > > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > > Is this necessary here? I would have thought the browser side would handle > > it > > > > appropriately. > > > > > > Without this check, I get the browser failing with an "Aw, Snap!" page crash. > > Should this be handled somewhere else instead? > > > > What's crashing? > > The stack trace is: > > [1:12:0330/163613.993344:FATAL:audio_converter.cc(35)] Check failed: input_params.IsValid(). > #0 0x7f6c917eca6b base::debug::StackTrace::StackTrace() > #1 0x7f6c917eb76c base::debug::StackTrace::StackTrace() > #2 0x7f6c9185496f logging::LogMessage::~LogMessage() > #3 0x7f6c88445ff0 media::AudioConverter::AudioConverter() > #4 0x7f6c8844ecf2 media::AudioRendererMixer::AudioRendererMixer() > #5 0x7f6c8c0a5485 content::AudioRendererMixerManager::GetMixer() > #6 0x7f6c88454507 media::AudioRendererMixerInput::Start() > #7 0x7f6c7ca40e1f media::WebAudioSourceProviderImpl::Start() > #8 0x7f6c8832cd15 media::AudioRendererImpl::OnAudioBufferStreamInitialized() > ... > > where input_params in audio_converter.cc(35) has DISCRETE layout but 0 channels. Seem like AudioConverter needs to be taught how to handle discrete channels. https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; On 2017/03/31 at 00:12:14, flim-chromium wrote: > On 2017/03/30 20:22:21, DaleCurtis wrote: > > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > > Isn't this information already in the AudioDecoderConfig? > > > > > > True, but this is a static function and the non-static config_ can't be > > accessed here? > > > > Ah right, this isn't like FFmpegVideoDecoder where we have a class level method. > > Is it enough to just check OPUS && channels > 8? > > I believe so - it includes the additional check that the Opus packet was encoded specifically with channel mapping 2. This mirrors the check in media/ffmpeg/ffmpeg_common.cc. > > I'd renamed to |is_opus_discrete| in both places because we'd want to extend this in the future to include mapping_family == 3 (and maybe 255 if that makes sense) I'm moving this to be a class method in https://codereview.chromium.org/2788483003 so we can use the config_ here if that lands before yours. https://codereview.chromium.org/2752323002/diff/160001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/160001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:80: Needs set_channels. https://codereview.chromium.org/2752323002/diff/160001/media/renderers/audio_... File media/renderers/audio_renderer_impl_unittest.cc (right): https://codereview.chromium.org/2752323002/diff/160001/media/renderers/audio_... media/renderers/audio_renderer_impl_unittest.cc:662: int audio_channels = 6; Should be impossible to get discrete + 6 channels per the ffmpeg_common code? https://codereview.chromium.org/2752323002/diff/160001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/160001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:912: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { Do we need the 4ch one if we have the 11ch one? We only do if you're planning to mark < 8 channels as non-discrete. This should test the mapping which gets sent to the NullAudioSink as well.
https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/140001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:73: // If the input has discrete channel layout, the hardware channel mapping is On 2017/03/31 19:06:53, DaleCurtis wrote: > On 2017/03/31 at 00:12:14, flim-chromium wrote: > > On 2017/03/30 20:22:21, DaleCurtis wrote: > > > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > > > Is this necessary here? I would have thought the browser side would > handle > > > it > > > > > appropriately. > > > > > > > > Without this check, I get the browser failing with an "Aw, Snap!" page > crash. > > > Should this be handled somewhere else instead? > > > > > > What's crashing? > > > > The stack trace is: > > > > [1:12:0330/163613.993344:FATAL:audio_converter.cc(35)] Check failed: > input_params.IsValid(). > > #0 0x7f6c917eca6b base::debug::StackTrace::StackTrace() > > #1 0x7f6c917eb76c base::debug::StackTrace::StackTrace() > > #2 0x7f6c9185496f logging::LogMessage::~LogMessage() > > #3 0x7f6c88445ff0 media::AudioConverter::AudioConverter() > > #4 0x7f6c8844ecf2 media::AudioRendererMixer::AudioRendererMixer() > > #5 0x7f6c8c0a5485 content::AudioRendererMixerManager::GetMixer() > > #6 0x7f6c88454507 media::AudioRendererMixerInput::Start() > > #7 0x7f6c7ca40e1f media::WebAudioSourceProviderImpl::Start() > > #8 0x7f6c8832cd15 media::AudioRendererImpl::OnAudioBufferStreamInitialized() > > ... > > > > where input_params in audio_converter.cc(35) has DISCRETE layout but 0 > channels. > > Seem like AudioConverter needs to be taught how to handle discrete channels. As discussed offline, I've removed this and added a webaudio check in AudioRendererImpl instead. https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/2752323002/diff/140001/media/filters/ffmpeg_a... media/filters/ffmpeg_audio_decoder.cc:99: bool is_opus_discrete = false; On 2017/03/31 19:06:53, DaleCurtis wrote: > On 2017/03/31 at 00:12:14, flim-chromium wrote: > > On 2017/03/30 20:22:21, DaleCurtis wrote: > > > On 2017/03/30 at 19:35:10, flim-chromium wrote: > > > > On 2017/03/29 22:46:55, DaleCurtis wrote: > > > > > Isn't this information already in the AudioDecoderConfig? > > > > > > > > True, but this is a static function and the non-static config_ can't be > > > accessed here? > > > > > > Ah right, this isn't like FFmpegVideoDecoder where we have a class level > method. > > > Is it enough to just check OPUS && channels > 8? > > > > I believe so - it includes the additional check that the Opus packet was > encoded specifically with channel mapping 2. This mirrors the check in > media/ffmpeg/ffmpeg_common.cc. > > > > I'd renamed to |is_opus_discrete| in both places because we'd want to extend > this in the future to include mapping_family == 3 (and maybe 255 if that makes > sense) > > I'm moving this to be a class method in > https://codereview.chromium.org/2788483003 so we can use the config_ here if > that lands before yours. thanks, this has been updated to use config_ https://codereview.chromium.org/2752323002/diff/160001/content/renderer/media... File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/2752323002/diff/160001/content/renderer/media... content/renderer/media/audio_renderer_mixer_manager.cc:80: On 2017/03/31 19:06:53, DaleCurtis wrote: > Needs set_channels. I ended up removing this. https://codereview.chromium.org/2752323002/diff/160001/media/renderers/audio_... File media/renderers/audio_renderer_impl_unittest.cc (right): https://codereview.chromium.org/2752323002/diff/160001/media/renderers/audio_... media/renderers/audio_renderer_impl_unittest.cc:662: int audio_channels = 6; On 2017/03/31 19:06:53, DaleCurtis wrote: > Should be impossible to get discrete + 6 channels per the ffmpeg_common code? Thanks for spotting this. It looks like UpmixDiscreteLayout below is also no longer needed; I've removed it. https://codereview.chromium.org/2752323002/diff/160001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/160001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:912: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/03/31 19:06:53, DaleCurtis wrote: > Do we need the 4ch one if we have the 11ch one? We only do if you're planning to > mark < 8 channels as non-discrete. This should test the mapping which gets sent > to the NullAudioSink as well. Now that we check that webaudio is attached before rendering discrete, are these pipeline tests still appropriate? As far as I understand, these seem to use media source?
Hmm, this isn't quite what I was expecting. I thought you would intercept the GetOutputDeviceInfo() call when WebAudio is attached so that the provision of the right parameters is seamless. Did you try that path?
https://codereview.chromium.org/2752323002/diff/220001/media/base/output_devi... File media/base/output_device_info.h (right): https://codereview.chromium.org/2752323002/diff/220001/media/base/output_devi... media/base/output_device_info.h:51: void SetIsWebAudioSource(bool value); Sorry skimmed review due to time yesterday. I see you're now doing what I thought you were. I don't think we need this though. Shouldn't it be enough to receive DISCRETE layout from WebAudio? https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:259: info = sink_->GetOutputDeviceInfo(); Instead of returning the true sink parameters when WebAudio is connected, I think you instead want to return some custom set of parameters from the WebAudio |client_|. I.e. whatever channel count its configured for along with the sample rate its currently using.
https://codereview.chromium.org/2752323002/diff/220001/media/base/output_devi... File media/base/output_device_info.h (right): https://codereview.chromium.org/2752323002/diff/220001/media/base/output_devi... media/base/output_device_info.h:51: void SetIsWebAudioSource(bool value); On 2017/04/06 19:44:42, DaleCurtis wrote: > Sorry skimmed review due to time yesterday. I see you're now doing what I > thought you were. I don't think we need this though. Shouldn't it be enough to > receive DISCRETE layout from WebAudio? That makes sense, done. https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:259: info = sink_->GetOutputDeviceInfo(); On 2017/04/06 19:44:42, DaleCurtis wrote: > Instead of returning the true sink parameters when WebAudio is connected, I > think you instead want to return some custom set of parameters from the WebAudio > |client_|. I.e. whatever channel count its configured for along with the sample > rate its currently using. It looks like the |client_| params are set only in WebAudioSourceProviderImpl::OnSetFormat, which is called from AudioRendererImpl after this function is called?
dalecurtis@chromium.org changed reviewers: + rtoy@chromium.org
https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:259: info = sink_->GetOutputDeviceInfo(); On 2017/04/07 at 22:01:43, flim-chromium wrote: > On 2017/04/06 19:44:42, DaleCurtis wrote: > > Instead of returning the true sink parameters when WebAudio is connected, I > > think you instead want to return some custom set of parameters from the WebAudio > > |client_|. I.e. whatever channel count its configured for along with the sample > > rate its currently using. > > It looks like the |client_| params are set only in WebAudioSourceProviderImpl::OnSetFormat, which is called from AudioRendererImpl after this function is called? Yes, that's how it works today, but I think we need it the other way around, so that WebAudio can specify it wants all channels instead of just whatever the hardware layout can handle. +rtoy for commentary.
https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... File media/blink/webaudiosourceprovider_impl.cc (right): https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... media/blink/webaudiosourceprovider_impl.cc:259: info = sink_->GetOutputDeviceInfo(); On 2017/04/10 19:38:51, DaleCurtis wrote: > On 2017/04/07 at 22:01:43, flim-chromium wrote: > > On 2017/04/06 19:44:42, DaleCurtis wrote: > > > Instead of returning the true sink parameters when WebAudio is connected, I > > > think you instead want to return some custom set of parameters from the > WebAudio > > > |client_|. I.e. whatever channel count its configured for along with the > sample > > > rate its currently using. > > > > It looks like the |client_| params are set only in > WebAudioSourceProviderImpl::OnSetFormat, which is called from AudioRendererImpl > after this function is called? > > Yes, that's how it works today, but I think we need it the other way around, so > that WebAudio can specify it wants all channels instead of just whatever the > hardware layout can handle. +rtoy for commentary. That's probably the right thing to do because some day we want an audio context to select the sample rate if possible (or internally resample if necessary). I don't think we should block this CL on this. If hongchan@ is happy that we know get all the channels available, I'm good with it.
On 2017/04/11 21:51:39, Raymond Toy wrote: > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > File media/blink/webaudiosourceprovider_impl.cc (right): > > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > media/blink/webaudiosourceprovider_impl.cc:259: info = > sink_->GetOutputDeviceInfo(); > On 2017/04/10 19:38:51, DaleCurtis wrote: > > On 2017/04/07 at 22:01:43, flim-chromium wrote: > > > On 2017/04/06 19:44:42, DaleCurtis wrote: > > > > Instead of returning the true sink parameters when WebAudio is connected, > I > > > > think you instead want to return some custom set of parameters from the > > WebAudio > > > > |client_|. I.e. whatever channel count its configured for along with the > > sample > > > > rate its currently using. > > > > > > It looks like the |client_| params are set only in > > WebAudioSourceProviderImpl::OnSetFormat, which is called from > AudioRendererImpl > > after this function is called? > > > > Yes, that's how it works today, but I think we need it the other way around, > so > > that WebAudio can specify it wants all channels instead of just whatever the > > hardware layout can handle. +rtoy for commentary. > > That's probably the right thing to do because some day we want an audio context > to select the sample rate if possible (or internally resample if necessary). > > I don't think we should block this CL on this. If hongchan@ is happy that we > know get all the channels available, I'm good with it. Yes, at this point what I want is being able to get all the channels available from the device by setting "discrete".
On 2017/04/13 18:01:53, hongchan wrote: > On 2017/04/11 21:51:39, Raymond Toy wrote: > > > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > > File media/blink/webaudiosourceprovider_impl.cc (right): > > > > > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > > media/blink/webaudiosourceprovider_impl.cc:259: info = > > sink_->GetOutputDeviceInfo(); > > On 2017/04/10 19:38:51, DaleCurtis wrote: > > > On 2017/04/07 at 22:01:43, flim-chromium wrote: > > > > On 2017/04/06 19:44:42, DaleCurtis wrote: > > > > > Instead of returning the true sink parameters when WebAudio is > connected, > > I > > > > > think you instead want to return some custom set of parameters from the > > > WebAudio > > > > > |client_|. I.e. whatever channel count its configured for along with the > > > sample > > > > > rate its currently using. > > > > > > > > It looks like the |client_| params are set only in > > > WebAudioSourceProviderImpl::OnSetFormat, which is called from > > AudioRendererImpl > > > after this function is called? > > > > > > Yes, that's how it works today, but I think we need it the other way > around, > > so > > > that WebAudio can specify it wants all channels instead of just whatever the > > > hardware layout can handle. +rtoy for commentary. > > > > That's probably the right thing to do because some day we want an audio > context > > to select the sample rate if possible (or internally resample if necessary). > > > > I don't think we should block this CL on this. If hongchan@ is happy that we > > know get all the channels available, I'm good with it. > > Yes, at this point what I want is being able to get all the channels available > from the device by setting "discrete". I had a look at third_party/WebKit but it's not clear where's the best place to set the number of channels in the client in HTMLMediaElement using e.g. the channels from ChannelSplitterNode. This also seems like something we'd want to do more generally than for just Opus ambisonics files. Would that still be within the scope of this CL? @hongchan, the behaviour of setting channelInterpretion=discrete is unchanged here, once this CL lands you'd also be able to access all Opus ambisonics channels (in the expected order).
On 2017/04/15 01:20:11, flim-chromium wrote: > On 2017/04/13 18:01:53, hongchan wrote: > > On 2017/04/11 21:51:39, Raymond Toy wrote: > > > > > > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > > > File media/blink/webaudiosourceprovider_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2752323002/diff/220001/media/blink/webaudioso... > > > media/blink/webaudiosourceprovider_impl.cc:259: info = > > > sink_->GetOutputDeviceInfo(); > > > On 2017/04/10 19:38:51, DaleCurtis wrote: > > > > On 2017/04/07 at 22:01:43, flim-chromium wrote: > > > > > On 2017/04/06 19:44:42, DaleCurtis wrote: > > > > > > Instead of returning the true sink parameters when WebAudio is > > connected, > > > I > > > > > > think you instead want to return some custom set of parameters from > the > > > > WebAudio > > > > > > |client_|. I.e. whatever channel count its configured for along with > the > > > > sample > > > > > > rate its currently using. > > > > > > > > > > It looks like the |client_| params are set only in > > > > WebAudioSourceProviderImpl::OnSetFormat, which is called from > > > AudioRendererImpl > > > > after this function is called? > > > > > > > > Yes, that's how it works today, but I think we need it the other way > > around, > > > so > > > > that WebAudio can specify it wants all channels instead of just whatever > the > > > > hardware layout can handle. +rtoy for commentary. > > > > > > That's probably the right thing to do because some day we want an audio > > context > > > to select the sample rate if possible (or internally resample if necessary). > > > > > > I don't think we should block this CL on this. If hongchan@ is happy that > we > > > know get all the channels available, I'm good with it. > > > > Yes, at this point what I want is being able to get all the channels available > > from the device by setting "discrete". > > I had a look at third_party/WebKit but it's not clear where's the best place to > set the number of channels in the client in HTMLMediaElement using e.g. the > channels from ChannelSplitterNode. This also seems like something we'd want to > do more generally than for just Opus ambisonics files. Would that still be > within the scope of this CL? > > @hongchan, the behaviour of setting channelInterpretion=discrete is unchanged > here, once this CL lands you'd also be able to access all Opus ambisonics > channels (in the expected order). First of all, I was confused by the goal of this CL. So flim@ explained the background for this change, and the following is my summary and what we can potentially do: 1. MediaElement does not support the multichannel stream that is > 8 channels. 2. Thus an HOA stream, which requires 18 channels, will not be played correctly by WebAudio. 3. This CL removes such limitation, so MediaElementSourceNode can take a multichannel stream up to 32 channels. I believe the confusion started from the assumption that WebAudio needs to know how many channels are coming from the media element. My answer is that WebAudio does not need to know that. This is because: When JS code creates a MediaElementSourceNode, the constructor calls SetFormat(channel_count) so WebAudio engine can arrange the channel setting of the node. Subsequently, the graph will dynamically be changed to adapt any channel count change along the way. This goes both ways: MediaElement does not need to know what WebAudio can or can't do, because WebAudio always assumes it as discrete channel layout. The element needs to send all the streams available to the MediaElementSourceNode. For the case of Ambisonics + non-diegetic audio, just calling SetFormat(channel_count) in the MESN constructor is good enough. On the other hand, the (web) application needs to know what type of media will be played and the WebAudio code should be prepared beforehand. Unfortunately, there is no magical way to workaround this problem, but I am also against the idea of taking care of channel mapping by magical communication between Blink and Renderer. It seems like a potential source of incompat issue between browsers. I rather expose a property like "mediaDescriptor" in MediaElement, so JS code can query the element to figure out "numberOfChannels" and "channelLayout" so the app can configure the WebAudio graph for its needs. However, this requires another spec work (potentially controversial) on MediaElement, and I doubt that it will happen in near future. dalecurtis@ WDYT?
On 2017/04/21 at 21:16:48, hongchan wrote: > On 2017/04/15 01:20:11, flim-chromium wrote: > > > > I had a look at third_party/WebKit but it's not clear where's the best place to > > set the number of channels in the client in HTMLMediaElement using e.g. the > > channels from ChannelSplitterNode. This also seems like something we'd want to > > do more generally than for just Opus ambisonics files. Would that still be > > within the scope of this CL? > > > > @hongchan, the behaviour of setting channelInterpretion=discrete is unchanged > > here, once this CL lands you'd also be able to access all Opus ambisonics > > channels (in the expected order). > > First of all, I was confused by the goal of this CL. So flim@ explained the background for this change, and the following is my summary and what we can potentially do: > > 1. MediaElement does not support the multichannel stream that is > 8 channels. > 2. Thus an HOA stream, which requires 18 channels, will not be played correctly by WebAudio. > 3. This CL removes such limitation, so MediaElementSourceNode can take a multichannel stream up to 32 channels. > > I believe the confusion started from the assumption that WebAudio needs to know how many channels are coming from the media element. My answer is that WebAudio does not need to know that. > > This is because: When JS code creates a MediaElementSourceNode, the constructor calls SetFormat(channel_count) so WebAudio engine can arrange the channel setting of the node. Subsequently, the graph will dynamically be changed to adapt any channel count change along the way. > > This goes both ways: MediaElement does not need to know what WebAudio can or can't do, because WebAudio always assumes it as discrete channel layout. The element needs to send all the streams available to the MediaElementSourceNode. For the case of Ambisonics + non-diegetic audio, just calling SetFormat(channel_count) in the MESN constructor is good enough. On the other hand, the (web) application needs to know what type of media will be played and the WebAudio code should be prepared beforehand. > > Unfortunately, there is no magical way to workaround this problem, but I am also against the idea of taking care of channel mapping by magical communication between Blink and Renderer. It seems like a potential source of incompat issue between browsers. I rather expose a property like "mediaDescriptor" in MediaElement, so JS code can query the element to figure out "numberOfChannels" and "channelLayout" so the app can configure the WebAudio graph for its needs. However, this requires another spec work (potentially controversial) on MediaElement, and I doubt that it will happen in near future. > > dalecurtis@ WDYT? I think the summary is accurate, but I believe it's a bit of an overstatement to call the communication magical. I think it's a reasonable expectation that the MediaElement would communicate the channel count to the WebAudio as the number of channels in the media. Long term we'd love to be able to seamlessly call Initialize() multiple times w/o introducing a gap in the audio; this would allow us to remove all sorts of complicated resampling and buffering code that we have. As a proxy to that for now, I think it's fine for us to add a AudioRendererSink::RenderCallback::SetPreferredChannelLayout(ch_layout, count) or even extend GetOutputDeviceInfo(AudioParameters& preferred_parameters) to take this into consideration. In the default no-WebAudio case we'll just ignore the provided values, but when WebAudio is attached we'll give the native sample rate, channel count, and layout without any rebuffering. Again this doesn't fix the adaptive case (stereo -> 5.1) or the HE-AAC (mono -> stereo) case with MSE; but would fix any mangling of single format audio streams.
On 2017/04/21 at 21:38:00, DaleCurtis wrote: > On 2017/04/21 at 21:16:48, hongchan wrote: > > On 2017/04/15 01:20:11, flim-chromium wrote: > > > > > > I had a look at third_party/WebKit but it's not clear where's the best place to > > > set the number of channels in the client in HTMLMediaElement using e.g. the > > > channels from ChannelSplitterNode. This also seems like something we'd want to > > > do more generally than for just Opus ambisonics files. Would that still be > > > within the scope of this CL? > > > > > > @hongchan, the behaviour of setting channelInterpretion=discrete is unchanged > > > here, once this CL lands you'd also be able to access all Opus ambisonics > > > channels (in the expected order). > > > > First of all, I was confused by the goal of this CL. So flim@ explained the background for this change, and the following is my summary and what we can potentially do: > > > > 1. MediaElement does not support the multichannel stream that is > 8 channels. > > 2. Thus an HOA stream, which requires 18 channels, will not be played correctly by WebAudio. > > 3. This CL removes such limitation, so MediaElementSourceNode can take a multichannel stream up to 32 channels. > > > > I believe the confusion started from the assumption that WebAudio needs to know how many channels are coming from the media element. My answer is that WebAudio does not need to know that. > > > > This is because: When JS code creates a MediaElementSourceNode, the constructor calls SetFormat(channel_count) so WebAudio engine can arrange the channel setting of the node. Subsequently, the graph will dynamically be changed to adapt any channel count change along the way. > > > > This goes both ways: MediaElement does not need to know what WebAudio can or can't do, because WebAudio always assumes it as discrete channel layout. The element needs to send all the streams available to the MediaElementSourceNode. For the case of Ambisonics + non-diegetic audio, just calling SetFormat(channel_count) in the MESN constructor is good enough. On the other hand, the (web) application needs to know what type of media will be played and the WebAudio code should be prepared beforehand. > > > > Unfortunately, there is no magical way to workaround this problem, but I am also against the idea of taking care of channel mapping by magical communication between Blink and Renderer. It seems like a potential source of incompat issue between browsers. I rather expose a property like "mediaDescriptor" in MediaElement, so JS code can query the element to figure out "numberOfChannels" and "channelLayout" so the app can configure the WebAudio graph for its needs. However, this requires another spec work (potentially controversial) on MediaElement, and I doubt that it will happen in near future. > > > > dalecurtis@ WDYT? > > I think the summary is accurate, but I believe it's a bit of an overstatement to call the communication magical. I think it's a reasonable expectation that the MediaElement would communicate the channel count to the WebAudio as the number of channels in the media. Long term we'd love to be able to seamlessly call Initialize() multiple times w/o introducing a gap in the audio; this would allow us to remove all sorts of complicated resampling and buffering code that we have. > > As a proxy to that for now, I think it's fine for us to add a AudioRendererSink::RenderCallback::SetPreferredChannelLayout(ch_layout, count) or even extend GetOutputDeviceInfo(AudioParameters& preferred_parameters) to take this into consideration. In the default no-WebAudio case we'll just ignore the provided values, but when WebAudio is attached we'll give the native sample rate, channel count, and layout without any rebuffering. Also I forgot to mention there's precedent for extending GetOutputDeviceInfo this way. See how we handle preferred parameters in AudioManager::GetPreferredOutputParameters(), etc. > > Again this doesn't fix the adaptive case (stereo -> 5.1) or the HE-AAC (mono -> stereo) case with MSE; but would fix any mangling of single format audio streams.
On 2017/04/21 21:39:31, DaleCurtis wrote:
> On 2017/04/21 at 21:38:00, DaleCurtis wrote:
> > On 2017/04/21 at 21:16:48, hongchan wrote:
> > > On 2017/04/15 01:20:11, flim-chromium wrote:
> > > >
> > > > I had a look at third_party/WebKit but it's not clear where's the best
> place to
> > > > set the number of channels in the client in HTMLMediaElement using e.g.
> the
> > > > channels from ChannelSplitterNode. This also seems like something we'd
> want to
> > > > do more generally than for just Opus ambisonics files. Would that still
be
> > > > within the scope of this CL?
> > > >
> > > > @hongchan, the behaviour of setting channelInterpretion=discrete is
> unchanged
> > > > here, once this CL lands you'd also be able to access all Opus
ambisonics
> > > > channels (in the expected order).
> > >
> > > First of all, I was confused by the goal of this CL. So flim@ explained
the
> background for this change, and the following is my summary and what we can
> potentially do:
> > >
> > > 1. MediaElement does not support the multichannel stream that is > 8
> channels.
> > > 2. Thus an HOA stream, which requires 18 channels, will not be played
> correctly by WebAudio.
> > > 3. This CL removes such limitation, so MediaElementSourceNode can take a
> multichannel stream up to 32 channels.
> > >
> > > I believe the confusion started from the assumption that WebAudio needs to
> know how many channels are coming from the media element. My answer is that
> WebAudio does not need to know that.
> > >
> > > This is because: When JS code creates a MediaElementSourceNode, the
> constructor calls SetFormat(channel_count) so WebAudio engine can arrange the
> channel setting of the node. Subsequently, the graph will dynamically be
changed
> to adapt any channel count change along the way.
> > >
> > > This goes both ways: MediaElement does not need to know what WebAudio can
or
> can't do, because WebAudio always assumes it as discrete channel layout. The
> element needs to send all the streams available to the MediaElementSourceNode.
> For the case of Ambisonics + non-diegetic audio, just calling
> SetFormat(channel_count) in the MESN constructor is good enough. On the other
> hand, the (web) application needs to know what type of media will be played
and
> the WebAudio code should be prepared beforehand.
> > >
> > > Unfortunately, there is no magical way to workaround this problem, but I
am
> also against the idea of taking care of channel mapping by magical
communication
> between Blink and Renderer. It seems like a potential source of incompat issue
> between browsers. I rather expose a property like "mediaDescriptor" in
> MediaElement, so JS code can query the element to figure out
"numberOfChannels"
> and "channelLayout" so the app can configure the WebAudio graph for its needs.
> However, this requires another spec work (potentially controversial) on
> MediaElement, and I doubt that it will happen in near future.
> > >
> > > dalecurtis@ WDYT?
> >
> > I think the summary is accurate, but I believe it's a bit of an
overstatement
> to call the communication magical. I think it's a reasonable expectation that
> the MediaElement would communicate the channel count to the WebAudio as the
> number of channels in the media. Long term we'd love to be able to seamlessly
> call Initialize() multiple times w/o introducing a gap in the audio; this
would
> allow us to remove all sorts of complicated resampling and buffering code that
> we have.
> >
> > As a proxy to that for now, I think it's fine for us to add a
> AudioRendererSink::RenderCallback::SetPreferredChannelLayout(ch_layout, count)
> or even extend GetOutputDeviceInfo(AudioParameters& preferred_parameters) to
> take this into consideration. In the default no-WebAudio case we'll just
ignore
> the provided values, but when WebAudio is attached we'll give the native
sample
> rate, channel count, and layout without any rebuffering.
>
> Also I forgot to mention there's precedent for extending GetOutputDeviceInfo
> this way. See how we handle preferred parameters in
> AudioManager::GetPreferredOutputParameters(), etc.
>
> >
> > Again this doesn't fix the adaptive case (stereo -> 5.1) or the HE-AAC (mono
> -> stereo) case with MSE; but would fix any mangling of single format audio
> streams.
If I understand correctly, this means we'd want to add in audio_renderer_sink.h
a virtual GetOutputDeviceInfo(AudioParameters& preferred_parameters). This would
be overridden in webaudiosourceproviderimpl.cc to return preferred_params if
client_ is attached, else sink_ params as normal. Other classes derived from
audio_renderer_sink would just override with
GetOutputDeviceInfo(AudioParameters& preferred_parameters) { return
GetOutputDeviceInfo(); }. Finally, in AudioRendererImpl::Initialize, we'd call
GetOutputDeviceInfo(preferred_params), passing in the params from
stream->audio_decoder_config()?
I'd just change the default signature to take an empty parameter set by default.
(i think that change should probably be done in a different Cl prior to landing this one; please add olka@ on it for her opinion too).
On 2017/04/24 20:41:57, DaleCurtis wrote: > (i think that change should probably be done in a different Cl prior to landing > this one; please add olka@ on it for her opinion too). OK, will create that separate CL. Thanks!
Patchset #15 (id:280001) has been deleted
On 2017/04/24 20:45:59, flim-chromium wrote: > On 2017/04/24 20:41:57, DaleCurtis wrote: > > (i think that change should probably be done in a different Cl prior to > landing > > this one; please add olka@ on it for her opinion too). > > OK, will create that separate CL. Thanks! Hi again, I've rebased to get the changes from the supporting CL (92836293002) and added support for decoding Opus channel mapping 2 (ambisonics content) in webm container. Could you please take another look? Thanks!
https://codereview.chromium.org/2752323002/diff/300001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2752323002/diff/300001/media/base/audio_decod... media/base/audio_decoder_config.h:66: void SetChannelsForDiscrete(int channels) { should be in .cc file. Should also DCHECK that is only called for discrete or equals the layout. https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:333: is_opus_discrete && codec_context->channels > 8 Did you ever figure out if this > 8 check could be dropped? Or does it not matter for your use case? https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:382: config->SetChannelsForDiscrete(codec_context->channels); Probably you don't want to always do this. channels from ffmpeg is not reliable. Only with layout == discrete should this be called I think. https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { These can all be kClockless type tests I think?
Thanks for looking! I'd also realized the check for webaudio in ARI wasn't narrow enough so added a fix in the latest patch. https://codereview.chromium.org/2752323002/diff/300001/media/base/audio_decod... File media/base/audio_decoder_config.h (right): https://codereview.chromium.org/2752323002/diff/300001/media/base/audio_decod... media/base/audio_decoder_config.h:66: void SetChannelsForDiscrete(int channels) { On 2017/05/24 00:49:58, DaleCurtis wrote: > should be in .cc file. Should also DCHECK that is only called for discrete or > equals the layout. Done. https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:333: is_opus_discrete && codec_context->channels > 8 On 2017/05/24 00:49:58, DaleCurtis wrote: > Did you ever figure out if this > 8 check could be dropped? Or does it not > matter for your use case? I'm not familiar enough with the potential problems (e.g. in OSX that you pointed out before) to drop this. Anyway, I think it's actually useful to keep this such that for <= 8ch, the default up/downmixing is applied if a webaudio renderer isn't attached. It wouldn't sound spatially correct but that's to be expected, and at least we'd get to preview the audio content. Potentially a future CL could look at some saner up/downmixing matrices, and also for >8 ch ambisonics. wdyt? https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:382: config->SetChannelsForDiscrete(codec_context->channels); On 2017/05/24 00:49:58, DaleCurtis wrote: > Probably you don't want to always do this. channels from ffmpeg is not reliable. > Only with layout == discrete should this be called I think. Right, I ran into this problem with the ffmpeg tests, hence the change in SetChannelsForDiscrete. Your way makes sense, I've changed it back now. https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/24 00:49:58, DaleCurtis wrote: > These can all be kClockless type tests I think? We might want to use with audio controls that enable multiple play/pause and I think this wouldn't be covered by the kClockless tests?
https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:333: is_opus_discrete && codec_context->channels > 8 On 2017/05/25 at 00:04:25, flim-chromium wrote: > On 2017/05/24 00:49:58, DaleCurtis wrote: > > Did you ever figure out if this > 8 check could be dropped? Or does it not > > matter for your use case? > > I'm not familiar enough with the potential problems (e.g. in OSX that you pointed out before) to drop this. > > Anyway, I think it's actually useful to keep this such that for <= 8ch, the default up/downmixing is applied if a webaudio renderer isn't attached. It wouldn't sound spatially correct but that's to be expected, and at least we'd get to preview the audio content. Potentially a future CL could look at some saner up/downmixing matrices, and also for >8 ch ambisonics. wdyt? Leaving sgtm for now, though I think WebAudio doesn't want channel mixing, so if we could always skip it when WebAudio is connected that is fine. https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/25 at 00:04:26, flim-chromium wrote: > On 2017/05/24 00:49:58, DaleCurtis wrote: > > These can all be kClockless type tests I think? > > We might want to use with audio controls that enable multiple play/pause and I think this wouldn't be covered by the kClockless tests? Hmmm? I don't understand what you're asking. I'm just saying change this to Start("", kClockless); https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == Why do we need this? I think what you had before is correct. I.e. never do conversion if WebAudio is attached?
https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/25 00:29:16, DaleCurtis wrote: > On 2017/05/25 at 00:04:26, flim-chromium wrote: > > On 2017/05/24 00:49:58, DaleCurtis wrote: > > > These can all be kClockless type tests I think? > > > > We might want to use with audio controls that enable multiple play/pause and I > think this wouldn't be covered by the kClockless tests? > > Hmmm? I don't understand what you're asking. I'm just saying change this to > Start("", kClockless); Oh, I thought that'd use ClocklessAudioSink and maybe I understood the comment in https://cs.chromium.org/chromium/src/media/audio/clockless_audio_sink.h?type=... https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == On 2017/05/25 00:29:16, DaleCurtis wrote: > Why do we need this? I think what you had before is correct. I.e. never do > conversion if WebAudio is attached? Yep, that's still there in line 376. But if webaudio isn't attached, then using the stream params will lead to an error at audio_converter.cc(35) (has DISCRETE layout but 0 channels)
https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/25 at 00:46:06, flim-chromium wrote: > On 2017/05/25 00:29:16, DaleCurtis wrote: > > On 2017/05/25 at 00:04:26, flim-chromium wrote: > > > On 2017/05/24 00:49:58, DaleCurtis wrote: > > > > These can all be kClockless type tests I think? > > > > > > We might want to use with audio controls that enable multiple play/pause and I > > think this wouldn't be covered by the kClockless tests? > > > > Hmmm? I don't understand what you're asking. I'm just saying change this to > > Start("", kClockless); > > Oh, I thought that'd use ClocklessAudioSink and maybe I understood the comment in > https://cs.chromium.org/chromium/src/media/audio/clockless_audio_sink.h?type=... That will be used, but until these tests actually do something else they should be clockless. I doubt you'll want to do much more here. https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == On 2017/05/25 at 00:46:06, flim-chromium wrote: > On 2017/05/25 00:29:16, DaleCurtis wrote: > > Why do we need this? I think what you had before is correct. I.e. never do > > conversion if WebAudio is attached? > > Yep, that's still there in line 376. But if webaudio isn't attached, then using the stream params will lead to an error at audio_converter.cc(35) (has DISCRETE layout but 0 channels) Seems like you could just drop this and always call set_channels_for_discrete() on parameters then? The channels() value should be mapped to the layout already if not discrete?
https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/25 00:53:38, DaleCurtis wrote: > On 2017/05/25 at 00:46:06, flim-chromium wrote: > > On 2017/05/25 00:29:16, DaleCurtis wrote: > > > On 2017/05/25 at 00:04:26, flim-chromium wrote: > > > > On 2017/05/24 00:49:58, DaleCurtis wrote: > > > > > These can all be kClockless type tests I think? > > > > > > > > We might want to use with audio controls that enable multiple play/pause > and I > > > think this wouldn't be covered by the kClockless tests? > > > > > > Hmmm? I don't understand what you're asking. I'm just saying change this to > > > Start("", kClockless); > > > > Oh, I thought that'd use ClocklessAudioSink and maybe I understood the comment > in > > > https://cs.chromium.org/chromium/src/media/audio/clockless_audio_sink.h?type=... > > That will be used, but until these tests actually do something else they should > be clockless. I doubt you'll want to do much more here. Got it, will upload this fix later with any changes needed for the other comment. https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == On 2017/05/25 00:53:38, DaleCurtis wrote: > On 2017/05/25 at 00:46:06, flim-chromium wrote: > > On 2017/05/25 00:29:16, DaleCurtis wrote: > > > Why do we need this? I think what you had before is correct. I.e. never do > > > conversion if WebAudio is attached? > > > > Yep, that's still there in line 376. But if webaudio isn't attached, then > using the stream params will lead to an error at audio_converter.cc(35) (has > DISCRETE layout but 0 channels) > > Seems like you could just drop this and always call set_channels_for_discrete() > on parameters then? We'd have to set the channels in AudioRendererMixerManager, AudioManagerPulse and PulseAudioOutputStream (and maybe ALSA too). The html audio control would then play but there is no sound. Not sure if this would be preferable? > The channels() value should be mapped to the layout already > if not discrete? Sorry, I didn't follow - this is for the discrete layout case only?
https://codereview.chromium.org/2752323002/diff/320001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:333: is_opus_discrete && codec_context->channels > 8 Add a note about why we have > 8 here; perhaps with a TODO in case we want to support this in the future. https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == On 2017/05/25 at 02:16:41, flim-chromium wrote: > On 2017/05/25 00:53:38, DaleCurtis wrote: > > On 2017/05/25 at 00:46:06, flim-chromium wrote: > > > On 2017/05/25 00:29:16, DaleCurtis wrote: > > > > Why do we need this? I think what you had before is correct. I.e. never do > > > > conversion if WebAudio is attached? > > > > > > Yep, that's still there in line 376. But if webaudio isn't attached, then > > using the stream params will lead to an error at audio_converter.cc(35) (has > > DISCRETE layout but 0 channels) > > > > Seems like you could just drop this and always call set_channels_for_discrete() > > on parameters then? > > We'd have to set the channels in AudioRendererMixerManager, AudioManagerPulse and PulseAudioOutputStream (and maybe ALSA too). The html audio control would then play but there is no sound. Not sure if this would be preferable? > > > The channels() value should be mapped to the layout already > > if not discrete? > Sorry, I didn't follow - this is for the discrete layout case only? Hmm, I was proposing a simplified logic, but after looking again I don't think a simpler form exists. (!a || !b || c || !d) && !(e && d) is the minimal form. So this is okay, just add a {} around the conditional here.
lgtm % those fixes.
Thanks for your patient review! https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/2752323002/diff/300001/media/test/pipeline_in... media/test/pipeline_integration_test.cc:569: TEST_F(PipelineIntegrationTest, BasicPlaybackOpusOgg_4ch_ChannelMapping2) { On 2017/05/25 02:16:41, flim-chromium wrote: > On 2017/05/25 00:53:38, DaleCurtis wrote: > > On 2017/05/25 at 00:46:06, flim-chromium wrote: > > > On 2017/05/25 00:29:16, DaleCurtis wrote: > > > > On 2017/05/25 at 00:04:26, flim-chromium wrote: > > > > > On 2017/05/24 00:49:58, DaleCurtis wrote: > > > > > > These can all be kClockless type tests I think? > > > > > > > > > > We might want to use with audio controls that enable multiple play/pause > > and I > > > > think this wouldn't be covered by the kClockless tests? > > > > > > > > Hmmm? I don't understand what you're asking. I'm just saying change this > to > > > > Start("", kClockless); > > > > > > Oh, I thought that'd use ClocklessAudioSink and maybe I understood the > comment > > in > > > > > > https://cs.chromium.org/chromium/src/media/audio/clockless_audio_sink.h?type=... > > > > That will be used, but until these tests actually do something else they > should > > be clockless. I doubt you'll want to do much more here. > > Got it, will upload this fix later with any changes needed for the other > comment. Done. https://codereview.chromium.org/2752323002/diff/320001/media/ffmpeg/ffmpeg_co... File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/ffmpeg/ffmpeg_co... media/ffmpeg/ffmpeg_common.cc:333: is_opus_discrete && codec_context->channels > 8 On 2017/05/25 18:51:01, DaleCurtis wrote: > Add a note about why we have > 8 here; perhaps with a TODO in case we want to > support this in the future. Done. https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... File media/renderers/audio_renderer_impl.cc (right): https://codereview.chromium.org/2752323002/diff/320001/media/renderers/audio_... media/renderers/audio_renderer_impl.cc:378: if (stream->audio_decoder_config().channel_layout() == On 2017/05/25 18:51:01, DaleCurtis wrote: > On 2017/05/25 at 02:16:41, flim-chromium wrote: > > On 2017/05/25 00:53:38, DaleCurtis wrote: > > > On 2017/05/25 at 00:46:06, flim-chromium wrote: > > > > On 2017/05/25 00:29:16, DaleCurtis wrote: > > > > > Why do we need this? I think what you had before is correct. I.e. never > do > > > > > conversion if WebAudio is attached? > > > > > > > > Yep, that's still there in line 376. But if webaudio isn't attached, then > > > using the stream params will lead to an error at audio_converter.cc(35) (has > > > DISCRETE layout but 0 channels) > > > > > > Seems like you could just drop this and always call > set_channels_for_discrete() > > > on parameters then? > > > > We'd have to set the channels in AudioRendererMixerManager, AudioManagerPulse > and PulseAudioOutputStream (and maybe ALSA too). The html audio control would > then play but there is no sound. Not sure if this would be preferable? > > > > > The channels() value should be mapped to the layout already > > > if not discrete? > > Sorry, I didn't follow - this is for the discrete layout case only? > > Hmm, I was proposing a simplified logic, but after looking again I don't think a > simpler form exists. (!a || !b || c || !d) && !(e && d) is the minimal form. So > this is okay, just add a {} around the conditional here. Done.
The CQ bit was checked by flim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2752323002/#ps360001 (title: "rebase to get updated DEPS from CL 2901383003")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by flim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry Dale, one more small change in ffmpeg_audio_decoder.cc to fix the case when there is a mismatch between the decoded channels and channel_layout, if it looks good I'll land it.
lgtm
The CQ bit was checked by flim@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1495817423883300,
"parent_rev": "fc4e84a80b729297c446bc1bf3b0fce67ecfd65f", "commit_rev":
"162236743c74fa040e822ea4d846215e4543791e"}
Message was sent while issue was closed.
Description was changed from ========== Support Opus Ambisonics playback Opus Ambisonics does not have a prescribed layout, so CHANNEL_LAYOUT_DISCRETE is used. In this case, the audio decoders and renderer also need to know the number of channels. BUG=693745 ========== to ========== Support Opus Ambisonics playback Opus Ambisonics does not have a prescribed layout, so CHANNEL_LAYOUT_DISCRETE is used. In this case, the audio decoders and renderer also need to know the number of channels. BUG=693745 Review-Url: https://codereview.chromium.org/2752323002 Cr-Commit-Position: refs/heads/master@{#475035} Committed: https://chromium.googlesource.com/chromium/src/+/162236743c74fa040e822ea4d846... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001) as https://chromium.googlesource.com/chromium/src/+/162236743c74fa040e822ea4d846... |
