Update the in-tree copy of libsoundtouch to get multichannel support

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: padenot, Assigned: michael)

Tracking

unspecified
mozilla31
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=padenot][lang=c++])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
We are using the SoundTouch library to implement the |playbackRate| member of the <audio> and <video> tags. This library gives us the ability to time stretch the audio, so it plays slower or faster, without affecting the pitch (so we don't get, say, a mickey mouse voice when playing the media at high speed).

SoundTouch only supports up to two channels (mono or stereo audio), and because we can play media file that have more than two channels, we need to either:

(1) Have multiple time stretcher in the AudioStream instance to handle all the channels (bad way)
(2) Patch SoundTouch so it can handle more than two channels, and upstream the patches (good way)

This bug is about implementing the second solution.

The code is in [1], and the library is independent [2], so there is not too much code to read before actually implementing the feature.

[1]: http://mxr.mozilla.org/mozilla-central/source/media/libsoundtouch/
[2]: http://www.surina.net/soundtouch/
Hi Paul,

I am just starting out and I would like to work on this ....
I will go through the docs and get in touch.

Ajay
(Reporter)

Comment 2

6 years ago
Ajay, if you need info, come on IRC, on irc.mozilla.com, channel #media, during late European timezone (UTC+1), I'm "padenot" there.
Hi Paul,

I read the link related to Soundtouch library and some of the stuff related to mono and stereo sounds.

Could you provide me some more related details :
1) This is related to Firefox OS ?
2) could I see the working of this sound library somewhere and its failure in case when channels > 2 ?
3) Any other docs which I should read ?

(I came to the #media channel I just tried to contact you on the normal chat, should I PM you next time)

Thanks;
Ajay
(Reporter)

Comment 4

6 years ago
1) It is not related to Firefox OS right now, the |playbackRate| member of the audio and video elements have not made up in time when the Firefox OS branch was cut. However, it will be in the second Firefox OS version for sure. So that leaves us with Firefox Desktop and Firefox for Android for now.

2a) Inside of Firefox, just get a nightly build (http://nightly.mozilla.org) for your platform, go and play any HTML5 video (such as those on this test page: http://paul.cx/public/playbackrate/, there are buttons to change the playback rate. Note that the pitch is not affected. The playback rate modification is achieved using the SoundTouch library. Inspecting the code, you see that this is achieved by setting the |playbackRate| property of an audio or video element).

2b) Outside of Firefox, simply get the source code of the library, compile it (there is instructions in the package), and run the examples. It might be easier working with the stand alone library. 

2c) The failure case is simply an assert: http://mxr.mozilla.org/mozilla-central/source/media/libsoundtouch/﷒0﷓

3) I believe reading the source code of SoundTouch will help. Basically, all we need to do here is to make every piece of code that is mono or stereo specific able to handle an arbitrary number of channels.

I saw you tried to ping me on irc, but I was not in front of my computer. Contacting me in the channel is fine :-).
I will check this info and will conatct you
Hi Paul,

I was trying out some changes in the soundtouch libraray files (.cpp files)
Basically I am using SoundStretch tool provided with it on a sample .wav file

I am trying to add some printfs or cout inside files like SoundTouch.cpp, but the output doesnot come on screen or in dmesg (Althogh it works without any error)? Any ideas how can I add printfs inside the Soundtocuh library?

Ajay
Created attachment 760995 [details] [diff] [review]
patch v1

I just changed the files for FIRFilter. There could have been similar changes to the files RateTransposer.cpp and TDStretch.cpp but it seems that they set the channel to 2 in the constructor, so I did not touch them. Should they be changed as well?
Attachment #760995 - Flags: feedback?(paul)

Comment 8

6 years ago
Created attachment 761002 [details] [diff] [review]
first patch for sound touch N channels (not working)
(Reporter)

Comment 9

6 years ago
So, the maintainer of libsoundtouch told us today that someone just added multichannel support to the library. This bug is now about importing the new version of the library and updating the patch we have on top, making sure it works properly, writing a test that uses a multichannel file and uses the library.
Assignee: nobody → peyrard.max43
Summary: Make the SoundTouch library able to handle more than 2 channels. → Update the in-tree copy of libsoundtouch to get multichannel support
(Reporter)

Comment 10

6 years ago
Comment on attachment 760995 [details] [diff] [review]
patch v1

Unsetting the flag since this is not needed anymore.
Attachment #760995 - Flags: feedback?(paul)

Comment 11

6 years ago
Created attachment 764048 [details] [diff] [review]
This patch updates the soundtouch library and adds a test
Attachment #764048 - Flags: review?(paul)
(Reporter)

Comment 12

6 years ago
Comment on attachment 764048 [details] [diff] [review]
This patch updates the soundtouch library and adds a test

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

You forgot to hg add the test file!
Attachment #764048 - Flags: review?(paul)

Comment 13

6 years ago
Created attachment 764061 [details] [diff] [review]
Test file added
Attachment #764048 - Attachment is obsolete: true
Attachment #764061 - Flags: review?(paul)
(Reporter)

Updated

6 years ago
Attachment #764061 - Flags: review?(paul) → review+
(Reporter)

Comment 14

6 years ago
Looks good. Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=2ceb1050ddb2

