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

FR (API): NF_GetMediaItemMaxPeak should return position #953

Closed
X-Raym opened this issue Feb 27, 2018 · 18 comments
Closed

FR (API): NF_GetMediaItemMaxPeak should return position #953

X-Raym opened this issue Feb 27, 2018 · 18 comments
Assignees

Comments

@X-Raym
Copy link
Contributor

@X-Raym X-Raym commented Feb 27, 2018

Hi !

Could reaper.NF_GetMediaItemMaxPeak( item ) also return the absolute pos (relative to item left edges) of this peaks ?

NF_AnalyzeTakeLoudness can do that, but when we don't need Loudness, it is a bit overkill (because it display the SWS analyse window and is less performant that this simple peak function).

Cheers !

@nofishonfriday nofishonfriday self-assigned this Mar 2, 2018
@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 4, 2018

Version for testing:

https://www.dropbox.com/s/szk73cd4wb6tzcl/reaper_sws64.dll?dl=1

I didn't want to break scripts already using NF_GetMediaItemMaxPeak() by changing functionality of this function, so I've added NF_GetMediaItemMaxPeakAndMaxPeakPos()

Test code:

reaper.ShowConsoleMsg("")

function msg(m)
  return reaper.ShowConsoleMsg(tostring(m) .. "\n")
end

item = reaper.GetSelectedMediaItem(0, 0)
maxPeak, maxPeakPos = reaper.NF_GetMediaItemMaxPeakAndMaxPeakPos(item)

msg("max peak: " .. maxPeak)
msg("max peak pos: " .. maxPeakPos)

-- set edit cursor to max peak pos,
-- should match with "SWS: Move cursor to item peak sample"
reaper.SetEditCurPos(maxPeakPos, true, false);

Btw.

because it display the SWS analyse window

NF_GetMediaItemMaxPeak() also always did display the analyze window. :P (Sorry, nothing I can do about it.)
But it should at least be quite faster getting the peak pos. with it now than using the NF_AnalyzeTakeLoudness() for it. :)

Let me know how it goes with testing.

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 4, 2018

@nofishonfriday thanks for taking a look !

Well, It doesn't seems to give the same results as the Loudness API function:

demo

NF_GetMediaItemMaxPeak() also always did display the analyze window.

How come ? reaper.NF_GetMediaItemMaxPeak( item ) doesn't return if I remember correctly ?

Anyway, it is several time faster than the Loudness function indeed !

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 4, 2018

I think the difference in results comes because the Loudness function uses true peaks measurement while GetMediaItemMaxPeak doesn't.
You can read about true peak measurement here:
https://tech.ebu.ch/docs/tech/tech3341.pdf

