Add MSVC support for GigPlayer #6595

Veratil · 2022-12-22T20:22:24Z

Add FindGig.cmake
- Tested on Windows 10 with vcpkg (pkg-config not present)
- Tested on Fedora 36 with localinstall rpm's (pkg-config present)
Fixes GigPlayer CMakeLists.txt for MSVC
Patches GigPlayer for MSVC builds
- Tested on Windows 10 (VS 17 2022)
- Tested on Fedora 36 (gcc-12.2.1)

vcpkg port for testing can be found here: https://github.com/Veratil/vcpkg/tree/add-port-libgig

plugins/GigPlayer/CMakeLists.txt

plugins/GigPlayer/GigPlayer.cpp

plugins/GigPlayer/CMakeLists.txt

cmake/modules/FindGig.cmake

DomClark · 2022-12-23T01:37:45Z

cmake/modules/FindGig.cmake

+	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)


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

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

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.

PhysSong · 2023-01-05T01:24:55Z

@Veratil BTW, are you going to submit a PR against the vcpkg upstream?

Veratil · 2023-01-05T02:18:03Z

@Veratil BTW, are you going to submit a PR against the vcpkg upstream?

I'm not too familiar with vcpkg so I was hoping Dom could check out the branch and see if there's anything I missed. You're welcome to do it, too, though. I see vcpkg has a very specific way to add new ports, so I'd have to go through and fix whatever I didn't do properly.

PhysSong · 2023-01-12T10:46:47Z

@Veratil BTW, are you going to submit a PR against the vcpkg upstream?

I'm not too familiar with vcpkg so I was hoping Dom could check out the branch and see if there's anything I missed. You're welcome to do it, too, though. I see vcpkg has a very specific way to add new ports, so I'd have to go through and fix whatever I didn't do properly.

I'm not sure if I'm doing things correctly, but here's what I'm doing: https://github.com/PhysSong/vcpkg/tree/libgig

sakertooth · 2023-06-29T16:12:02Z

plugins/GigPlayer/GigPlayer.cpp

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


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?

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.

Rossmaxx · 2023-08-15T06:49:11Z

What's the state of this pr now? Is the gig port going to vcpkg or would it be fine enough to submodule it and get it merged, then followed up by a submodule removal pr when libgig becomes available on vcpkg.

Veratil · 2023-08-15T14:32:06Z

What's the state of this pr now? Is the gig port going to vcpkg or would it be fine enough to submodule it and get it merged, then followed up by a submodule removal pr when libgig becomes available on vcpkg.

libgig still needs to be PR'd to vcpkg, then this could be merged. I just haven't done it.

Rossmaxx · 2023-08-15T14:41:17Z

libgig still needs to be PR'd to vcpkg, then this could be merged. I just haven't done it.

Is it ok if I open the pr before you?

Veratil · 2023-08-15T14:44:25Z

libgig still needs to be PR'd to vcpkg, then this could be merged. I just haven't done it.

Is it ok if I open the pr before you?

Feel free to.

Rossmaxx · 2023-08-18T11:46:54Z

PR opened upstream here

DomClark · 2023-08-19T19:14:26Z

plugins/GigPlayer/GigPlayer.cpp

 				sampleFrame convertBuf[frames];
+#else
+				const auto convertBuf = static_cast<sampleFrame*>(_alloca(frames * sizeof(sampleFrame)));
+#endif


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

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.

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.

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.

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.

DomClark · 2023-08-19T19:18:47Z

cmake/modules/FindGig.cmake

@@ -0,0 +1,35 @@
+


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

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

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.

Rossmaxx · 2023-08-20T11:31:07Z

Forgot to mention, it would be good to add libgig to the ci dependency list once vcpkg merges the port.

Rossmaxx · 2023-09-01T09:46:12Z

After some testing, I found that libgig crashes on release builds but not on debug builds. I have such instances of crashes with other places too but might as well mention this here because i could get lmms to crash by opening a gig file on gig player using a release build.

edit : saw dom's comment on a line. Is it related?

DomClark · 2023-09-01T16:52:19Z

After some testing, I found that libgig crashes [...] Saw dom's comment on a line. Is it related?

Short of addressing the comment and seeing if that fixes it, there's no way to tell unless you can get a backtrace. If you need help with this, ask in #dev-support in Discord.

Rossmaxx · 2023-09-10T03:53:35Z

happened to reproduce the crash again and the prompt i'm getting is noteEnd() triggered without a corresponding activate()!

steps to reproduce:

drop a gig player on song editor and open a gig file with it. (it crashes here if built with release or relwithdebinfo)
drop a few notes.
delete the track (crashes here if debug mode)

Rossmaxx · 2023-10-10T03:09:48Z

Forgot to mention, it would be good to add libgig to the ci dependency list once vcpkg merges the port.

Update: merged.

happened to reproduce the crash again and the prompt i'm getting is noteEnd() triggered without a corresponding activate()!

Not related. Need to investigate the crash again.

Inviting @superpaik for testing.

Veratil · 2023-10-10T03:47:44Z

the prompt i'm getting is noteEnd() triggered without a corresponding activate()!

This is indeed unrelated, and a result of trying to activate too many notes at the same time afaict. The note start will queue and get dropped then the note end will process and trigger this.

superpaik · 2023-10-10T12:06:38Z

Inviting @superpaik for testing.

It builds and works ok.
Tested on Win10 and MSVC 2019

What I did to build it:

download the vcpkg libgig port from here: https://github.com/Veratil/vcpkg/tree/add-port-libgig
download the PR code
rebuild all

