Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add MSVC support for GigPlayer #6595

Closed
wants to merge 5 commits into from
Closed

Add MSVC support for GigPlayer #6595

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
63246ad
Support GigPlayer with MSVC
Veratil Dec 21, 2022
1186840
Update to build on Linux
Veratil Dec 22, 2022
a746547
Add back removed libgig name for find_library
Veratil Dec 23, 2022
d1f4563
Remove unneeded option
Veratil Dec 23, 2022
8bac268
Update CMake and add const
Veratil Dec 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,13 +507,13 @@ ENDIF(WANT_SF2)

# check for libgig
If(WANT_GIG)
PKG_CHECK_MODULES(GIG gig)
IF(GIG_FOUND)
find_package(Gig)
IF(Gig_FOUND)
SET(LMMS_HAVE_GIG TRUE)
SET(STATUS_GIG "OK")
ELSE(GIG_FOUND)
ELSE(Gig_FOUND)
SET(STATUS_GIG "not found, libgig needed for decoding .gig files")
ENDIF(GIG_FOUND)
ENDIF(Gig_FOUND)
ENDIF(WANT_GIG)

# check for pthreads
Expand Down
34 changes: 34 additions & 0 deletions cmake/modules/FindGig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Attempt to find libigg using PkgConfig
find_package(PkgConfig QUIET)
if(PKG_CONFIG_FOUND)
pkg_check_modules(LIBGIG_PKG gig)
endif()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a blank line here. Also, do you want to add a copyright notice to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what copyright to put since it's nearly a direct copy of some others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no lawyer, but I imagine something like this should be fine:

# Copyright (c) <year> <name>
# Based on <file>, copyright (c) <name> <year>
#
# Redistribution and use is allowed according to the terms of the New BSD license.
# For details see the accompanying COPYING-CMAKE-SCRIPTS file.

Besides, if you, the author, are not sure about the copyright, some random person in the future wanting to use this in their project is going to have absolutely no clue.

