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)
Core
Audio/Video: Playback
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 | ||
Comment 1•7 years ago
|
||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8824326 -
Attachment is obsolete: true
Attachment #8824326 -
Flags: feedback?(jyavenard)
Attachment #8828198 -
Flags: feedback?(jyavenard)
Comment 5•7 years ago
|
||
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-
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-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 ::: 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+
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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 13•7 years ago
|
||
mozreview-review-reply |
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 :)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-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 ::: 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+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
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 21•7 years ago
|
||
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 22•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 26•7 years ago
|
||
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
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/401683745308 https://hg.mozilla.org/mozilla-central/rev/0cdf5249e6d0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 28•7 years ago
|
||
Chunmin, Good job!
Comment 29•7 years ago
|
||
Noted: https://developer.mozilla.org/en-US/Firefox/Releases/54
Keywords: dev-doc-needed
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Type: defect → enhancement
You need to log in
before you can comment on or make changes to this bug.
Description
•