GrandOrgue / grandorgue

GrandOrgue software
Other
149 stars 39 forks source link

Not enough samples for a crossfade #1724

Closed eturpault closed 6 months ago

eturpault commented 7 months ago

Now that the attribute Pipe999LoopCrossfadeLength is set by OdfEdit when making conversion from HW to GO (in link with the issue #1612), I get the following error with 5 wav files when loading the Prytanée sample set of Sonus Paradisi : "Not enough samples for a crossfade"

Here are the properties of these 5 wav files and the value Pipe999LoopCrossfadeLength set for them (they are all at 48kHz 24 bits and contain at least 3 loops) :

For example the first file is this one : 061-c#_050.zip

I see in the GO source code that this error is generated in the file GOSoundAudioSection.cpp :

        if (start_seg.start_offset < fade_len)
          throw(wxString) _("Not enough samples for a crossfade");

Is the error triggered because one loop of the wav file is starting too early ? They are all starting after 2 seconds, it is not so early. Else what can be causing that ? I get no more error by reducing the Pipe999LoopCrossfadeLength values of these pipes at 45 for example.

oleg68 commented 6 months ago

@eturpault I cannot reproduce the problem with the sample you provided.

Could you make a test .organ file with only one sample that would reproduce this issue?

eturpault commented 6 months ago

@oleg68 : Here is a mini sample set permitting to reproduce the issue with one sample. In GO settings "Loop loading" must be set to "All loops" and "Attack loading" to All, else the issue is not reproducible. Issue1724_demo.zip

oleg68 commented 6 months ago

This error comes from another sample: 061-c#.wav. It has six loops:

loops

One of them starts with 5265, that is too early, because the crossfade requires 118*48=5664 samples before the loop start point.

I see the following subissues:

  1. The error message does not show the actual sample filename.
  2. If one of the loop is not valid for some reason (ex. not enough samples for the crossfade), GrandOrgue rejects loading the entire pipe. May be ignoring the single loop and continuing loading another loops would be better.

I'm not sure that a crossfade is really necessary for good loops, because the samples near their start should be the same as ones near their end. But, obviously, L3 in this sample is not good enough.

@larspalo what is your opinion about handling loops without enouth samples before it's start position?

larspalo commented 6 months ago

@oleg68 I agree that 1) the actual filename should be written in the message, and 2) that if there is at least one valid loop then the pipe could be loaded anyway just discarding invalid ones, perhaps with a warning that could actually be helpful for sample set producers to correct the loops in the sample before distribution. For me, it's enough if the warning is written only in strict mode.

However, this would of course introduce the need for more logic checking in the sample loading.

oleg68 commented 6 months ago

@larspalo I think Pipe999LoopCrossfadeLength in general is not necessary for correct samples with correct loops. But it may help the ODF writer to compensate loops with bad bounds. But in this case the value of this parameter is applicable only for certain loops, so different loops of one sample should have defferent crossfades, shouldn't they?

We already have Pipe999Loop999Start and Pipe999Loop999End ODF keys that override the loops specified in the .wav file. Should we also introduce the Pipe999Loop999CrossFadeLength ODF key?

eturpault commented 6 months ago

In the HW ODF format, a LoopCrossfadeLength key can be defined for each attack sample and a ReleaseCrossfadeLength key for each release sample. As in GO ODF these values can be defined only at pipe level, OdfEdit translates only the value of the first attack sample in Pipe999LoopCrossfadeLength and the first release sample in Pipe999ReleaseCrossfadeLength. So if new keys can be added, I suggest to add keys Pipe999Attack999LoopCrossFadeLength and Pipe999Release999ReleaseCrossfadeLength, and to use Pipe999LoopCrossfadeLength/Pipe999ReleaseCrossfadeLength only to the first attack/release samples respectively.

oleg68 commented 6 months ago

@eturpault Are you sure that Hauptwerk uses the value OdfEdit translates to ...LoopCrossfadeLength in the same way as GrandOrgue does?

eturpault commented 6 months ago

In Hauptwerk, here are the available keys :

My assumption is that these values can be translated as they are to GrandOrgue keys Pipe999LoopCrossfadeLength / Pipe999ReleaseCrossfadeLength as the naming is very close and the unit is the same (milliseconds).

In the Hauptwerk custom organ design module user guide, it is written this : Whilst the pipe is sounding the sample is played looped, and a phase-aligned cross-fade is performed to the section starting at the release marker when the pipe stops sounding.

eturpault commented 6 months ago

To be exhaustive here are all possible keys to define an attack sample in HW :

LoadSampleRange_StartPositionTypeCode
LoadSampleRange_StartPositionValue
LoadSampleRange_EndPositionTypeCode
LoadSampleRange_EndPositionValue
AttackSelCriteria_HighestVelocity
AttackSelCriteria_MinTimeSincePrevPipeCloseMs
AttackSelCriteria_HighestCtsCtrlValue
LoopCrossfadeLengthInSrcSampleMs

and a release sample :