if(WIN32)
Veratil marked this conversation as resolved.
Show resolved Hide resolved
find_package(libgig ${Gig_FIND_VERSION} CONFIG QUIET)
Veratil marked this conversation as resolved.
Show resolved Hide resolved
if(libgig_FOUND)
get_target_property(libgig_Location libgig::libgig LOCATION)
get_target_property(libgig_Include_Path libgig::libgig INTERFACE_INCLUDE_DIRECTORIES)
get_filename_component(libgig_Path LibGig_Location PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using individual variables extracted from the imported target, it's more idiomatic modern CMake to link to the target directly. In the pkg-config case, consider instead assembling the information read from pkg-config into an imported target named the same as that from vcpkg.

(Feel free to consider this out of scope, since the current style is the one in use prior to this PR, but I've taken the opportunity to switch over to targets when doing similar PRs in the past.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is beyond what I know of CMake, but if you have a reference of another done then I can work on converting it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FindPortaudio.cmake is probably closest to what you're trying to do, but FindLame.cmake and FindSTK.cmake are quite similar too. FindFluidSynth.cmake and FindSDL2.cmake are a bit more complex, but should serve as further examples.

endif()
endif()

# Find the library and headers using the results from PkgConfig as a guide
find_path(LIBGIG_INCLUDE_DIR
NAMES gig.h
PATHS ${LIBGIG_PKG_INCLUDE_DIRS} "${libgig_Include_Path}/libgig"
)

find_library(LIBGIG_LIBRARY
NAMES gig
PATHS ${LIBGIG_PKG_LIBRARY_DIRS} ${libgig_Path}
)

find_package(PackageHandleStandardArgs)
find_package_handle_standard_args(Gig DEFAULT_MSG LIBGIG_LIBRARY LIBGIG_INCLUDE_DIR)

set(GIG_LIBRARY_DIRS ${LIBGIG_LIBRARY})
set(GIG_LIBRARIES ${LIBGIG_LIBRARY})
set(GIG_INCLUDE_DIRS ${LIBGIG_INCLUDE_DIR})

mark_as_advanced(LIBGIG_LIBRARY LIBGIG_LIBRARIES LIBGIG_INCLUDE_DIR LIBGIG_INCLUDE_DIRS)
10 changes: 6 additions & 4 deletions plugins/GigPlayer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@ if(LMMS_HAVE_GIG)
INCLUDE_DIRECTORIES(${GIG_INCLUDE_DIRS})

# Required for not crashing loading files with libgig
SET(GCC_COVERAGE_COMPILE_FLAGS "-fexceptions")
add_definitions(${GCC_COVERAGE_COMPILE_FLAGS})
if(NOT MSVC)
SET(GCC_COVERAGE_COMPILE_FLAGS "-fexceptions")
add_definitions(${GCC_COVERAGE_COMPILE_FLAGS})
endif(NOT MSVC)
Veratil marked this conversation as resolved.
Show resolved Hide resolved

# disable deprecated check for mingw-x-libgig
if(LMMS_BUILD_WIN32)
if(LMMS_BUILD_WIN32 AND MINGW)
SET(GCC_GIG_COMPILE_FLAGS "-Wno-deprecated")
add_definitions(${GCC_GIG_COMPILE_FLAGS})
endif(LMMS_BUILD_WIN32)
endif(LMMS_BUILD_WIN32 AND MINGW)
Veratil marked this conversation as resolved.
Show resolved Hide resolved

LINK_DIRECTORIES(${GIG_LIBRARY_DIRS} ${SAMPLERATE_LIBRARY_DIRS})
LINK_LIBRARIES(${GIG_LIBRARIES} ${SAMPLERATE_LIBRARIES})
Expand Down
20 changes: 16 additions & 4 deletions plugins/GigPlayer/GigPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,11 @@ void GigInstrument::play( sampleFrame * _working_buffer )
}

// Load this note's data
#ifndef _MSC_VER
sampleFrame sampleData[samples];
#else
const auto sampleData = static_cast<sampleFrame*>(_alloca(samples * sizeof(sampleFrame)));
#endif
loadSample(sample, sampleData, samples);

// Apply ADSR using a copy so if we don't use these samples when
Expand All @@ -460,7 +464,11 @@ void GigInstrument::play( sampleFrame * _working_buffer )
// Output the data resampling if needed
if( resample == true )
{
#ifndef _MSC_VER
sampleFrame convertBuf[frames];
#else
const auto convertBuf = static_cast<sampleFrame*>(_alloca(frames * sizeof(sampleFrame)));
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid calling _alloca within a loop - it can easily lead to a stack overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we use instead? I don't recall where I got this from but I do remember MSVC did not like convertBuf as it's defined above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lift the variable out of the loop if possible - it should be fine to reuse the same buffer for each iteration. There's another instance in this file where the array size is computed inside the loop - in this case, see if it's possible to compute an upper bound for the size.

Eventually, we should get rid of compiler-specific, non-standard stuff like this and preallocate buffers on the heap. However, that's a fairly significant change that's out of scope here, and needs to be applied elsewhere in the codebase too.

Copy link
Member

@PhysSong PhysSong Aug 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another simple solution: move the loop body to another function to make sure the memory allocated by _alloca is deallocated at the end of every iteration.

Copy link
Contributor

@Rossmaxx Rossmaxx Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an even simpler solution for now (It's an ugly hack which should be replaced asap but for the current use case, didn't find anything simpler).

We can add

#ifdef _MSC_VER
_freea(convertBuf)
#endif

Just before closing the loop. That way, it'll deallocate the convertBuf and avoid the memory leak. Tbh we should be rewriting this part but it's beyond the scope.


// Only output if resampling is successful (note that "used" is output)
if (sample.convertSampleRate(*sampleData, *convertBuf, samples, frames, freq_factor, used))
Expand Down Expand Up @@ -531,7 +539,11 @@ void GigInstrument::loadSample( GigSample& sample, sampleFrame* sampleData, f_cn
}

unsigned long allocationsize = samples * sample.sample->FrameSize;
#ifndef _MSC_VER
int8_t buffer[allocationsize];
#else
Copy link
Contributor

@Rossmaxx Rossmaxx Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also add _freea(buffer) within the ifdef i posted in the other review comment.

Edit : GitHub view messed up I'm referring to the line below what's shown.

auto buffer = static_cast<int8_t*>(_alloca(allocationsize * sizeof(int8_t)));
Veratil marked this conversation as resolved.
Show resolved Hide resolved
#endif

// Load the sample in different ways depending on if we're looping or not
if( loop == true && ( sample.pos >= loopStart || sample.pos + samples > loopStart ) )
Expand Down Expand Up @@ -576,14 +588,14 @@ void GigInstrument::loadSample( GigSample& sample, sampleFrame* sampleData, f_cn
{
sample.sample->SetPos( sample.pos );

unsigned long size = sample.sample->Read( &buffer, samples ) * sample.sample->FrameSize;
std::memset( (int8_t*) &buffer + size, 0, allocationsize - size );
unsigned long size = sample.sample->Read( buffer, samples ) * sample.sample->FrameSize;
std::memset( (int8_t*) buffer + size, 0, allocationsize - size );
Comment on lines +591 to +592
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unsigned long size = sample.sample->Read( buffer, samples ) * sample.sample->FrameSize;
std::memset( (int8_t*) buffer + size, 0, allocationsize - size );
const auto size = sample.sample->Read(buffer, samples) * sample.sample->FrameSize;
std::memset(buffer + size, 0, allocationsize - size);

Perhaps this can be written a bit better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of changes that should happen here in GigPlayer, but I decided to leave this as a minimum viable PR to get it to build on Windows. Definitely something that should be updated in a followup.

}

// Convert from 16 or 24 bit into 32-bit float
if( sample.sample->BitDepth == 24 ) // 24 bit
{
auto pInt = reinterpret_cast<uint8_t*>(&buffer);
auto pInt = reinterpret_cast<uint8_t*>(buffer);

for( f_cnt_t i = 0; i < samples; ++i )
{
Expand Down Expand Up @@ -615,7 +627,7 @@ void GigInstrument::loadSample( GigSample& sample, sampleFrame* sampleData, f_cn
}
else // 16 bit
{
auto pInt = reinterpret_cast<int16_t*>(&buffer);
auto pInt = reinterpret_cast<int16_t*>(buffer);

for( f_cnt_t i = 0; i < samples; ++i )
{
Expand Down