Comment 15

6 years ago
Created attachment 764137 [details] [diff] [review]
Patch updated : alloca function replaced by malloc
Attachment #764061 - Attachment is obsolete: true
Attachment #764137 - Flags: review?(paul)
(Reporter)

Comment 16

6 years ago
Comment on attachment 764137 [details] [diff] [review]
Patch updated : alloca function replaced by malloc

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

We found that this version is super crashy, cancelling review for now.
Attachment #764137 - Flags: review?(paul)
A new version of libsoundtouch was released in Jan-2014. Should we reconsider this?
(Reporter)

Comment 18

5 years ago
We should, yes.
Let me email the core developer of libsoundtouch and I will report here.
Core developer of libsoundtouch replied to my email:

"The multichannel support has been there quite some time now, and no bug reports have been submitted about it for ages so I consider it as stable :).

For testing and debugging reasons, in case you're interested about such, there's a compile-time flag which enforces using multi-channel routine always also for mono- and stereo-sound, because 1- and 2-channel audio can be considered as specific cases of multichannel audio.

Benefit of such multichannel-algorithm-enforce mode is to achieve better testing exposure for the multichannel support (as most available audio sources do not have more than 2 channels), with penalty of bit worse runtime performance as optimizations of native mono/stereo audio routines then do not apply, yet such is quite a small issue nowadays anyway.

best regards, Olli"
(Assignee)

Comment 21

5 years ago
Created attachment 8403053 [details] [diff] [review]
update-libsoundtouch-multichannel.patch

I've updated libsoundtouch to r198, which includes support for multichannel audio. I have also updated AudioStream.cpp and MediaDecoderStateMachine.cpp in content/media to remove the restriction on multichannel audio sources. Finally I have added a 6-channel audio file (content/media/test/bug495794.ogg) to the list of files to be used for testing playback.
(Assignee)

Updated

5 years ago
Attachment #8403053 - Flags: feedback?(paul)
(Reporter)

Comment 22

5 years ago
Comment on attachment 8403053 [details] [diff] [review]
update-libsoundtouch-multichannel.patch

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

http://tbpl.mozilla.org/?tree=Try&rev=50493f2f4584

::: content/media/test/manifest.js
@@ +44,5 @@
>    { name:"seek.webm", type:"video/webm", duration:3.966 },
>    { name:"gizmo.mp4", type:"video/mp4", duration:5.56 },
>    { name:"owl.mp3", type:"audio/mpeg", duration:3.29 },
> +  { name:"vbr.mp3", type:"audio/mpeg", duration:10.0 },
> +  { name:"bug495794.ogg", type:"audio/ogg", duration:0.3 }

Note that the tests that currently exercise .playbackRate are currently disabled. I'll push to try servers with them enabled.
Attachment #8403053 - Flags: feedback?(paul) → feedback+
(Reporter)

Comment 23

5 years ago
We need to do something for Windows, it does not have alloca, but has _alloca. A little #define hackery will work, I think.
(Reporter)

Comment 24

5 years ago
Comment on attachment 8403053 [details] [diff] [review]
update-libsoundtouch-multichannel.patch

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

::: media/libsoundtouch/src/FIRFilter.cpp
@@ +169,5 @@
>  
> +uint FIRFilter::evaluateFilterMulti(SAMPLETYPE *dest, const SAMPLETYPE *src, uint numSamples, uint numChannels) const
> +{
> +    uint i, j, end, c;
> +    LONG_SAMPLETYPE *sum=(LONG_SAMPLETYPE*)alloca(numChannels*sizeof(*sum));

Here, this does not compile on vs2010, which is our default compiler on windows.

::: media/libsoundtouch/src/SoundTouch.cpp
@@ +348,4 @@
>      int i;
>      int nUnprocessed;
>      int nOut;
> +    SAMPLETYPE *buff=(SAMPLETYPE*)alloca(64*channels*sizeof(SAMPLETYPE));

Also there.
(Assignee)

Comment 25

5 years ago
Created attachment 8403373 [details] [diff] [review]
update-libsoundtouch-multichannel.patch

I've updated FIRFilter.cpp and SoundTouch.cpp to use _alloca when compiling with MSVC.
Attachment #8403053 - Attachment is obsolete: true
Attachment #8403373 - Flags: feedback?(paul)
(Reporter)

Comment 26

5 years ago
Comment on attachment 8403373 [details] [diff] [review]
update-libsoundtouch-multichannel.patch

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

Can you please regenerate the file moz-libsoundtouch.patch, now that we carry additional changes (alloca -> _alloca) ?
Attachment #8403373 - Flags: feedback?(paul)
(Reporter)

Comment 27

5 years ago
Forget that. I pushed to try again, it looks good: https://tbpl.mozilla.org/?tree=Try&rev=28b9150c2cb3
(Assignee)

Updated

5 years ago
Attachment #8403373 - Flags: review?(paul)
(Reporter)

Updated

5 years ago
Attachment #8403373 - Flags: review?(paul) → review+
(Reporter)

Comment 28

5 years ago
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f46ce50538
Assignee: peyrard.max43 → michael
https://hg.mozilla.org/mozilla-central/rev/a5f46ce50538
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.