Rossmaxx · 2023-10-10T12:10:12Z

Thanks for testing.

download the vcpkg libgig port from here: https://github.com/Veratil/vcpkg/tree/add-port-libgi

No need anymore, just use standard vcpkg, be sure to git pull from vcpkg. Libgig got merged today.

Also,i noticed a crash while using libgig. Can you reproduce it?

superpaik · 2023-10-10T12:45:09Z

Retested with standard vcpkg, and everything Ok.

Also,i noticed a crash while using libgig. Can you reproduce it?

Yes, it crashes when deleting the track. But I don't think it's related to gigplayer, because it crashes also if you do the same with the tripleOsc.

Rossmaxx · 2023-10-31T16:48:30Z

plugins/GigPlayer/GigPlayer.cpp

 	int8_t buffer[allocationsize];
+#else


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.

Rossmaxx · 2023-11-06T12:43:19Z

libgig-comment.patch

I have a patch here which I did. You can apply this here if you are fine with this.

Veratil · 2024-03-26T00:46:12Z

Closing in favor of #7162

Veratil added 3 commits December 20, 2022 21:12

Support GigPlayer with MSVC

63246ad

Update to build on Linux

1186840

Add back removed libgig name for find_library

a746547

PhysSong reviewed Dec 23, 2022

View reviewed changes

plugins/GigPlayer/CMakeLists.txt Outdated Show resolved Hide resolved

Remove unneeded option

d1f4563

DomClark reviewed Dec 23, 2022

View reviewed changes

Update CMake and add const

8bac268

PhysSong mentioned this pull request Jan 2, 2023

MSVC support #3999

Open

15 tasks

sakertooth reviewed Jun 29, 2023

View reviewed changes

DomClark reviewed Aug 19, 2023

View reviewed changes

Rossmaxx mentioned this pull request Sep 16, 2023

Fix invalid use of iterators in piano roll #6869

Merged

Rossmaxx reviewed Oct 31, 2023

View reviewed changes

Veratil mentioned this pull request Mar 26, 2024

Add MSVC support for GigPlayer (rework) #7162

Open

Veratil closed this Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MSVC support for GigPlayer #6595

Add MSVC support for GigPlayer #6595

Veratil commented Dec 22, 2022

DomClark Dec 23, 2022

Veratil Dec 24, 2022

DomClark Jan 5, 2023

PhysSong commented Jan 5, 2023

Veratil commented Jan 5, 2023

PhysSong commented Jan 12, 2023

sakertooth Jun 29, 2023

Veratil Jun 29, 2023

Rossmaxx commented Aug 15, 2023

Veratil commented Aug 15, 2023

Rossmaxx commented Aug 15, 2023

Veratil commented Aug 15, 2023

Rossmaxx commented Aug 18, 2023

DomClark Aug 19, 2023

Veratil Aug 19, 2023

DomClark Aug 19, 2023

PhysSong Aug 20, 2023 •

edited

Loading

Rossmaxx Oct 31, 2023 •

edited

Loading

DomClark Aug 19, 2023

Veratil Aug 19, 2023

DomClark Aug 19, 2023

Rossmaxx commented Aug 20, 2023 •

edited

Loading

Rossmaxx commented Sep 1, 2023 •

edited

Loading

DomClark commented Sep 1, 2023

Rossmaxx commented Sep 10, 2023 •

edited

Loading

Rossmaxx commented Oct 10, 2023 •

edited

Loading

Veratil commented Oct 10, 2023

superpaik commented Oct 10, 2023 •

edited

Loading

Rossmaxx commented Oct 10, 2023 •

edited

Loading

superpaik commented Oct 10, 2023 •

edited

Loading

Rossmaxx Oct 31, 2023 •

edited

Loading

Rossmaxx commented Nov 6, 2023

Veratil commented Mar 26, 2024

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

Add MSVC support for GigPlayer #6595

Add MSVC support for GigPlayer #6595

Conversation

Veratil commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong commented Jan 5, 2023

Veratil commented Jan 5, 2023

PhysSong commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rossmaxx commented Aug 15, 2023

Veratil commented Aug 15, 2023

Rossmaxx commented Aug 15, 2023

Veratil commented Aug 15, 2023

Rossmaxx commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong Aug 20, 2023 • edited Loading

Choose a reason for hiding this comment

Rossmaxx Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rossmaxx commented Aug 20, 2023 • edited Loading

Rossmaxx commented Sep 1, 2023 • edited Loading

DomClark commented Sep 1, 2023

Rossmaxx commented Sep 10, 2023 • edited Loading

Rossmaxx commented Oct 10, 2023 • edited Loading

Veratil commented Oct 10, 2023

superpaik commented Oct 10, 2023 • edited Loading

Rossmaxx commented Oct 10, 2023 • edited Loading

superpaik commented Oct 10, 2023 • edited Loading

Rossmaxx Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Rossmaxx commented Nov 6, 2023

Veratil commented Mar 26, 2024

PhysSong Aug 20, 2023 •

edited

Loading

Rossmaxx Oct 31, 2023 •

edited

Loading

Rossmaxx commented Aug 20, 2023 •

edited

Loading

Rossmaxx commented Sep 1, 2023 •

edited

Loading

Rossmaxx commented Sep 10, 2023 •

edited

Loading

Rossmaxx commented Oct 10, 2023 •

edited

Loading

superpaik commented Oct 10, 2023 •

edited

Loading

Rossmaxx commented Oct 10, 2023 •

edited

Loading

superpaik commented Oct 10, 2023 •

edited

Loading

Rossmaxx Oct 31, 2023 •

edited

Loading