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

RESOLVED FIXED in mozilla31

Status

()

defect
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/src/SoundTouch.cpp#146

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
Posted patch patch v1Splinter Review
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)
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)
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
Posted patch Test file added (obsolete) — Splinter Review
Attachment #764048 - Attachment is obsolete: true
Attachment #764061 - Flags: review?(paul)
Reporter

Updated

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

Comment 15

6 years ago
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
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
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.