Skip to content

Add MSVC support for GigPlayer #6595

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

Closed
Veratil wants to merge 5 commits into from
Closed

Add MSVC support for GigPlayer #6595

Show file tree
Hide file tree
Changes from all 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
10 changes: 5 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,14 +507,14 @@ 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()
SET(STATUS_GIG "not found, libgig needed for decoding .gig files")
ENDIF(GIG_FOUND)
ENDIF(WANT_GIG)
ENDIF()
ENDIF()

# check for pthreads
IF(LMMS_BUILD_LINUX OR LMMS_BUILD_APPLE OR LMMS_BUILD_OPENBSD OR LMMS_BUILD_FREEBSD OR LMMS_BUILD_HAIKU)
Expand Down
35 changes: 35 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,35 @@

Copy link
Member

@DomClark DomClark Aug 19, 2023

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

@Veratil Veratil Aug 19, 2023

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

@DomClark DomClark Aug 19, 2023

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.

# Attempt to find package libgig (vcpkg first), then fall back to pkg-config if not found
find_package(libgig ${Gig_FIND_VERSION} CONFIG QUIET)

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)
else()
find_package(PkgConfig QUIET)
if(PKG_CONFIG_FOUND)
pkg_check_modules(LIBGIG_PKG gig)
endif()
endif()

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

# vcpkg finds libgig, pkg-config finds gig
find_library(LIBGIG_LIBRARY
NAMES gig libgig
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: 3 additions & 7 deletions plugins/GigPlayer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,14 @@ if(LMMS_HAVE_GIG)
INCLUDE(BuildPlugin)
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})

# 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()

LINK_DIRECTORIES(${GIG_LIBRARY_DIRS} ${SAMPLERATE_LIBRARY_DIRS})
LINK_LIBRARIES(${GIG_LIBRARIES} ${SAMPLERATE_LIBRARIES})
BUILD_PLUGIN(gigplayer GigPlayer.cpp GigPlayer.h PatchesDialog.cpp PatchesDialog.h PatchesDialog.ui MOCFILES GigPlayer.h PatchesDialog.h UICFILES PatchesDialog.ui EMBEDDED_RESOURCES "${CMAKE_CURRENT_SOURCE_DIR}/*.png")
endif(LMMS_HAVE_GIG)
endif()

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

@DomClark DomClark Aug 19, 2023

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

@Veratil Veratil Aug 19, 2023

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

@DomClark DomClark Aug 19, 2023

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.

const auto buffer = static_cast<int8_t*>(_alloca(allocationsize * sizeof(int8_t)));
#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

@sakertooth sakertooth Jun 29, 2023

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

@Veratil Veratil Jun 29, 2023

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