In short: True peak also takes waveform 'overshoots' into account, so it's more exact with the drawback that it's also slower (since it uses oversampling).
If you compare the return values of both functions, the Loudness function should always return a true peak value >= the GetMediaItemMaxPeak function (edit: Actually I think it's always > in (almost ?) all cases), which means, positions of peak and true peak can also be different.

I just tried with a random audio item:

max peak: 0.15545237637443
max peak pos: 1670.6489488957
true peak: 0.19827933103314
true peak pos.: 301.60463718821

How come ? reaper.NF_GetMediaItemMaxPeak( item ) doesn't return if I remember correctly ?

If by 'doesn't return' you mean doesn't open the analyze window, here's the discussion we had about it. 🗡️

#856

edit:
Hold on, there may be a bug in the truePeakPos, need to check...

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 4, 2018

@nofishonfriday I understand true peaks and max peak isn't the same, but don't you think the Max Peak Pos results I get in my screenshot above looks a bit off ?

In short: True peak also takes waveform 'overshoots' into account, so it's more exact with the drawback that it's also slower (since it uses oversampling).

In that case, it appears that True Peaks will be more useful than max peak then 🐍

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 4, 2018

You're right, as said above, there's definitely a bug currently, sorry.
New version coming in some minutes...

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 4, 2018

New version:
https://www.dropbox.com/s/szk73cd4wb6tzcl/reaper_sws64.dll?dl=1

TruePeakPos returned wrong results when item start not at 0.0, as in your gif.
This version should now give more sensible results.

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 4, 2018

@nofishonfriday this new version has the same problem with the TruePeaks and MaxPeaks function: the value isn't the same (not in absolute or not even in relative) if items position change:

bug

EDIT: I confirm NF_AnalyzeTakeLoudness is broken in last build (from your comment), previous behavior was consistent results no matter the item the position.

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Which version you mean worked correctly ?
The official v2.9.7, and the first build I posted here broke it ? Or did I misunderstand ?

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 5, 2018

#953 (comment) this one is broken.

I think the working version I restored is the actual public 2.9.7 (I'm not extra sure, I have a lots of SWS dll files now ^^)

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Just to be clear, truePeakPos and maxPeakPos are supposed to give the absolute time counting from project start.
So I'm currently wondering is that a valid test what you're doing with setting snap offset = truePeakPos in your above gif, as snap offset is defined relative from item start, no ?
But maybe I'm confused now...

My test procedure is as follows:

item = reaper.GetSelectedMediaItem(0, 0)
take = reaper.GetActiveTake(item)
_, _, _, truePeak, truePeakPos, shortTermMax, momentaryMax, shortTermMaxPos, momentaryMaxPos = reaper.NF_AnalyzeTakeLoudness2(take, true)
maxPeak, maxPeakPos = reaper.NF_GetMediaItemMaxPeakAndMaxPeakPos(item)
reaper.SetEditCurPos(truePeakPos, true, false)
-- reaper.SetEditCurPos(maxPeakPos, true, false)
-- insert a stretch marker to mark this position
reaper.Main_OnCommand(41842, 0) -- Item: Add stretch marker at cursor

Now move item somewhere else and run script again, cursor should land on same position in item
This works here for both maxPeakPos and truePeakPos with the latest build I posted here, so I'm wondering where the flaw is ?

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 5, 2018

snap offset is defined relative from item start, no ?

Indeed.

truePeakPos [is] supposed to give the absolute time counting from project start.

Actually, it is not the behavior I see in public 2.9.7. It works relative there. And I like it this way, I think it make more sense as it is a extrapolated property of the item.

maxPeakPos should also be relative.

It seems it breaks right from #953 (comment) test release.

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Ah now we're getting somewhere. :D
You're abslolutely right, I actually never intended it to work relative and was unaware that it did in the public 2.9.7, so when I discovered it shortly ago I regarded it as bug and thought I fix immediately to work absolute haha.
Ok, so I think the conclusion is the new GetMediaItemMaxPeakAndMaxPeakPos (which currently works absolute) should also work relative for consistency (and Loudness reverted to relative)?

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Next try, both should work relative (again) now:
https://www.dropbox.com/s/szk73cd4wb6tzcl/reaper_sws64.dll?dl=1

Code I'm using for testing:

reaper.ShowConsoleMsg("")

function msg(m)
  return reaper.ShowConsoleMsg(tostring(m) .. "\n")
end

item = reaper.GetSelectedMediaItem(0, 0)
take = reaper.GetActiveTake(item)

_, _, _, truePeak, truePeakPos, _, _, _, _ = reaper.NF_AnalyzeTakeLoudness2(take, true)
maxPeak, maxPeakPos = reaper.NF_GetMediaItemMaxPeakAndMaxPeakPos(item)

msg("max peak: " .. maxPeak)
msg("rel. max peak pos: " .. maxPeakPos)
msg("true peak: " .. truePeak)
msg("rel. true peak pos: " .. truePeakPos)

reaper.SetMediaItemInfo_Value(item, "D_SNAPOFFSET", maxPeakPos) -- snapoffset should match with "SWS: Move cursor to item peak sample"
-- reaper.SetMediaItemInfo_Value(item, "D_SNAPOFFSET", truePeakPos) -- snapoffset should match with SWS/BR: Analyze loudness... -> Go to true peak

reaper.UpdateArrange()
@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 5, 2018

@nofishonfriday Ahh that is good !
It works faster than the loudness function, and does works with complex scenarios (rate, stretch markers etc).

demo

Note that this MaxPeaks function is post fades and post envelopes, contrary to the true peaks one.

Fades/Envelopes

This is a significant difference between the two.

Maybe at some point a post fade/ post envelopes NF_AnalyzeTakeLoudness request ticket could be proposed, but this is another subject 😄
(though, it may be good to add specify this behavior in the API doc, at it is not said, contrary to the max peak function for which it is written that it is post fades / post gain / post envelopes etc...).

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Thanks for testing (and the patience to get it right. :D)
Good point btw. seeing the true/max peak positions as property of the item, it makes sense, so having this as relative also makes more sense.

Good point also about the API doc,, I'll add this, thanks.
Looking at here
http://wiki.cockos.com/wiki/index.php/Measure_and_normalize_loudness_with_SWS
it seems it should actually be possible to at least have post envelopes included in the Loudness analyzis as Breeder seems to do it, I may have a look at some point...

@X-Raym
Copy link
Contributor Author

@X-Raym X-Raym commented Mar 5, 2018

@nofishonfriday thansks for your time and determination to get it right :P

I may have a look at some point...

Do you want me to open a new ticket about that ? ^^

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Mar 5, 2018

Yes, probably better so it doesn't get lost.
(As implemented issues such as this one will get auto-closed when merged with Master branch as Tim told me.)

@nofishonfriday
Copy link
Collaborator

@nofishonfriday nofishonfriday commented Feb 6, 2019

addid in v2.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants