View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000761OpenMPTFile Format Supportpublic2016-03-23 19:392016-04-25 00:55
Reporterchris82 Assigned ToSaga Musix  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product VersionOpenMPT 1.25.04.00 / libopenmpt 0.2-beta16 (upgrade first) 
Target VersionOpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first)Fixed in VersionOpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first) 
Summary0000761: Sf2 import( and probably other too) doesn't set c5 speed properly
Description

When importing Soundfonts, the c5speed is totally wrong.
I can't understand what the transpose convoluted conversion is supposed to do, so I rewrote it.
I attached a sample sf2 for you to verify.
I created a diff from git, not sure if it's ok, not an expert of git.

It would be handy to have in general a way of specify which note the sample really is, instead of trying to guess it with the tuning method, some window that just call my method. I am not familiar with the UI code of openMpt.

TagsNo tags attached.
Attached Files
test_violin.zip (193,064 bytes)
dls_fix.diff (3,305 bytes)    
commit 3bd99f6e086a1c876d8b9b85061ed4452c850dde
Author: 00christian00 <00christian00@users.noreply.github.com>
Date:   Fri Apr 1 09:27:22 2016 +0200

    fixed DLS import

diff --git a/soundlib/Dlsbank.cpp b/soundlib/Dlsbank.cpp
index 7cf0140..f82ee45 100644
--- a/soundlib/Dlsbank.cpp
+++ b/soundlib/Dlsbank.cpp
@@ -1583,6 +1583,7 @@ bool CDLSBank::ExtractSample(CSoundFile &sndFile, SAMPLEINDEX nSample, uint32 nI
 			sample.nC5Speed = p->dwSampleRate;
 			sample.RelativeTone = p->byOriginalPitch;
 			sample.nFineTune = p->chPitchCorrection;
+			
 			if (p->szName[0]) memcpy(sndFile.m_szNames[nSample], p->szName, MAX_SAMPLENAME - 1);
 
 
@@ -1632,6 +1633,7 @@ bool CDLSBank::ExtractSample(CSoundFile &sndFile, SAMPLEINDEX nSample, uint32 nI
 			int sFineTune = pRgn->sFineTune;
 			int lVolume = pRgn->usVolume;
 
+		
 			WSMPCHUNK wsmp;
 			if(!(pRgn->fuOptions & DLSREGION_OVERRIDEWSMP) && wsmpChunk.ReadStructPartial(wsmp))
 			{
@@ -1651,10 +1653,10 @@ bool CDLSBank::ExtractSample(CSoundFile &sndFile, SAMPLEINDEX nSample, uint32 nI
 						sample.nLoopEnd = loop.ulLoopStart + loop.ulLoopLength;
 					}
 				}
-			} else
+			} 
 			if (m_nType & SOUNDBANK_TYPE_SF2)
 			{
-				usUnityNote = (usUnityNote < 0x80) ? usUnityNote : sample.RelativeTone;
+				usUnityNote = sample.RelativeTone;
 				sFineTune += sample.nFineTune;
 			}
 		#ifdef DLSINSTR_LOG
@@ -1666,7 +1668,7 @@ bool CDLSBank::ExtractSample(CSoundFile &sndFile, SAMPLEINDEX nSample, uint32 nI
 								sFineTune+(60 + transpose - usUnityNote)*100);
 			sample.nFineTune = static_cast<int8>(nBaseTune & 0x7F);*/
 			//sample.RelativeTone = sample.RelativeTone -60;
-			sample.RootNoteToFreq(sample.RelativeTone, sample.nFineTune, sample.nC5Speed);
+			sample.nC5Speed=sample.RootNoteToFreq(usUnityNote, sFineTune, sample.nC5Speed);
 			if (lVolume > 256) lVolume = 256;
 			if (lVolume < 16) lVolume = 16;
 			sample.nGlobalVol = (uint8)(lVolume / 4);	// 0-64
diff --git a/soundlib/ModSample.cpp b/soundlib/ModSample.cpp
index 09d4bb2..f6e0ea6 100644
--- a/soundlib/ModSample.cpp
+++ b/soundlib/ModSample.cpp
@@ -314,13 +314,13 @@ void ModSample::TransposeToFrequency()
 	nC5Speed = TransposeToFrequency(RelativeTone, nFineTune);
 }
 
-void ModSample::RootNoteToFreq(int Root, int finetune, int SampleFreq)
+uint32 ModSample::RootNoteToFreq(int Root, int finetune, int SampleFreq)
 //------------------------------------
 {
 	int Relative = 60 -Root;
 	float steps = (Relative * 128.0f + nFineTune) * (1.0f / (12.0f * 128.0f));
 	//nC5Speed = Util::Round<uint32>(std::pow(2.0f, (RelativeTone * 128.0f + nFineTune) * (1.0f / (12.0f * 128.0f))) * 440.0f);;
-		nC5Speed = Util::Round<uint32>(std::pow(2.0f, steps) * SampleFreq);;
+		return Util::Round<uint32>(std::pow(2.0f, steps) * SampleFreq);;
 
 }
 
