1588233 - Update libsoundtouch to 2.1.2

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P3)

Description

[C.M.Chang[:chunmin]]

The current soundtouch works fine, but some users complain about the audio quality after speeding-up the audio (e.g., bug 1383363). The soundtouch hasn't been updated for a while. The current soundtouch version is 2.1.2, updating the soundtouch to the newest version might be helpful for improving the audio quality after changing the speed I guess.

Comment 1

[C.M.Chang[:chunmin]]

Attachment: Bug 1588233 - P1: Correct the moz-patch for soundtouch. r?padenot

Correct the format of the diff for AAFilter.cpp so the update-script can
work.

Comment 2

[C.M.Chang[:chunmin]]

Attachment: Bug 1588233 - P3: Update soundtouch to version 2.1.2. r?padenot

The changed-files are updated by running sh update.sh <upstream-path>
with moz-libsoundtouch.patch applied.

Depends on D49028

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:chunmin, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 4

[C.M.Chang[:chunmin]]

I'll need to record the two audio files, one is from current playback, the other is the audio after bumping up the libsoundtouch, to see if the audio quality is improved.

Comment 5

[C.M.Chang[:chunmin]]

Updating libsoundtouch would solve some old issues, so we should update it no matter it gives better audio quility or not.

Comment 6

[C.M.Chang[:chunmin]]

Hi, glandium,

I was wondering if you can give me some hints about how to solve this build failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e5aa174b08c417d8684a27780daef804c556a40&selectedJob=275423737

It seems that ST_NO_EXCEPTION_HANDLING won't be defined, even it's set in media/libsoundtouch/src/moz.build, when building StaticComponents. I don't know why this only happens after updating the libsoundtouch. Does the ST_NO_EXCEPTION_HANDLING needs to be set in /xpcom/components/moz.build? Why some builds doesn't include libsoundtouch in StaticComponents: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=275423746&repo=try&lineNumber=18623?

Comment 7

[Mike Hommey [:glandium]]

It's pretty clear from the log:

In file included from StaticComponents.cpp:42:
In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MediaManager.h:8:
In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/MediaEngine.h:10:
In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/MediaTrackGraph.h:9:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/AudioStream.h:19:
In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/soundtouch/SoundTouchFactory.h:11:
In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/soundtouch/SoundTouch.h:68:
 /builds/worker/workspace/build/src/obj-firefox/dist/include/soundtouch/FIFOSamplePipe.h:62:9: error: cannot use 'throw' with exceptions disabled
        ST_THROW_RT_ERROR("Error: Illegal number of channels");
        ^

I don't know why this only happens after updating the libsoundtouch.

Because libsoundtouch wasn't using ST_THROW_RT_ERROR from a header before.

Does the ST_NO_EXCEPTION_HANDLING needs to be set in /xpcom/components/moz.build?

Probably not. Because I suspect many other things will be including AudioStream.h or something that includes it. The question is more: does AudioStream.h actually need to include SoundTouchFactory.h. And as far as I can see, it doesn't. It could just forward-declare soundtouch::SoundTouch.

Why some builds doesn't include libsoundtouch in StaticComponents

It is actually included, but apparently the compiler doesn't care that an exception is thrown. That part is weird, because it's the same compiler on all platforms. Maybe it's a libstdc++ thing...

Comment 8

[C.M.Chang[:chunmin]]

