jcsteh / osara

OSARA: Open Source Accessibility for the REAPER Application
GNU General Public License v2.0
127 stars 46 forks source link

Advanced Peakwatcher Functionality: True Peak, at least, not working #580

Closed Justinmac84 closed 3 years ago

Justinmac84 commented 3 years ago

Hi,

I'm sure this worked back along, but, when I try to set the Peakwatcher to true peak mode, the meters set to 0.0 and stay there.

Scott informs me that OSARA no longer adds a plugin for this advanced functionality because Reaper does it natively, but OSARA still is, although, when you highlight a track, it is not reported as being there. it only shows up when you enter the fx chain window and is undeletable, which would make sense if everything was working.

What's more, both Kenny from Reapermania and John from the Reaper Blog still seem to be adding loudness plugins to their fx chain, so I'm wondering if Reaper really does do this stuff natively or if the plugin is still required.

I'm noticing that there are two loudness plugins, the one that OSARA still automatically adds and one with a chattier name including the types of info it will give you. I was wondering if the plugin that OSARA is using was deprecated?

I'm running the latest version of OSARA and Reaper 6.40 and have done experiments with this setup using a project I was working on and a simpler one, just involving a tone generator with the same results.

Oh yes and, when I tried to switch from true peak back to peak in my mullti-track project, Reaper crashed entirely.

Thanks for any help.

Justinmac84 commented 3 years ago

Hi,

Hopefully this replies to the right place.

Spoke to Robbie, who has diagnosed the problem as being that OSARA is currently incompatible with the post 6.32 version of the loudness meter. As a temporary work around, he has provided the old version of the meter for those requiring it to copy in.

ScottChesworth commented 3 years ago

@RDMurray, can you give us any nudges on what needs to be tweaked?

jcsteh commented 3 years ago

My guess is that Cockos changed the order of parameters or added new ones in the middle. Since OSARA gets the parameters by number, that would break things surely enough. So the parameter numbers in the code will need tweaking again. Blerg.

jcsteh commented 3 years ago

I vaguely recall reading something in the REAPER changelog very recently about being able to get parameters by some sort of unique id in some cases now. If that's a thing, that would make this a lot more reliable. However, I think that might only be internal params like bypass, delta, etc.

jcsteh commented 3 years ago

Ah, TrackFX_GetParamIdent and TrackFX_GetParamFromIdent (new in 6.37).

RDMurray commented 3 years ago

They also added a new parameter "Output values as automation" which has to be set before the output parameters will work.

@jcsteh I am happy to work on this issue if you aren't already.

jcsteh commented 3 years ago

@RDMurray, happy for you to take it if you want to. That said, I was thinking about this more and I'm not sure TrackFX_GetParamFromIdent is going to be worth it here, at least in the short term. We won't want to call that every 30 ms, so that means we'll have to cache the parameter numbers for all the idents we use. The intuitive thing is to use a map, but that means we'll be doing a pointless map lookup for a string every 30 ms. The remaining solution then is to have a bunch of structs containing a string and a parameter number and each parameter type would pass in the appropriate struct. We'd also need to put all of those structs in an array to make it easy to initialise the cache. Something like this:

struct LoudnessMeterParam {
    LoudnessMeterParam(string name): name(name), param(-1) {}
    string name;
    int param;
};

LoudnessMeterParam truePeakEnable("some string");
LoudnessMeterParam truePeakOutput("some other string");
...

LoudnessMeterParam& loudnessMeterParams[] = {
    truePeakEnable, truePeakOutput,
    ...
};

...

void initLoudnessMeterParams() {
    for (auto param: loudnessMeterParams) {
        // Use TrackFX_GetParamFromIdent to set param.param...
    }
}

double getLoudnessMeterParam(MediaTrack* track,
    LoudnessMeterParam& configParam, double configValue,
    LoudnessMeterParam& queryParam
) {
    if (configParam.param == -1) {
        initLoudnessMeterParams();
    }
    ...
}

...

} PW_LEVEL_TYPES[] = {
    ...
        {"true peak dBTP",
        /* getValue */ [](MediaTrack* track, int channel) {
            return getLoudnessMeterParam(track, truePeakEnable, 1.0, truePeakOutput);
        },
...

That seems like a hell of a lot of code to support parameter numbers that hopefully won't change all that often henceforth. Or maybe I'm wrong and they will...

RDMurray commented 3 years ago

@jcsteh Agreed, that does seem like a lot when it might not be needed in the future. It looks like you already have mostly implemented it though, so I don't see the harm if it works.

Another option might be to bundle a renamed copy of the current version of the plugin with Osara. That way it would continue to work in the future if the param numbers change again.

jcsteh commented 3 years ago

@jcsteh Agreed, that does seem like a lot when it might not be needed in the future. It looks like you already have mostly implemented it though, so I don't see the harm if it works.

That's fair. That said, I've sketched the framework, but not the tedium of defining all the individual parameters. :)

I realised we'll probably want two arrays, one for the config parameters and one for the output parameters. That way, we can flip off all the config parameters with a simple loop, then flip the one we want on. The cache init code then just walks both arrays.

Another option might be to bundle a renamed copy of the current version of the plugin with Osara. That way it would continue to work in the future if the param numbers change again.

It is indeed an option, but not one I'm super fond of. I was never fond of bundling the JSFX when we had to do it before.

jcsteh commented 3 years ago

@RDMurray, to clarify, are you still planning on working on this? Otherwise, I'll try to take it when I find some time. At this point, I think I'd take a PR which just fixes the parameter numbers since that's quicker and easier. However, I'd happily take a PR for the more arduous ParamIdent stuff too. :)

RDMurray commented 3 years ago

@jcsteh Yes, I'm happy to do it. How about I just fix the param numbers now, and we can look at it again if the params change in the future.

LeonarddeR commented 3 years ago

Is there anything about trying to do it with the new param ident functions? I see it can be more work, but it will probably fix it for good. Or am I missing something essential here?

jcsteh commented 3 years ago

It's just a tradeoff between time and severity. The ParamIdent solution is more tedious and time consuming and it's possible no one has the time/inclination to work on that right now. Meanwhile, this bug is hurting users now, so we should get it fixed ASAP, and the simpler fix should only take a few minutes.

LeonarddeR commented 3 years ago

I had a look at the several *FX_GetParamIdent versions, simply by changing FX_GetParamName to FX_GetParamIdent on line 650 paramsUi.cpp. This was easy to do as the signatures match.

The param identifiers are just the zero based parameter numbers, except for the standard parameters as @jcsteh pointed out, they have an ident like bypass prefixed with the zero based param number and a colon. Not very helpful for us IMO.

jcsteh commented 3 years ago

Haha. I totally messed this up. I switched paramsUi to use GetParamident too, except I made a copy/paste error and it turns out I was just calling GetParamname. I thought the idents were the parameter names, which would have been more useful, since the numbers might change but the names probably wouldn't. In reality, as you say, the ident stuff is useless to us.