diff --git a/soundlib/ModSample.h b/soundlib/ModSample.h
index e69b4c1..02590dc 100644
--- a/soundlib/ModSample.h
+++ b/soundlib/ModSample.h
@@ -95,7 +95,7 @@ struct ModSample
 	// Transpose <-> Frequency conversions
 	static uint32 TransposeToFrequency(int transpose, int finetune = 0);
 	void TransposeToFrequency();
-	void RootNoteToFreq(int Root, int finetune, int SampleFreq);
+	uint32 RootNoteToFreq(int Root, int finetune, int SampleFreq);
 	static int FrequencyToTranspose(uint32 freq);
 	void FrequencyToTranspose();
dls_fix.diff (3,305 bytes)   
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Relationships

has duplicate 0000679 closedSaga Musix Soundfont Transposer 
has duplicate 0000767 closed Soundfont Compatability 

Activities

Saga Musix

Saga Musix

2016-03-23 19:58

administrator   ~0002298

Thanks for the patch - It's better than before for some instruments, but it's still not quite correct sadly (see e.g. instrument 107 / Koto in SGM-180).

It would be handy to have in general a way of specify which note the sample really is, instead of trying to guess it with the tuning method

I don't see how that would help - the real problem is that OpenMPT does not compute the sample frequency correct for SF2 samples, and the answer to this is not to let the user do the work but to implement it correctly instead.
chris82

chris82

2016-03-23 20:05

reporter   ~0002301

I'll have a look at that sf2, strange that doesn't work.

As for the the latter, i meant generally as a sample loading, not as sf2 loading.
When you load a generic sample, you can just specify the note it represent if you know it, and have it compute the c5 speed from the note.
Saga Musix

Saga Musix

2016-03-23 20:09

administrator   ~0002302

When you load a generic sample, you can just specify the note it represent if you know it, and have it compute the c5 speed from the note.

Are you aware that there is already a sample tuner? Click the pitch fork icon. ;) This is more effective than "knowing" the correct note most of the time.
chris82

chris82

2016-03-23 20:12

reporter   ~0002303

Yes, that's the one I was referring before that's not precise :P
Try it on the samples loaded from the soundfont I gave you for example.
If you know the exact note, why guess it?

Btw, I can only find SGM 2, is it the same? SGM 180 is not available anymore.
Saga Musix

Saga Musix

2016-03-23 20:23

administrator   ~0002305

It's not the same file. You can download SGM-180 here: ftp://ftp.untergrund.net/users/sagamusix/samples/SGM-180%20v1.5.sf2.7z

If you know the exact note, why guess it?

Because you still need to know at which sample rate it is that note? When loading a soundfont or similar, you rarely know the correct note and sample rate - if you did, you would probably already have set the sample up correctly!
chris82

chris82

2016-03-23 20:28

reporter   ~0002306

Maybe I am not explaining well.
Let's say you have a sample as a normal wav file, as you can find tons everywhere.
You load it in OpenMpt and you have to tune it and guess the c5 frequency.
But you know that's actually a C3, and the wav is recorded at 44100 hz(you know this, it's in the wav or any other audio format header), so you can just input C3 and let openMpt compute the c5speed exactly.
Saga Musix

Saga Musix

2016-03-23 20:30

administrator   ~0002307

If you know it was recorded at 44100 Hz, why don't you enter 44100 Hz into the sample rate field then? The only situation in which no guesswork is necessary is when you already know the correct sample rate!
chris82

chris82

2016-03-23 20:35

reporter   ~0002308

Cause if you input 44100 there, to play that sample you must play C5 and not the note the sample represent.

Also If you load another sample for the same instrument then C5 is already taken.
chris82

chris82

2016-03-23 20:39

reporter   ~0002309

by the way, how do I chose which instrument load from an sf2 with multiple instruments inside?
I can load it in the midi library but I don't know how to use them unless I open a midi.
Saga Musix

Saga Musix

2016-03-23 20:42

administrator   ~0002310

Cause if you input 44100 there, to play that sample you must play C5 and not the note the sample represent.

So you are talking about multi-sample instruments, in which case we're back to square one: Either it's a broken SF2 import, in which case we want to fix the import frequency, or all the samples are already correctly tuned. The sample tuner only fails with those SF2 samples because their base frequency is already way off.

by the way, how do I chose which instrument load from an sf2 with multiple instruments inside?

You either drag it onto the tree view, or you right-click the tree view and choose "add sound bank".
chris82

chris82

2016-03-23 21:47

reporter   ~0002311

I had a look at SGM 180, it's not reporting the correct root note, so In that case you are right you need to guess it.
Saga Musix

Saga Musix

2016-03-23 22:57

administrator   ~0002312

