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.

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

Fix #279 - Fail to render when non-VST fx are in fx chains #280

Merged
merged 4 commits into from
Jun 4, 2024

Conversation

firthm01
Copy link
Contributor

@firthm01 firthm01 commented May 21, 2024

Problem is outlined in detail in #279

The fix chosen was to replace some of our custom functions with the built-in Reaper API function for getting original plugin names in cases where plugins may have been renamed in the DAW.
This API function is only supported in REAPER v6.37+ and so the minimum version for the EPS would jump from v6.11 to v6.37. However, v6.37 is now 3 years old and anyone with a license for v6.11 should also be able to upgrade to any v6.xx version so I don't see this causing issues for many (if any) users.

Closes #279

@firthm01 firthm01 requested a review from rsjbailey May 21, 2024 11:07
@firthm01 firthm01 self-assigned this May 21, 2024
@firthm01 firthm01 force-pushed the fix279-actual-fx-name branch from faa8eb9 to aee9fd0 Compare May 21, 2024 14:51
firthm01 added 4 commits May 21, 2024 16:01
v6.37 supports TrackFX_GetNamedConfigParm( track, 0, "fx_name" )
Some earlier versions support TrackFX_GetNamedConfigParm, but not with the "fx_name" arg
Modification is to address this bug in the header: https://forum.cockos.com/showthread.php?p=2548853 (post 4)
@firthm01 firthm01 force-pushed the fix279-actual-fx-name branch from aee9fd0 to b63ba92 Compare May 21, 2024 15:02
@firthm01 firthm01 marked this pull request as ready for review May 21, 2024 17:25
Comment on lines +834 to +835
const size_t fxNameMaxLen = 1024; // Should be plenty
char fxName[fxNameMaxLen];
Copy link
Contributor

@rsjbailey rsjbailey Jun 4, 2024

Choose a reason for hiding this comment

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

Just on 'stackwatch' because of the previous issue - do we call this from anywhere that will be troubled by pushing 1k onto the stack? If so:

const std::size_t fxNameMaxLen = 1023;
// I think the '\0' is superfluous, but it shouldn't hurt.
std::vector<char> fxName(fxNameMaxLen + 1, '\0'); 

if(auto fxNameRes = TrackFX_GetNamedConfigParm(track, fxNum, "fx_name", fxName.data(), fxNameMaxLen)) {
     fxName[fxNameMaxLen] = '\0'; // just in case REAPER is naughty
     name = std::string(fxName.data());
   } else {
     return false;
}

Or if we're doing it a few times

template<typename F>
std::optional<std::string> stringFromAPICall(F&& f, std::size_t maxLength = 1023) 
{
    std::vector<char> buffer(maxLength + 1, '\0');
    if(auto result = f(buffer.data(), maxLength)) {
        buffer[maxLength] = '\0'; 
        return std::string(buffer.data());
    } else { 
        return {}; 
    } 
}

and then

if(auto name = stringFromAPICall([track, fxNum](char* buffer, std::size_t size) {
  return TrackFX_GetNamedConfigParm(track, fxNum, "fx_name", buffer, size);
}) {
    // do something with *name
}

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 think we're safe as the old code used to allocate 64k on the stack for the entire state chunk before parsing it. That said, if you think we should go with the vector for safety anyway, happy to go ahead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, good point. No worries then.

@firthm01 firthm01 merged commit a9e2cfb into main Jun 4, 2024
7 checks passed
@firthm01 firthm01 deleted the fix279-actual-fx-name branch June 4, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue when rendering ADM from a project with JS plugins
2 participants