Add MSVC support for GigPlayer (rework) #7162
Conversation
|
@Monospace-V seems to have a few gig files and is looking into the spec. Can you test this one? |
|
One of my files seems to make it crash for some reason. I may look into it when I have more time, but I have multiple gig files, and my file "Tic Tok Men RetroDrums 1.gig" tends to cause issues if I switch to it after using a different gig file, particularly if I am on a non-zero patch before I switch. In this case LMMS seems to tend to crash. Other than this, at first glance, this seems to support pretty decently. Thank you for altering me to the pr, Rossmaxx. I am simply trying to understand the spec tho I don't actually have much knowledge I'm trying to learn SF2 first but yes. I have a bunch of gig files and this is something I care about |
|
@Monospace-V can you send that file in here, in a zip. |
Unfortunately not. Please find it here: https://www.tictokmen.com/sample/ |
|
@Monospace-V looks like the sample buffer isn't large enough at 512, I will bump this up again. |
|
Oh actually I missed updating the size check (me being dumb and not using a constant) |
|
After bumping the size here there's no error anymore. 👍 |
I'm just wondering what happens if it's even bigger? Like I'm not saying it should be bigger and this might be off topic discussion but is it not possible it could be bigger, or, what stopping it from needing to be larger? Perhaps I don't understand enough. |
It "slows down" from being on the heap now instead of the stack. We're safe from stack overflows in the worst case situations, but we lose the cache locality of the array. The larger the array the more needs to be moved around. The buffer is at 1Kb right now. |
|
Would it be a good idea to create an arbitrarily large buffer but keep a todo comment and cleanup the calculation in a follow up pr (or do a refactor in this pr itself) |
|
There's still some cleanup in this PR left to do. |
There was a problem hiding this comment.
This review is just of the CMake, as it sounds like the C++ changes are still in progress.
The find module isn't working on non-MSVC configurations (I've left comments on it as to why), but this doesn't cause the CI to fail as GIG support is an optional feature. It's out of scope here, but I think we need some way to catch this sort of thing automatically, to avoid problems in the future.
I don't quite understand, I guess I am not quite at that level yet [: It's all right. I plan to test this pr out again later with the rest of my gig files, try to induce anything. I hope I have enough time. I have a few more gig files I want to try, in the event they induce a crash somehow. |
Veratil
force-pushed
the
new-msvc-gigplayer
branch
from
b7a311e
to
b6c78d0
Compare
|
Rebased on master + address Dom's review |
|
I can stil get this to crash using one of my gig files. |
|
Alright, I'll look into them. Thanks! |
|
I confirmed I can't hear anything with patch 4, and patch 5 overloads the bounds of the arrays (by a lot, 38k sized array for example). I'm testing another idea for the buffers. 👍 |
|
@Monospace-V so I've found out that when the buffers are actually sized properly, patch 4 and 5 actually do make sounds. |
|
Well apparently we're not actually C++17 ready on linux and mingw builds. |
|
Looking around, seems like |
|
mingw failures are because the CI running 18.04 has installed libgig-4.0.0, which apparently is right before 4.0.0.svn3 where |
Can't you use an ifdef for now, with a todo comment, atleast to get the CI working. |
The issue is getting the libgig version. There is no define in the code to tell it. |
#7259 should fix this. |
As MinGW libgig is provided by tobydox repo, I'm afraid it won't and doesn't; the highest version there is simply 4.0.0 ; bumping Ubuntu version does help Linux builds w.r.t. this, but not MinGW ones. |
…on is older than where it's defined
Veratil
force-pushed
the
new-msvc-gigplayer
branch
from
2d35a86
to
e424d0c
Compare
|
I am thinking of taking over this PR and fixing MSVC builds. For now, the idea I have is to disable version checking on MSVC and force the new behaviour explicitly (we have libgig 4.4.0 guaranteed so it's fine) |
|
Gig player builds on MSVC now, but there seems to be an error while loading files. I'll look into it. @Veratil can you help me with this? It's still fine if you don't. |
nvm. A clean rebuild fixed it. |
| @@ -440,50 +430,44 @@ void GigInstrument::play( SampleFrame* _working_buffer ) | |||
| samples = frames / freq_factor + Sample::s_interpolationMargins[m_interpolation]; | |||
| } | |||
|
|
|||
| const auto neededCapacity = static_cast<std::vector<SampleFrame>::size_type>(samples); | |||
There was a problem hiding this comment.
samples is already std::size_t so it looks like there's no need for the static cast. And the neededCapacity variable may be unnecessary if you can just use samples.
| unsigned long allocationsize = samples * sample.sample->FrameSize; | ||
| int8_t buffer[allocationsize]; | ||
| const auto allocationSize = samples * sample.sample->FrameSize; | ||
| const auto neededCapacity = static_cast<std::vector<int8_t>::size_type>(allocationSize); |
There was a problem hiding this comment.
allocationSize is already std::size_t so it looks like there's no need for the static cast. And the neededCapacity variable may be unnecessary if you can just use allocationSize.
There was a problem hiding this comment.
I would defer this point to @Veratil whenever he returns as I don't know about the inner workings.
| /* .data_in */ reinterpret_cast<const float*>(oldBuf.data()), | ||
| /* .data_out */ reinterpret_cast<float*>(newBuf.data()), |
There was a problem hiding this comment.
| /* .data_in */ reinterpret_cast<const float*>(oldBuf.data()), | |
| /* .data_out */ reinterpret_cast<float*>(newBuf.data()), | |
| /* .data_in */ oldBuf[0].data(), | |
| /* .data_out */ newBuf[0].data(), |
I believe this is possible now
There was a problem hiding this comment.
I don't understand this too
Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>
|
|
||
| // Delete ended notes (either in the completed state or all the samples ended) | ||
| if( it->state == GigState::Completed || it->samples.empty() ) | ||
| // TODO: C++20 std::erase_if |
There was a problem hiding this comment.
Yes, the member function erase_if will be available, and in GCC 10 the free function will also be available.
|
Files don't seem to load properly on msvc now. |
| @@ -3,15 +3,34 @@ if(LMMS_HAVE_GIG) | |||
| include_directories(SYSTEM ${GIG_INCLUDE_DIRS}) | |||
| SET(CMAKE_AUTOUIC ON) | |||
|
|
|||
| # Version checking logic disabled for msvc as vcpkg guarantees gig > 4.3 | |||
There was a problem hiding this comment.
I think it would be better to do version checking unconditionally as a sanity check. It's less complicated that way, makes fewer assumptions about our CI, and would catch any future mistakes.
Plus, if someone tried to build lmms on their own with MSVC, we'd want it to detect if they had an unsupported libgig version.
There was a problem hiding this comment.
The version checking is broken on MSVC and causes a build fail, that's why I brought in this workaround.
Updated form of #6595 as there are too many conflicts to try and resolve in that branch now (I tried for ~1 hour).
Notes:
std::array, in the event a Giga sample is loaded which will overload any buffer astd::runtime_errorwill be thrown; not the best solution but worked the few test gig files I have at least