911482 - Move DownmixToStereo functionality into AudioData class

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alex Chronopoulos [:achronop]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130807125214

Steps to reproduce:

This bug is used for tracking the transfer of downmix functionality from OggReader class to AudioData class. 


Actual results:

Currently, the downmix functionality, is available for Ogg stream only and performed by function OggReader::DownmixToStereo. 


Expected results:

By moving to AudioData class, downmix functionality, will be available to every audio stream.

Comment 1

[Alex Chronopoulos [:achronop]]

Ralph, can you please confirm the bug and assign it to me? Thanks!

Comment 2

[Alex Chronopoulos [:achronop]]

In "AudioData" class, the member "mChannels" is const. By executing downmixing inside the class, we need to modify the mChannels, so the const should be removed. Is that acceptable?

Comment 3

[Ralph Giles (:rillian)]

I think this is worth doing. Eventually we want to plumb surround through to cubeb, but this is good practice and make the current downmix code available to other decoders.

Chris, you ok with this?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Moving the downmix code into either AudioData, or into a stand alone function that lives in VideoUtils.cpp sounds reasonable to me.

I think kinetik or padenot should answer this one, they're the audio experts.

Comment 5

[Matthew Gregan [:kinetik]]

Ideally, libcubeb would provide the downmixing when the audio is output.  That functionality is lacking because nobody has had time to implement it yet.

But as a smaller step of sharing the existing downmix code, that's a useful improvement to have in the meantime.

Comment 6

[Paul Adenot (:padenot)]

Yes, this would be faster than writing the code for all the libcubeb backends indeed. Hopefully we can then move the code to cubeb (for backends that need manual downmixing, such as WASAPI) without too much trouble, when we want to support surround. For this reason, I'd prefer this to be written using pure functions that are not using fancy C++ features.

Comment 7

[Alex Chronopoulos [:achronop]]

Thank you all for your response. I will remove the const declaration from AudioData::mChannels. You can always re-think it on review time.

I can try to import the downmix functionality to libcubeb as well, if you will.

Comment 8

[Ralph Giles (:rillian)]

(In reply to Alexandros Chronopoulos [:achronop] from comment #2)
> In "AudioData" class, the member "mChannels" is const. By executing
> downmixing inside the class, we need to modify the mChannels, so the const
> should be removed. Is that acceptable?

I think this depends on the call pattern. If the idea is that there's a method the cubeb driver would call to downmix AudioData objects as it pulls them out of the queue, then it makes sense for mChannels to be mutable. If you want to keep mChannels const you could instead have a 'downmix constructor' which makes new a AudioData object. I'd pick lower overhead instead of safety here, but I don't feel strongly.

But looking at the code, cpearce's suggestion of a stand-alone function in VideoUtils.cpp operating on float buffers makes more sense than an AudioData method. I like the AudioQueue design, but it seems to terminate in MediaDecoderStateMachine::SendStreamAudio, which unpacks the data and copies it into an AudioStream. Unfortunately that's too soon to know whether we need to downmix or not.

If you want to go the method route you'd need to work on AudioStream instead, which seems to use buffers without object encapsulation. Or you could try unifying the MediaDecoder's AudioData with webaudio's AudioChunk (but that has different downmix matricies!) and plumb that through into AudioStream so it's used at playback time. Sounds like a lot of work for a temporary step.

Comment 9

[Alex Chronopoulos [:achronop]]

Attachment: move DownmixToStereo in AudioData class

This is the original idea tested successfully. The mChannel member of AudioData changed to mutable. I will continue investigating the AudioStream approach with stand alone function in VideoUtil.cpp. Thanks!

Comment 10

[Alex Chronopoulos [:achronop]]

Attachment: DownmixToStereo as VideoUtil function

Sorry for my slow response but it took me a while to learn the ropes inside the Audio thread. The challenge, downmixing in there, is that you need to keep both the input and output channels and use the correct each time.

The downmixing function has been moved in VideoUtil as independent function. The call happen inside the AudioStream::Write() before the audio data is written to the file. My opinion is to create a separate AudioSteam method (wrapper of the VideoUtil method) to perform the downmixing, in order to make the procedure more obvious.

This is an initial effort to see if we are in the right path. Feel free to purpose any change.
Tested successfully on desktop and mobile. The following days I will run the whole test suite.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 816374 [details] [diff] [review]
DownmixToStereo as VideoUtil function

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

Hi achronop. Was nice to meet you in Brussels. Sorry I took so long to get back to you on this.

Looks fine to me. Please address the mChannels > 8 case and resubmit.

::: content/media/AudioStream.cpp
@@ +608,5 @@
>      "Stream write in unexpected state.");
>  
> +  // Downmix
> +  if (mChannels > 2)
> +    if (mChannels <= 8)

