Closed Bug 1321502 Opened 7 years ago Closed 7 years ago

Support audio 5.1 on Windows(playback)

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 2 obsolete files)

      No description provided.
Assignee: nobody → cchang
Blocks: 1300455
Depends on: 1300018
Hi jya,
I was wondering if you could give me some suggestions about moving up/downmixing from gecko to cubeb.
Attachment #8818814 - Attachment is obsolete: true
Attachment #8824326 - Flags: feedback?(jyavenard)
Attachment #8824326 - Attachment is obsolete: true
Attachment #8824326 - Flags: feedback?(jyavenard)
Attachment #8828198 - Flags: feedback?(jyavenard)
Comment on attachment 8828198 [details] [diff] [review]
Part2: Bypass up/downmix when no MonoAudio or AudioSinkForceStereo is specified

Review of attachment 8828198 [details] [diff] [review]:
-----------------------------------------------------------------

any chance you can use ReviewBoard instead?

makes it impossible to r+ while asking for changes to be made (and enforced)

::: dom/media/AudioConverter.cpp
@@ +65,5 @@
>  
>  size_t
>  AudioConverter::ProcessInternal(void* aOut, const void* aIn, size_t aFrames)
>  {
> +  if (MediaPrefs::MonoAudio() || MediaPrefs::AudioSinkForceStereo()) {

It is not the job of the AudioConverter to check about prefs and determine what to convert to.

This is the responsibility of the AudioConverter's caller.

Please remove

::: dom/media/mediasink/DecodedAudioDataSink.cpp
@@ +69,5 @@
>    }
>    MOZ_DIAGNOSTIC_ASSERT(mOutputRate, "output rate can't be 0.");
>  
> +  mOutputChannels = MediaPrefs::MonoAudio() ?
> +    1 : (MediaPrefs::AudioSinkForceStereo() ? 2 : mInfo.mChannels);

this isn't per coding standard

  mOutputChannels =
    MediaPrefs::MonoAudio()
      ? 1
      : (MediaPrefs::AudioSinkForceStereo() ? 2 : mInfo.mChannels);
Attachment #8828198 - Flags: feedback?(jyavenard) → feedback-
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Comment on attachment 8828198 [details] [diff] [review]
> Part2: Bypass up/downmix when no MonoAudio or AudioSinkForceStereo is
> specified
> 
> Review of attachment 8828198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> any chance you can use ReviewBoard instead?
I think reviewboard doesn't provide feedback service. 
but anyway, I'll use reviewboard next time.

> makes it impossible to r+ while asking for changes to be made (and enforced)
> 
> ::: dom/media/AudioConverter.cpp
> @@ +65,5 @@
> >  
> >  size_t
> >  AudioConverter::ProcessInternal(void* aOut, const void* aIn, size_t aFrames)
> >  {
> > +  if (MediaPrefs::MonoAudio() || MediaPrefs::AudioSinkForceStereo()) {
> 
> It is not the job of the AudioConverter to check about prefs and determine
> what to convert to.
> 
> This is the responsibility of the AudioConverter's caller.
> 
> Please remove
Instead of using pref, is it ok to use channel count to determine whether we should use up/downmix?

> ::: dom/media/mediasink/DecodedAudioDataSink.cpp
> @@ +69,5 @@
> >    }
> >    MOZ_DIAGNOSTIC_ASSERT(mOutputRate, "output rate can't be 0.");
> >  
> > +  mOutputChannels = MediaPrefs::MonoAudio() ?
> > +    1 : (MediaPrefs::AudioSinkForceStereo() ? 2 : mInfo.mChannels);
> 
> this isn't per coding standard
> 
>   mOutputChannels =
>     MediaPrefs::MonoAudio()
>       ? 1
>       : (MediaPrefs::AudioSinkForceStereo() ? 2 : mInfo.mChannels);
OK.
(In reply to Chun-Min Chang[:chunmin] from comment #6)
> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > Comment on attachment 8828198 [details] [diff] [review]
> > Part2: Bypass up/downmix when no MonoAudio or AudioSinkForceStereo is
> > specified
> > 
> > Review of attachment 8828198 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > any chance you can use ReviewBoard instead?
> I think reviewboard doesn't provide feedback service. 
> but anyway, I'll use reviewboard next time.
> 
> > makes it impossible to r+ while asking for changes to be made (and enforced)
> > 
> > ::: dom/media/AudioConverter.cpp
> > @@ +65,5 @@
> > >  
> > >  size_t
> > >  AudioConverter::ProcessInternal(void* aOut, const void* aIn, size_t aFrames)
> > >  {
> > > +  if (MediaPrefs::MonoAudio() || MediaPrefs::AudioSinkForceStereo()) {
> > 
> > It is not the job of the AudioConverter to check about prefs and determine
> > what to convert to.
> > 
> > This is the responsibility of the AudioConverter's caller.
> > 
> > Please remove
> Instead of using pref, is it ok to use channel count to determine whether we
> should use up/downmix?

Not sure I understand what you mean.

This is already what the AudioConverter is doing...
https://dxr.mozilla.org/mozilla-central/source/dom/media/AudioConverter.cpp#68

When you create the AudioConverter, you give it an channel layout on entry, and one on output. If you don't want the AudioConverter to remux the channels, provide the same layout on entry and exit. Then all left to do for the AudioConverter is resampling.

Though you still need to manage content that changes layout half-way. I don't believe cubeb can handle that.

Ultimately, there should be no need for the AudioConverter at all, cubeb should be able to get data that change content on the fly.

AudioConverter is only a workaround cubeb API limitations. Hopefully we can remove those in the near future
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Not sure I understand what you mean.
Thanks for your reply. I think I made a mistake. Let me summarize the flow here. Please correct me if there is anything wrong.

The AudioConverter is used to convert or mix the data by layouts or channel counts, since cubeb doesn't support multiple channel before. Since we now provide multiple channel support and up/downmixing in cubeb on Windows, it should bypass the mixing part in Gecko. However, if there is a pref to force audio data to be stereo or mono, it still need up/downmixing before passing audio into cubeb because cubeb doesn't provide this feature. That is, the up/downmixing in AudioConverter only works when |MediaPrefs::MonoAudio()| or |MediaPrefs::AudioSinkForceStereo()| is set to True. 

In such case, the input channel counts of AudioConverter is different from its output channel counts, so the up/downmix will be called. Thus, we don't need to do anything in AudioConverter. If there is no force pref for mono or stereo, then AudioConverter won't remix the audio data.
The audio converter constructor takes an audio config for the input and an audio config for the output.

It is in those configuration that you specify what you want.

You do not have the audio converter check on preferences to modify how and what conversion will be performed.

So if on Windows, you don't want to perform up or downmixing, then you specify on output the same channel layout as you get on input. It is whatever is constructing the AudioConverter that takes care of the preferences. *NOT* in the AudioConverter itself.
Comment on attachment 8829811 [details]
Bug 1321502 - part 1: Enable multi-channel support in Gecko on Windows;

https://reviewboard.mozilla.org/r/106802/#review107956

::: dom/media/mediasink/DecodedAudioDataSink.cpp:200
(Diff revision 1)
>  DecodedAudioDataSink::InitializeAudioStream(const PlaybackParams& aParams)
>  {
>    mAudioStream = new AudioStream(*this);
> -  nsresult rv = mAudioStream->Init(mOutputChannels, mOutputRate, mChannel);
> +  // The layout map used here is already processed by mConverter with
> +  // mOutputChannels into SMPTE format, so there is no need to worry if
> +  // MediaPrefs::MonoAudio() or MediaPrefs::AudioSinkForceStereo() is applied.

Make sure that the use of MonoAudio preference isn't lost.

This is for people with an auditive impairement (e.g. they can't hear from one ear); to ensure that they don't lose information.
Attachment #8829811 - Flags: review?(jyavenard) → review+
Comment on attachment 8829811 [details]
Bug 1321502 - part 1: Enable multi-channel support in Gecko on Windows;

https://reviewboard.mozilla.org/r/106802/#review107956

> Make sure that the use of MonoAudio preference isn't lost.
> 
> This is for people with an auditive impairement (e.g. they can't hear from one ear); to ensure that they don't lose information.

Thanks for your review!

Should I add an assert for checking ```MediaPrefs::MonoAudio()``` and ```Layout().Map()``` here? Actually, we will make sure the layout matches the channel count in [cubeb](http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/media/libcubeb/src/cubeb_wasapi.cpp#1793).
Comment on attachment 8829811 [details]
Bug 1321502 - part 1: Enable multi-channel support in Gecko on Windows;

https://reviewboard.mozilla.org/r/106802/#review107956

> Thanks for your review!
> 
> Should I add an assert for checking ```MediaPrefs::MonoAudio()``` and ```Layout().Map()``` here? Actually, we will make sure the layout matches the channel count in [cubeb](http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/media/libcubeb/src/cubeb_wasapi.cpp#1793).

no I think that's fine..

so long as you're aware of it :)
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

https://reviewboard.mozilla.org/r/107390/#review108776

::: dom/media/mediasink/DecodedAudioDataSink.cpp:210
(Diff revision 2)
> +    channelMap = mConverter->OutputConfig().Layout().Map();
> +  }
> +  MOZ_ASSERT(channelMap);
> +
>    // The layout map used here is already processed by mConverter with
>    // mOutputChannels into SMPTE format, so there is no need to worry if

That's not entirely correct per say.

AudioConvert takes a channel layout, which may not have to be SMPTE.

It just happens to only be used with SMPTE currently
Attachment #8830654 - Flags: review?(jyavenard) → review+
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

https://reviewboard.mozilla.org/r/107390/#review108776

> That's not entirely correct per say.
> 
> AudioConvert takes a channel layout, which may not have to be SMPTE.
> 
> It just happens to only be used with SMPTE currently

Is there other way to get channel layout when there is no data in AudioQueue? 

AFAIK, the channel layout is converted into SMPTE in [NotifyAudioNeeded()](http://searchfox.org/mozilla-central/source/dom/media/mediasink/DecodedAudioDataSink.cpp#476) if the input layout is different from the output one. However, when AudioQueue is empty, we still need to initialize cubeb to play(test_SeekToEnd_mp4.html for bug 1297036). If there is no input audio data, then there is no way to know what's the used input layout.

In such case, we could consider what we should do when user only know its audio channel counts but without knowledge of its layout. 

Will it be better if we request cubeb's preferred layout by channel counts instead of using SMPTE directly for ```AudioStream::Init```?

We could add a function named ```CubebUtils::PreferredChannelLayout(uint32_t aChannels)``` to call [```cubeb_get_preferred_channel_layout```](http://searchfox.org/mozilla-central/source/media/libcubeb/include/cubeb.h#467). If its returned layout has different channel counts from ```aChannels```, we do some non-matched handling there.

What do you think?
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

Hi jya,
Sorry to bother you again. I was wondering if you could take a look for this minor changes. When user only know the audio's channel counts but have no knowledge about its layout, we guess one matched layout for it. Instead of using SMPTE's defualt setting, we could ask device's preferred layout first. If the preferred layout matches the channel counts, then apply it. Otherwise, we fallback to use the SMPTE's default layout as previous patch.
Attachment #8830654 - Flags: review+ → review?(jyavenard)
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

this was already r+ in reviewboard
Attachment #8830654 - Flags: review?(jyavenard) → review+
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

https://reviewboard.mozilla.org/r/107390/#review110540

::: dom/media/CubebUtils.cpp:236
(Diff revision 3)
> +  if (!context) {
> +    return false;
> +  }
> +  return cubeb_get_preferred_channel_layout(context,
> +                                            &sPreferredChannelLayout) == CUBEB_OK
> +    ? true : false;

? should be aligned with cubeb_get_preferred_channel_layout

::: dom/media/CubebUtils.cpp:269
(Diff revision 3)
> +  };
> +
> +  // Use SMPTE default channel map if we can't get preferred layout
> +  // or the channel counts of preferred layout is different from input's one
> +  if (!InitPreferredChannelLayout() ||
> +      channelCounts[sPreferredChannelLayout][0] != aChannels) {

|| aligned with InitPreferredChannelLayout and on the following line:

if (!InitPreferredChannelLayout()
    || channelCounts[...

::: dom/media/mediasink/DecodedAudioDataSink.cpp:200
(Diff revision 3)
>  DecodedAudioDataSink::InitializeAudioStream(const PlaybackParams& aParams)
>  {
>    mAudioStream = new AudioStream(*this);
> +  // When AudioQueue is empty, there is no way to know the channel layout of
> +  // the coming audio data, so we use the predefined channel map instead.
> +  uint32_t channelMap = mConverter ? mConverter->OutputConfig().Layout().Map():

: underneath ?
or

like so:
uint32_t channelMap = mConverter 
                      ? mConverter->OutputConfig().Layout().Map()
                      : AudioStream..
Comment on attachment 8830654 [details]
Bug 1321502 - part 2: Use preferred layout for initializing cubeb when audio queue is empty;

https://reviewboard.mozilla.org/r/107390/#review110540

Thanks for your help!

> ? should be aligned with cubeb_get_preferred_channel_layout

OK.

> || aligned with InitPreferredChannelLayout and on the following line:
> 
> if (!InitPreferredChannelLayout()
>     || channelCounts[...

OK.

> : underneath ?
> or
> 
> like so:
> uint32_t channelMap = mConverter 
>                       ? mConverter->OutputConfig().Layout().Map()
>                       : AudioStream..

Fixed.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/401683745308
part 1: Enable multi-channel support in Gecko on Windows; r=jya
https://hg.mozilla.org/integration/autoland/rev/0cdf5249e6d0
part 2: Use preferred layout for initializing cubeb when audio queue is empty; r=jya
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/401683745308
https://hg.mozilla.org/mozilla-central/rev/0cdf5249e6d0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Chunmin, 
Good job!
See Also: → 1337805
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.