If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Move DownmixToStereo functionality into AudioData class

RESOLVED FIXED in mozilla29

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: achronop, Assigned: achronop)

Tracking

(Depends on: 1 bug)

Trunk
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 790459
(Assignee)

Comment 1

4 years ago
Ralph, can you please confirm the bug and assign it to me? Thanks!
Whiteboard: [lang=c++]
(Assignee)

Comment 2

4 years ago
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?
(Assignee)

Updated

4 years ago
Assignee: nobody → achronop
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(paul)
Flags: needinfo?(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.
Flags: needinfo?(kinetik)
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.
Blocks: 899050
Flags: needinfo?(paul)
(Assignee)

Comment 7

4 years ago
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.
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 806226 [details] [diff] [review]
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!
(Assignee)

Comment 10

4 years ago
Created attachment 816374 [details] [diff] [review]
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.
Attachment #816374 - Flags: review?(giles)
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?
Attachment #816374 - Flags: review?(giles) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 8333483 [details] [diff] [review]
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!
Attachment #816374 - Attachment is obsolete: true
Attachment #8333483 - Flags: review?(giles)
(Assignee)

Comment 13

4 years ago
Try progress:

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

Comment 14

4 years ago
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!
(Assignee)

Comment 15

4 years ago
Created attachment 8333580 [details] [diff] [review]
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!
Attachment #8333483 - Attachment is obsolete: true
Attachment #8333483 - Flags: review?(giles)
Attachment #8333580 - Flags: review?(giles)
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
Attachment #8333580 - Flags: review?(giles) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 8342590 [details] [diff] [review]
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.
Attachment #8333580 - Attachment is obsolete: true
Attachment #8342590 - Flags: review?(giles)
(Assignee)

Comment 18

4 years ago
Try link:
https://tbpl.mozilla.org/?tree=Try&rev=37b3b7c98242
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.
Attachment #8342590 - Flags: review?(giles) → review+
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 21

4 years ago
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!
Attachment #806226 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/41fc2afc8465
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/41fc2afc8465
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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.)
Blocks: 952326
No longer blocks: 952326
Depends on: 952326
(Assignee)

Comment 25

4 years ago
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).

Updated

3 years ago
Depends on: 1073793
You need to log in before you can comment on or make changes to this bug.