please combine these if they have the same body:

if (mChannels > 2 && mChannels <= 8) {
  DownmixAudioToStereo();
}

What happens if mChannels > 8?

Comment 12

[Alex Chronopoulos [:achronop]]

Attachment: Apply review comments

Hi rillian. It was great meeting you in Brussels! It was a great summit in general! 

Thank you for your comments. I have improved the if statement and I made the Write() abort for mChannels>8. For Vorbis and Opus the latter will never take effect since the check happen at the decoder. I have built and tested successfully on desktop. I will push to try as well. Cheers!

Comment 13

[Alex Chronopoulos [:achronop]]

Try progress:

https://tbpl.mozilla.org/?tree=Try&rev=1cabb798dfa6

Comment 14

[Alex Chronopoulos [:achronop]]

I missed some warnings in my code and try server is not so tolerant.

New push to try without warnings:
https://tbpl.mozilla.org/?tree=Try&rev=4def24cbf6c7
Thanks!

Comment 15

[Alex Chronopoulos [:achronop]]

Attachment: Updated patch without try errors.

The same with previous one apart of a reorder in the constructor's member declaration to correct try errors. Thanks!

Comment 16

[Ralph Giles (:rillian)]

Comment on attachment 8333580 [details] [diff] [review]
Updated patch without try errors.

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

r=me. Thanks!

::: content/media/VideoUtils.h
@@ +151,5 @@
> +// Downmix multichannel Audio samples to Stereo.
> +// Input are the buffer contains multichannel data,
> +// the number of channels and the number of frames.
> +int DownmixAudioToStereo(mozilla::AudioDataValue* buffer,
> +                         int channel,

int channels

Comment 17

[Alex Chronopoulos [:achronop]]

Attachment: Updated with comments and Bug 938686 change

Thanks for the comments. I updated the parameter name in VideoUtils.h file and updated the WebMReader.cpp after the changes of Bug 938686. I set it for re-review just in case. Built and tested successfully.

Comment 18

[Alex Chronopoulos [:achronop]]

Try link:
https://tbpl.mozilla.org/?tree=Try&rev=37b3b7c98242

Comment 19

[Ralph Giles (:rillian)]

Comment on attachment 8342590 [details] [diff] [review]
Updated with comments and Bug 938686 change

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

Sorry it took so long to get back to you.

Comment 20

[Ralph Giles (:rillian)]

BTW, I think we should hold off landing this until Tuesday. We've changed a bunch of thinks with the VP9 and Opus updates and I'd like to keep things stable until after the Firefox 28 branch point.

Comment 21

[Alex Chronopoulos [:achronop]]

Thank you for the r+. I rebuilt and everything looks fine so I set it ready for check-in.
https://tbpl.mozilla.org/?tree=Try&rev=3f01e018433f

Something I am thinking about this patch is that the code of Bug 899050 will never run again, since the output audio will be downmixed to stereo in every case. Maybe we need to transfer part of the downmix decision/logic inside the driver which knows better the platforms facilities.

Thanks!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41fc2afc8465

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41fc2afc8465

Comment 24

[Matthew Gregan [:kinetik]]

diff --git a/content/media/MediaDecoderStateMachine.cpp b/content/media/MediaDecoderStateMachine.cpp
@@ -1273,16 +1273,18 @@ uint32_t MediaDecoderStateMachine::PlayF
+  aChannels = mAudioStream->GetOutChannels();
[...]
   mEventManager.QueueWrittenAudioData(audio->mAudioData.get(),
                                       audio->mFrames * aChannels,

This can't be right.  mEventManager is initialized with the channel count of the media in MediaDecoderStateMachine::DecodeMetadata.  That also means the data in the AudioAvailable event doesn't match what's expected according to MediaElement.mozChannels.

Also, HTMLMediaElement::SetPlaybackRate rejects playbackRate changes for media with > 2 channels, but that condition will be hit if we're downmixing to stereo and could support playbackRate changes.

I'm not a fan of changing AudioData in place.  There's a reason AudioStream::Write is passed a const buffer; it should not be discarded lightly.  (See also bug 495040 comment 47, where this came up before.)

Comment 25

[Alex Chronopoulos [:achronop]]

Thanks for the comment. That would be even more valuable in review time. I will continue the conversation on the follow up bug (Bug 952326).