(In reply to Mike Hommey [:glandium] (high latency) from comment #7)

Does the ST_NO_EXCEPTION_HANDLING needs to be set in /xpcom/components/moz.build?

Probably not. Because I suspect many other things will be including AudioStream.h or something that includes it. The question is more: does AudioStream.h actually need to include SoundTouchFactory.h. And as far as I can see, it doesn't. It could just forward-declare soundtouch::SoundTouch.

I've tried that by forward-declaring for soundtouch::SoundTouch in AudioStream.h and move the SoundTouchFactory.h to AudioStream.cpp but the my local build fails for Unified files:

 9:40.14 toolkit/library/build/libxul.so
 9:42.17 ld.lld: error: undefined hidden symbol: soundtouch::SoundTouch::setSampleRate(unsigned int)
 9:42.17 >>> referenced by AudioStream.cpp:172 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:172)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::EnsureTimeStretcherInitializedUnlocked())
 9:42.17 ld.lld: error: undefined hidden symbol: soundtouch::SoundTouch::setChannels(unsigned int)
 9:42.17 >>> referenced by AudioStream.cpp:173 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:173)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::EnsureTimeStretcherInitializedUnlocked())
 9:42.17 ld.lld: error: undefined hidden symbol: soundtouch::SoundTouch::setPitch(double)
 9:42.17 >>> referenced by AudioStream.cpp:174 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:174)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::EnsureTimeStretcherInitializedUnlocked())
 9:42.17 ld.lld: error: undefined hidden symbol: soundtouch::SoundTouch::setTempo(double)
 9:42.17 >>> referenced by AudioStream.cpp:0 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:0)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::SetPlaybackRate(double))
 9:42.17 >>> referenced by AudioStream.cpp:226 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:226)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::SetPreservesPitch(bool))
 9:42.17 >>> referenced by AudioStream.cpp:229 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:229)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::SetPreservesPitch(bool))
 9:42.17 ld.lld: error: undefined hidden symbol: soundtouch::SoundTouch::setRate(double)
 9:42.17 >>> referenced by AudioStream.cpp:0 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:0)
 9:42.17 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::SetPlaybackRate(double))
 9:42.18 >>> referenced by AudioStream.cpp:0 (/home/cm/Work/gecko/dom/media/AudioStream.cpp:0)
 9:42.18 >>>               ../../../dom/media/Unified_cpp_dom_media1.o:(mozilla::AudioStream::SetPreservesPitch(bool))
 9:42.34 clang-9: error: linker command failed with exit code 1 (use -v to see invocation)

I guess the libsoundtouch needs to be set somewhere?

Instead of adding settings in different places, defining ST_NO_EXCEPTION_HANDLING directly in STTypes.h would be easier.

Comment 9

[C.M.Chang[:chunmin]]

Attachment: Bug 1588233 - P2: Set ST_NO_EXCEPTION_HANDLING before using SoundTouch. r?glandium

The comming updated libsoundtouch would be built within the
StaticComponents, which doesn't set ST_NO_EXCEPTION_HANDLING. The
ST_NO_EXCEPTION_HANDLING setting in libsoundtouch/src won't be passed to
there. Instead of setting ST_NO_EXCEPTION_HANDLING in different places,
defining it in STTypes.h would be easier to manage this setting.

Depends on D49028

Comment 10

[Mike Hommey [:glandium]]

(In reply to C.M.Chang[:chunmin] from comment #8)

(In reply to Mike Hommey [:glandium] (high latency) from comment #7)

Does the ST_NO_EXCEPTION_HANDLING needs to be set in /xpcom/components/moz.build?

Probably not. Because I suspect many other things will be including AudioStream.h or something that includes it. The question is more: does AudioStream.h actually need to include SoundTouchFactory.h. And as far as I can see, it doesn't. It could just forward-declare soundtouch::SoundTouch.

I've tried that by forward-declaring for soundtouch::SoundTouch in AudioStream.h and move the SoundTouchFactory.h to AudioStream.cpp but the my local build fails for Unified files:

Try again, but also remove the soundtouch headers from config/system-headers.mozbuild

Comment 11

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] (high latency) from comment #10)

(In reply to C.M.Chang[:chunmin] from comment #8)

(In reply to Mike Hommey [:glandium] (high latency) from comment #7)

Does the ST_NO_EXCEPTION_HANDLING needs to be set in /xpcom/components/moz.build?

Probably not. Because I suspect many other things will be including AudioStream.h or something that includes it. The question is more: does AudioStream.h actually need to include SoundTouchFactory.h. And as far as I can see, it doesn't. It could just forward-declare soundtouch::SoundTouch.

I've tried that by forward-declaring for soundtouch::SoundTouch in AudioStream.h and move the SoundTouchFactory.h to AudioStream.cpp but the my local build fails for Unified files:

Try again, but also remove the soundtouch headers from config/system-headers.mozbuild

Ah no, that won't work. Add a MOZ_EXPORT to your forward-declaration, then.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2731c91357d9
P1: Correct the moz-patch for soundtouch. r=padenot

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d46f9bfcab5
P2: Set ST_NO_EXCEPTION_HANDLING before using SoundTouch. r=glandium
https://hg.mozilla.org/integration/autoland/rev/41cd531e5e83
P3: Update soundtouch to version 2.1.2. r=padenot

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2731c91357d9
https://hg.mozilla.org/mozilla-central/rev/1d46f9bfcab5
https://hg.mozilla.org/mozilla-central/rev/41cd531e5e83

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1588233 - Follow-up: Update moz.yaml for the 2.1.2 update.

DONTBUILD

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/721ef7797a3f
Follow-up: Update moz.yaml for the 2.1.2 update. r=chunmin

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/721ef7797a3f