Tests and fixes for seeking and skipping in audio streams#1124
Tests and fixes for seeking and skipping in audio streams#1124rryan merged 8 commits intomixxxdj:masterfrom uklotzde:soundsource_tests_and_fixes
Conversation
|
Some additional commits (trial and error) were necessary to fix the build and tests for all supported platforms. |
rryan
left a comment
rryan
left a comment
There was a problem hiding this comment.
I skimmed the decoder changes and they look good -- since there is high test coverage I didn't do a thorough review. Thanks!
| // TODO(XXX): Replace 'static const' with 'static constexpr' | ||
| // in the corresponding .h file after upgrading to Visual | ||
| // Studio 2015 and remove all definitions from this .cpp file. | ||
| #if !defined(_MSC_VER) || (_MSC_VER >= 1900) |
There was a problem hiding this comment.
"not Visual Studio OR Visual Studio 2015 and newer"
Is that what you intended? Did you mean "only Visual Studio earlier than 2015"?
#if defined(_MSC_VER) && (_MSC_VER < 1900)
There was a problem hiding this comment.
GCC: With or without definitions, doesn't matter -> optional
LLVM: Definitions are mandatory
Visual Studio 2013: Definitions produce link errors -> forbidden
There is no intersection that works for all 3 compilers: I decided to add the definitions as a default with an exception for Visual Studio <= 2013. Starting with the Visual Studio 2015 the definitions will be included (as for GCC/LLVM) and we will see what happens.
There was a problem hiding this comment.
Hm ok. But I'm still confused why this builds successfully at head?
There was a problem hiding this comment.
What's confusing is that master builds fine with clang and the static constants defined inline in audiosignal.h and not in audiosignal.o, which contradicts what you are saying about them being mandatory?
|
|
||
| namespace mixxx { | ||
|
|
||
| // Definitions of static class constants are required by |
|
|
||
| // Definitions of static class constants are required by | ||
| // LLVM CLang, but cause "multiply defined symbols" errors | ||
| // in Visual Studio 2013. GCC is happy either with or |
|
|
||
| // Definitions of static class constants are required by | ||
| // LLVM CLang, but cause "multiply defined symbols" errors | ||
| // in Visual Studio 2013. GCC is happy either with or |
There was a problem hiding this comment.
I'm confused -- how does this build at head (since Travis/Appveyor uses all of GCC, LLVM, and MSVC2013)?
|
!defined(_MSC_VER) -> GCC, LLVM
_MSC_VER >= 1900 -> Visual Studio 2015, 2017, ...
…On 16.01.2017 23:09, RJ Skerry-Ryan wrote:
MSC_VER >= 1900
|
|
I have reset the branch and excluded the confusing workaround. Now the build for OS X fails:
```
[LD] osx64_build/mixxx-test
Undefined symbols for architecture x86_64:
"mixxx::AudioSignal::kChannelCountStereo", referenced from:
mixxx::SoundSourceOpus::tryOpen(mixxx::AudioSourceConfig const&) in soundsourceopus.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
scons: *** [osx64_build/mixxx-test] Error 1
scons: building terminated because of errors.
```
…On 17.01.2017 00:48, RJ Skerry-Ryan wrote:
***@***.**** commented on this pull request.
----------------------------------------------------------------------------
In src/util/audiosignal.cpp <#1124>:
> @@ -4,6 +4,31 @@
namespace mixxx {
+// Definitions of static class constants are required by
+// LLVM CLang, but cause "multiply defined symbols" errors
+// in Visual Studio 2013. GCC is happy either with or
+// without those definitions.
+// TODO(XXX): Replace 'static const' with 'static constexpr'
+// in the corresponding .h file after upgrading to Visual
+// Studio 2015 and remove all definitions from this .cpp file.
+#if !defined(_MSC_VER) || (_MSC_VER >= 1900)
What's confusing is that master builds fine with clang and the static
constants defined inline in audiosignal.h and not in audiosignal.o, which
contradicts what you are saying about them being mandatory?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1124>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZ-altR92BtxZFinJe6-qXzUV968JNEks5rTAHXgaJpZM4Lj3If>.
|
Very weird, so it must be some change within this PR that causes clang to need this? The constants were already used within multiple .o's before this PR. |
|
Sorry @uklotzde , I didn't mean to make you waste time on this when what you had was working. I was just curious what the reason was. We can merge what you had if you force push it back. |
|
On ubuntu 16.10, gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005 lin64_build/sources/soundsourceopus.o: In function `mixxx::SoundSourceOpus::tryOpen(mixxx::AudioSourceConfig const&)': /home/tim/development/mixxx/src/sources/soundsourceopus.cpp:205: undefined reference to `mixxx::AudioSignal::kChannelCountStereo' collect2: error: ld returned 1 exit status scons: *** [lin64_build/mixxx] Error 1 scons: building terminated because of errors.Note that |
|
This should fix the linking issues on all platform:
#1139
As we've learned the definitions in the .cpp file are actually required. And
Visual Studio 2015 does not complain about duplicate symbols anymore.
…On 21.01.2017 00:02, Tim Rae wrote:
On ubuntu 16.10, gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005
|scons -j8 optimize=off faad=1| fails with linking errors:
lin64_build/sources/soundsourceopus.o: Infunction `mixxx::SoundSourceOpus::tryOpen(mixxx::AudioSourceConfig const&)': /home/tim/development/mixxx/src/sources/soundsourceopus.cpp:205:
undefined reference to `mixxx::AudioSignal::kChannelCountStereo' collect2: error: ld returned 1exit status scons:*** [lin64_build/mixxx] Error 1 scons: building terminated because of errors.```
`optimize=native` works as expected
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1124 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZ-aiAzlKACHboNjmUhUtvY4TONUZp_ks5rUT0JgaJpZM4Lj3If>.
|
3 participants
I added the following test cases for SoundSources:
Apparently various implementations were broken! All fixed with one exception: Seeking near the end of an MP3 stream fails with SoundSourceMP3 (libmad). I temporarily excluded/disabled the failing test and marked it with a TODO. I have no idea what's going wrong here.
The fix for SoundSourceOpus is the biggest one, because opusfile neither implements skipping nor provides sample accurate seeking and decoding. By prefetching sample frames while seeking the accuracy can be improved substantially. All tunable parameters that affect the sample accuracy are now defined in SoundSourceOpus and are available as constants for verifying the test results.