LoadSampleRange_StartPositionTypeCode
LoadSampleRange_StartPositionValue
LoadSampleRange_EndPositionTypeCode
LoadSampleRange_EndPositionValue
AttackSelCriteria_HighestVelocit
AttackSelCriteria_MinTimeSincePrevPipeCloseMs
AttackSelCriteria_HighestCtsCtrlValue
ScaleAmplitudeAutomatically
DontBypassAmplitudeScalingIfUserDisablesMultipleReleases
PhaseAlignAutomatically
ReleaseCrossfadeLengthMs
ReleaseSelCriteria_HighestVelocity
ReleaseSelCriteria_LatestKeyReleaseTimeMs
ReleaseSelCriteria_HighestCtsCtrlValue
ReleaseSelCriteria_PreferThisRelForAttackID

Currently OdfEdit converts only the following keys : attack : LoopCrossfadeLengthInSrcSampleMs -> Pipe999LoopCrossfadeLength release : ReleaseCrossfadeLengthMs -> Pipe999ReleaseCrossfadeLength ReleaseSelCriteria_LatestKeyReleaseTimeMs -> Pipe999MaxKeyPressTime

oleg68 commented 6 months ago

Cross-fade is a processus that is performed for smooth transition between two different sounds. GrandOrgue use Pipe999LoopCrossfadeLength for transition between the end of loop and start of the same loop when the pipe is played looped. Because the sound at the start and the end are the same, usually no crossfade is necessary for loops.

Whilst the pipe is sounding the sample is played looped, and a phase-aligned cross-fade is performed to the section starting at the release marker when the pipe stops sounding.

But according this text, Hauptwerk does crossfade not for playing the loop but for transition between the loop and the release sample.

eturpault commented 6 months ago

The key LoopCrossfadeLengthInSrcSampleMs looks really to be used for looping management (and it has a naming so close to Pipe999LoopCrossfadeLength), not for release management which is rather tuned by ReleaseCrossfadeLengthMs.

In the Prytanée sample set, the sample 061-c#.wav which is causing an issue has not the key LoopCrossfadeLengthInSrcSampleMs defined (so its value is 0), 118ms is the value of LoopCrossfadeLengthInSrcSampleMs for the other sample 061-c#_050.wav. So having new keys Pipe999Attack999LoopCrossFadeLength / Pipe999Release999ReleaseCrossfadeLength would permit to assign the proper crossfade values to each sample as defined in the HW ODF and avoid the issue reported here.

larspalo commented 6 months ago

But in this case the value of this parameter is applicable only for certain loops, so different loops of one sample should have defferent crossfades, shouldn't they?

@oleg68 Ideally, that's the case, yes. But I think that the suggestion of @eturpault is perfectly good enough and less cumbersome - allow an individual loop crossfade value and release crossfade value for each attack and each release of every pipe if someone really wants to write them. If the samples are prepared really well, then one wouldn't need to set any special values for them at all in the odf, though.

oleg68 commented 6 months ago

@eturpault As was mentioned above there are three different things:

  1. Wrong filename in the error message. It will be fixed as #1755
  2. If one loop is not suitable for the specified crossfade, GrandOrgue refuse to load the pipe at all. https://github.com/oleg68/GrandOrgue-official/tree/bugfix/invalid-loop fixes it: GO start ignoring one loop instead of rejecting the pipe. It will be submitted after approving and merging #1755
  3. Now it is not possible to specify different crossfade lengths for different attack or release samples. I may not provide such capability in 3.13.3, so it was extracted to a separate issue: #1760 and it will be done in 3.14.0.
eturpault commented 6 months ago

Thank you @oleg68 for these enhancements. I will release OdfEdit v2.9 supporting these new keys once GO 3.14.0 will have been released (and with the sort feature that you requested me here https://github.com/GrandOrgue/OdfEdit/issues/28#issuecomment-1848328515).

Can you please confirm my understanding below about the way to use these keys : For first sample :

For next samples :

Pipe999ReleaseCrossfadeLength will be deprecated. Is it correct ?

larspalo commented 6 months ago

Pipe999ReleaseCrossfadeLength will be deprecated. Is it correct ?

No, it should not be deprecated, since a release might perfectly well be loaded in the Pipe999 attack sample without any further notice or specification, this key (Pipe999ReleaseCrossfadeLength) should still be possible to set at main pipe level too. Only the additional releases that are separately specified should have the Pipe999Release999ReleaseCrossfadeLength.

Thus we need for the main pipe number the old:

For additional attacks there should be Pipe999Attack999LoopCrossFadeLength available that override main pipe value. EDIT: As releases can equally well be loaded from the additional attacks too without separate specifications, we'll also need a Pipe999Attack999ReleaseCrossfadeLength option for them...

For additional releases there should be Pipe999Release999ReleaseCrossfadeLength available that override main pipe value.

eturpault commented 6 months ago

Ok thanks @larspalo. What you call "main pipe" is the first attack sample specified by Pipe999, isn't it ?

larspalo commented 6 months ago

@eturpault Yes. Also, I edited above that the additional attacks can also load releases without separate specifications.