Well, there should be no need for guessing - the soundfont works as intended in proper soundfont players. However, my knowledge of SF2 is minimal and I do not have any intention to broaden it to fix those issues myself really.
chris82

chris82

2016-04-01 07:33

reporter   ~0002315

Seem I had broken DLS import with the patch, I attached the new version with the fix.
Saga Musix

Saga Musix

2016-04-02 01:09

administrator   ~0002316

Yes, I noticed that too, but it probably doesn't matter because the fix, as it is, still breaks other soundfonts.
As far as I can see, right now the line that either breaks a soundfont or not is:
usUnityNote = (usUnityNote < 0x80) ? usUnityNote : sample.RelativeTone;
Changing it to
usUnityNote = sample.RelativeTone;
works for some instruments and doesn't for others. It would be necessary to figure out when this is the case.
Saga Musix

Saga Musix

2016-04-02 01:16

administrator   ~0002317

Okay, actually it was easier than that - r1287 broke SF2 handling because FileReader::ReadStructPartial was successful even when it shouldn't be, hence some code that should only execute in the DLS case also was applied to SF2 files. Now all SF2 files should be tuned correctly again.
chris82

chris82

2016-04-02 07:49

reporter   ~0002318

I was wondering why it was calling that when the result was obviously wrong.
Btw, I noticed you reverted to the old way of converting c5 speed, I'd rather keep my version in my local repo since it's more accurate due to less conversions.
Saga Musix

Saga Musix

2016-04-02 12:21

administrator   ~0002319

Last edited: 2016-04-02 12:22

I'll look into understanding it better, but did you actually encounter any practical problems (i.e. tuning issues) from these conversions?

BTW, since this code is only required for DLS/SF2 import, it really shouldn't be part of struct ModSample. It's not relevant for the module playing parts of (lib)openmpt.
chris82

chris82

2016-04-02 17:12

reporter   ~0002320

Not yet, but didn't test much.
In my test there was around 150 hz difference from the real sample rate, so I don't know if in some case it can be greater or it's a constant delta.
Saga Musix

Saga Musix

2016-04-02 17:16

administrator   ~0002321

Well, if anything, your current code will break drums because, as far as I can see, it completely ignored the transpose parameter. :\
Saga Musix

Saga Musix

2016-04-07 17:49

administrator   ~0002324

The new frequency calculation code (with minor fixes) is now applied in r6231.

Issue History

Date Modified Username Field Change
2016-03-23 19:39 chris82 New Issue
2016-03-23 19:39 chris82 File Added: test_violin.zip
2016-03-23 19:58 Saga Musix Note Added: 0002298
2016-03-23 20:05 chris82 Note Added: 0002301
2016-03-23 20:09 Saga Musix Note Added: 0002302
2016-03-23 20:12 chris82 Note Added: 0002303
2016-03-23 20:23 Saga Musix Note Added: 0002305
2016-03-23 20:28 chris82 Note Added: 0002306
2016-03-23 20:30 Saga Musix Note Added: 0002307
2016-03-23 20:35 chris82 Note Added: 0002308
2016-03-23 20:39 chris82 Note Added: 0002309
2016-03-23 20:42 Saga Musix Note Added: 0002310
2016-03-23 21:47 chris82 Note Added: 0002311
2016-03-23 22:57 Saga Musix Note Added: 0002312
2016-03-24 16:00 Saga Musix Assigned To => Saga Musix
2016-03-24 16:00 Saga Musix Status new => assigned
2016-04-01 07:33 chris82 File Added: dls_fix.diff
2016-04-01 07:33 chris82 Note Added: 0002315
2016-04-02 01:09 Saga Musix Note Added: 0002316
2016-04-02 01:16 Saga Musix Note Added: 0002317
2016-04-02 01:16 Saga Musix Status assigned => feedback
2016-04-02 07:49 chris82 Note Added: 0002318
2016-04-02 07:49 chris82 Status feedback => assigned
2016-04-02 12:21 Saga Musix Note Added: 0002319
2016-04-02 12:21 Saga Musix Status assigned => feedback
2016-04-02 12:21 Saga Musix Target Version => OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first)
2016-04-02 12:22 Saga Musix Note Edited: 0002319
2016-04-02 17:12 chris82 Note Added: 0002320
2016-04-02 17:12 chris82 Status feedback => assigned
2016-04-02 17:16 Saga Musix Note Added: 0002321
2016-04-07 17:49 Saga Musix Note Added: 0002324
2016-04-07 17:49 Saga Musix Status assigned => resolved
2016-04-07 17:49 Saga Musix Resolution open => fixed
2016-04-07 17:49 Saga Musix Fixed in Version => OpenMPT 1.26.01.00 / libopenmpt 0.2-beta17 (upgrade first)
2016-04-16 14:50 Saga Musix Relationship added has duplicate 0000679
2016-04-25 00:55 Saga Musix Relationship added has duplicate 0000767