Closed Bug 1588233 Opened 11 months ago Closed 10 months ago

Update libsoundtouch to 2.1.2

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files)

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.

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

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

Depends on D49028

Assignee: nobody → cchang

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.

Flags: needinfo?(cchang)

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.

Flags: needinfo?(cchang)

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

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?

Flags: needinfo?(mh+mozilla)

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...

Flags: needinfo?(mh+mozilla)

(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.

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

Attachment #9100611 - Attachment description: Bug 1588233 - P2: Update soundtouch to version 2.1.2. r?padenot → Bug 1588233 - P3: Update soundtouch to version 2.1.2. r?padenot

(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

(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.

Attachment #9108570 - Attachment description: Bug 1588233 - P2: Define ST_NO_EXCEPTION_HANDLING in STTypes.h directly. r?glandium → Bug 1588233 - P2: forward declaring SoundTouch. r?glandium
Attachment #9108570 - Attachment description: Bug 1588233 - P2: forward declaring SoundTouch. r?glandium → Bug 1588233 - P2: Set ST_NO_EXCEPTION_HANDLING before using SoundTouch. r?glandium
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2731c91357d9
P1: Correct the moz-patch for soundtouch. r=padenot
Pushed by ccoroiu@mozilla.com:
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
Blocks: 1427267
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/721ef7797a3f
Follow-up: Update moz.yaml for the 2.1.2 update. r=chunmin
See Also: → 1614381
You need to log in before you can comment on or make changes to this bug.