Windows support#47
Conversation
We need to add getopt/dirent for MS Visual Studio support.
Separate libmusly from kissfft and libsamplerate
Add explicit static_cast<>s where we implicitly downcasted the value.
Adds #ifdefs for MSVC and missing headers
- Unreachable code - Correct data type for varables
This fixes an annoying warning with MSVC++
As the macros are used/written they yield an extra ";". This commit fixes the pedantic warning.
C code from external libraries uses a custom warning level.
This is required because config.h is generated in the binary dir of the cmake build.
The bit-flags in av_seek_frame() were logically ||'d instead of bitwise |'d
Too many ;
Once again too many ;
f0k
left a comment
f0k
left a comment
There was a problem hiding this comment.
Hey, I'm only halfway through, but thought I'd share the comments I have so far. You did a lot! 👍
| - cd build | ||
| - cmake .. | ||
| - make -j | ||
| - cmake -DBUILD_STATIC=ON -DCMAKE_BUILD_TYPE=Release ${CARGS} .. |
There was a problem hiding this comment.
Why the static build? Is it not working otherwise?
| set_property(GLOBAL PROPERTY USE_FOLDERS ON) | ||
|
|
||
| if (MSVC) | ||
| # Ninja uses those flags on Windows, MSBuild ignores them |
There was a problem hiding this comment.
Maybe also add a comment on what these flags are meant to do, similar to the next block?
| set(BUILD_SHARED_LIBS ON) | ||
| endif () | ||
|
|
||
| set(CMAKE_POSITION_INDEPENDENT_CODE ON) |
There was a problem hiding this comment.
Can you give a short explanation why setting this is a good idea? (Or add a one-line comment to the file?) According to the documentation, it's enabled by default for shared libraries, but not for executables.
| endif() | ||
|
|
||
| # configure libresample | ||
| find_file(HAVE_INTTYPES inttypes.h) |
|
|
||
| target_link_libraries(libmusly | ||
| kissfft | ||
| libresample |
There was a problem hiding this comment.
Sorry if that was included in one of the commit messages, but can you explain the implications of separating out kissfft and libresample into static libraries and linking them here, in case we're building a shared libmusly library for Linux? Any possible negative effects compared to compiling and linking all the objects at once?
| excerpt_start * st->time_base.den / st->time_base.num, | ||
| AVSEEK_FLAG_BACKWARD || AVSEEK_FLAG_ANY) >= 0)) { | ||
| if ((excerpt_start > 0) && (av_seek_frame(fmtx, audio_stream_idx, | ||
| static_cast<int64_t>(static_cast<double>(excerpt_start) * st->time_base.den) / st->time_base.num, |
There was a problem hiding this comment.
Now you're doing the multiplication in double, convert to int64, and divide? Shouldn't we either do everything in int, or do everything in double if we fear an overflow? And do we want the code to look like that for 32-bit targets as well?
There was a problem hiding this comment.
For consistency, we might adopt what the code for file_length = does.
| if (excerpt_start < 0) { | ||
| // center excerpt, but start at -excerpt_start the latest | ||
| float file_length = decoded_pcm.size() / decx->sample_rate; | ||
| float file_length = static_cast<float>(decoded_pcm.size() / decx->sample_rate); |
There was a problem hiding this comment.
Maybe we'd want the division to happen in float?
2 participants
This PR addresses issue #15. It enables building with Visual studio.
(1) It adds a Travis Windows build, so we don't break it again. See the Windows build for this branch: https://travis-ci.org/dominikschnitzer/musly/builds/579697952
(2) bumps the Warning level to Wall (gcc/clang) and /W4 (VC++)
(3) compiles with with warnings as errors (Werror and /WX)
(4) Subsequently fixes all compiler warnings that broke the build due to (2) and (3)