Move sample and loop validation to fluid_sfont

Makes it possible to use the same validation logic for both SF2 and SF3
samples.

src/sfloader/fluid_defsfont.c

@@ -1850,28 +1850,20 @@ fluid_sample_import_sfont(fluid_sample_t* sample, SFSample* sfsample, fluid_defs
   sample->pitchadj = sfsample->pitchadj;
   sample->sampletype = sfsample->sampletype;
 
-  if (sample->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS)
+  if (fluid_sample_validate(sample, sfont->samplesize) == FLUID_FAILED)
   {
-    int ret = fluid_sample_decompress_vorbis(sample);
+      return FLUID_OK;
-    if (sample->data == NULL || ret == FLUID_FAILED)
-    {
-      sample->valid = 0;
-      return ret;
-    }
   }
 
-  if (sample->sampletype & FLUID_SAMPLETYPE_ROM) {
+  if ((sample->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS)
-    sample->valid = 0;
+      && fluid_sample_decompress_vorbis(sample) == FLUID_FAILED)
-    FLUID_LOG(FLUID_WARN, "Ignoring sample '%s': can't use ROM samples", sample->name);
+  {
-  }
+      return FLUID_FAILED;
-  else if (sample->end - sample->start < 8) {
-    sample->valid = 0;
-    FLUID_LOG(FLUID_WARN, "Ignoring sample '%s': too few sample data points", sample->name);
-  }
-  else {
-    sample->valid = TRUE;
   }
 
+  fluid_sample_sanitize_loop(sample, sfont->samplesize);
+
+  sample->valid = TRUE;
+
   return FLUID_OK;
 }
-

src/sfloader/fluid_sffile.c

@@ -274,7 +274,6 @@ static int load_igen(SFData *sf, int size);
 static int load_shdr(SFData *sf, unsigned int size);
 static int fixup_pgen(SFData *sf);
 static int fixup_igen(SFData *sf);
-static int fixup_sample(SFData *sf);
 
 static int chunkid(unsigned int id);
 static int read_listchunk(SFData *sf, SFChunk *chunk);
@@ -490,8 +489,6 @@ static int load_body(SFData *sf, unsigned int size)
         return FALSE;
     if (!fixup_igen(sf))
         return FALSE;
-    if (!fixup_sample(sf))
-        return FALSE;
 
     /* sort preset list by bank, preset # */
     sf->preset = fluid_list_sort(sf->preset, (fluid_compare_func_t)preset_compare_func);
@@ -651,7 +648,7 @@ static int process_sdta(SFData *sf, unsigned int size)
     /* sample data follows */
     sf->samplepos = sf->fcbs->ftell(sf->sffd);
 
-    /* used in fixup_sample() to check validity of sample headers */
+    /* used to check validity of sample headers */
     sf->samplesize = chunk.size;
 
     FSKIP(sf, chunk.size);
@@ -1679,119 +1676,6 @@ static int fixup_igen(SFData *sf)
     return TRUE;
 }
 
-/* Make sure sample start/end and loopstart/loopend pointers are valid */
-static int fixup_sample(SFData *sf)
-{
-    fluid_list_t *p;
-    SFSample *sam;
-    int invalid_loops = FALSE;
-    int invalid_loopstart;
-    int invalid_loopend, loopend_end_mismatch;
-    unsigned int total_bytes = sf->samplesize;
-    unsigned int total_samples = total_bytes / sizeof(short);
-
-    p = sf->sample;
-    while (p)
-    {
-        unsigned int max_end;
-
-        sam = (SFSample *)(p->data);
-
-        /* Standard SoundFont files (SF2) use sample word indices for sample start and end pointers,
-         * but SF3 files with Ogg Vorbis compression use byte indices for start and end. */
-        max_end = (sam->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS) ? total_bytes : total_samples;
-
-        /* ROM samples are unusable for us by definition, so simply ignore them. */
-        if (sam->sampletype & FLUID_SAMPLETYPE_ROM)
-        {
-            sam->start = sam->end = sam->loopstart = sam->loopend = 0;
-            goto next_sample;
-        }
-
-        /* If end is over the sample data chunk or sam start is greater than 4
-         * less than the end (at least 4 samples).
-         *
-         * FIXME: where does this number 4 come from? And do we need a different number for SF3
-         * files?
-         * Maybe we should check for the minimum Ogg Vorbis headers size? */
-        if ((sam->end > max_end) || (sam->start > (sam->end - 4)))
-        {
-            FLUID_LOG(FLUID_WARN, "Sample '%s' start/end file positions are invalid,"
-                                    " disabling and will not be saved",
-                      sam->name);
-            sam->start = sam->end = sam->loopstart = sam->loopend = 0;
-            goto next_sample;
-        }
-
-        /* The SoundFont 2.4 spec defines the loopstart index as the first sample point of the loop
-         */
-        invalid_loopstart = (sam->loopstart < sam->start) || (sam->loopstart >= sam->loopend);
-        /* while loopend is the first point AFTER the last sample of the loop.
-         * this is as it should be. however we cannot be sure whether any of sam.loopend or sam.end
-         * is correct. hours of thinking through this have concluded, that it would be best practice
-         * to mangle with loops as little as necessary by only making sure loopend is within
-         * max_end. incorrect soundfont shall preferably fail loudly. */
-        invalid_loopend = (sam->loopend > max_end) || (sam->loopstart >= sam->loopend);
-
-        loopend_end_mismatch = (sam->loopend > sam->end);
-
-        if (sam->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS)
-        {
-            /*
-             * compressed samples get fixed up after decompression
-             *
-             * however we cant use the logic below, because uncompressed samples are stored in
-             * individual buffers
-             */
-        }
-        else if (invalid_loopstart || invalid_loopend ||
-                 loopend_end_mismatch) /* loop is fowled?? (cluck cluck :) */
-        {
-            /* though illegal, loopend may be set to loopstart to disable loop */
-            /* is it worth informing the user? */
-            invalid_loops |= (sam->loopend != sam->loopstart);
-
-            /* force incorrect loop points into the sample range, ignore padding */
-            if (invalid_loopstart)
-            {
-                FLUID_LOG(FLUID_DBG, "Sample '%s' has unusable loop start '%d',"
-                                       " setting to sample start at '%d'",
-                          sam->name, sam->loopstart, sam->start);
-                sam->loopstart = sam->start;
-            }
-
-            if (invalid_loopend)
-            {
-                FLUID_LOG(FLUID_DBG, "Sample '%s' has unusable loop stop '%d',"
-                                       " setting to sample stop at '%d'",
-                          sam->name, sam->loopend, sam->end);
-                /* since at this time sam->end points after valid sample data (will correct that few
-                 * lines below),
-                 * set loopend to that first invalid sample, since it should never be played, but
-                 * instead the last
-                 * valid sample will be played */
-                sam->loopend = sam->end;
-            }
-            else if (loopend_end_mismatch)
-            {
-                FLUID_LOG(FLUID_DBG, "Sample '%s' has invalid loop stop '%d',"
-                                       " sample stop at '%d', using it anyway",
-                          sam->name, sam->loopend, sam->end);
-            }
-        }
-
-    next_sample:
-        p = fluid_list_next(p);
-    }
-
-    if (invalid_loops)
-    {
-        FLUID_LOG(FLUID_WARN, "Found samples with invalid loops, audible glitches possible.");
-    }
-
-    return TRUE;
-}
-
 static void delete_preset(SFPreset *preset)
 {
     fluid_list_t *entry;

src/sfloader/fluid_sfont.c

@@ -554,6 +554,119 @@ int fluid_sample_set_pitch(fluid_sample_t* sample, int root_key, int fine_tune)
 }
 
 
+/**
+ * Validate parameters of a sample
+ *
+ */
+int fluid_sample_validate(fluid_sample_t *sample, unsigned int buffer_size)
+{
+    /* ROM samples are unusable for us by definition */
+    if (sample->sampletype & FLUID_SAMPLETYPE_ROM)
+    {
+        FLUID_LOG(FLUID_WARN, "Sample '%s': ROM sample ignored", sample->name);
+        return FLUID_FAILED;
+    }
+
+    /* Ogg vorbis compressed samples in the SF3 format use byte indices for
+     * sample start and end pointers before decompression. Standard SF2 samples
+     * use sample word indices for all pointers, so use half the buffer_size
+     * for validation. */
+    if (!(sample->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS))
+    {
+        if (buffer_size % 2)
+        {
+            FLUID_LOG(FLUID_WARN, "Sample '%s': invalid buffer size", sample->name);
+            return FLUID_FAILED;
+        }
+        buffer_size /= 2;
+    }
+
+    if ((sample->end > buffer_size) || (sample->start >= sample->end))
+    {
+        FLUID_LOG(FLUID_WARN, "Sample '%s': invalid start/end file positions", sample->name);
+        return FLUID_FAILED;
+    }
+
+    return TRUE;
+}
+
+/* Check the sample loop pointers and optionally convert them to something
+ * usable in case they are broken. Return a boolean indicating if the pointers
+ * have been modified, so the user can be notified of possible audio glitches.
+ */
+int fluid_sample_sanitize_loop(fluid_sample_t *sample, unsigned int buffer_size)
+{
+    int modified = FALSE;
+    unsigned int max_end = buffer_size / 2;
+    /* In fluid_sample_t the sample end pointer points to the last sample, not
+     * to the data word after the last sample. FIXME: why? */
+    unsigned int sample_end = sample->end + 1;
+
+    /* Checking loops on compressed samples makes no sense at all and is really
+     * a programming error. Disable the loop to be on the safe side. */
+    if (sample->sampletype & FLUID_SAMPLETYPE_OGG_VORBIS)
+    {
+        FLUID_LOG(FLUID_ERR, "Sample '%s': checking loop on compressed sample, disabling loop",
+                sample->name);
+        sample->loopstart = sample->loopend = 0;
+        return TRUE;
+    }
+
+    if (sample->loopstart == sample->loopend)
+    {
+        /* Some SoundFonts disable loops by setting loopstart = loopend. While
+         * technically invalid, we decided to accept those samples anyway. Just
+         * ensure that those two pointers are within the sampledata by setting
+         * them to 0. Don't set modified here, as this change has no audible
+         * effect. */
+        sample->loopstart = sample->loopend = 0;
+    }
+    else if (sample->loopstart > sample->loopend)
+    {
+        unsigned int tmp;
+
+        /* If loop start and end are reversed, try to swap them around and
+         * continue validation */
+        FLUID_LOG(FLUID_DBG, "Sample '%s': reversed loop pointers '%d' - '%d', trying to fix",
+                sample->name, sample->loopstart, sample->loopend);
+        tmp = sample->loopstart;
+        sample->loopstart = sample->loopend;
+        sample->loopend = tmp;
+        modified = TRUE;
+    }
+
+    /* The SoundFont 2.4 spec defines the loopstart index as the first sample
+     * point of the loop while loopend is the first point AFTER the last sample
+     * of the loop. However we cannot be sure whether any of loopend or end is
+     * correct. Hours of thinking through this have concluded that it would be
+     * best practice to mangle with loops as little as necessary by only making
+     * sure the pointers are within sample->start to max_end. Incorrect
+     * soundfont shall preferably fail loudly. */
+    if ((sample->loopstart < sample->start) || (sample->loopstart > max_end))
+    {
+        FLUID_LOG(FLUID_DBG, "Sample '%s': invalid loop start '%d', setting to sample start '%d'",
+                sample->name, sample->loopstart, sample->start);
+        sample->loopstart = sample->start;
+        modified = TRUE;
+    }
+
+    if ((sample->loopend < sample->start) || (sample->loopend > max_end))
+    {
+        FLUID_LOG(FLUID_DBG, "Sample '%s': invalid loop end '%d', setting to sample end '%d'",
+                sample->name, sample->loopend, sample_end);
+        sample->loopend = sample_end;
+        modified = TRUE;
+    }
+
+    if ((sample->loopstart > sample_end) || (sample->loopend > sample_end))
+    {
+        FLUID_LOG(FLUID_DBG, "Sample '%s': loop range '%d - %d' after sample end '%d', using it anyway",
+                sample->name, sample->loopstart, sample->loopend, sample_end);
+    }
+
+    return modified;
+}
+
 #if LIBSNDFILE_SUPPORT
 
 // virtual file access rountines to allow for handling

src/sfloader/fluid_sfont.h

@@ -24,7 +24,10 @@
 
 #include "fluidsynth.h"
 
+FLUIDSYNTH_API int fluid_sample_validate(fluid_sample_t *sample, unsigned int max_end);
+FLUIDSYNTH_API int fluid_sample_sanitize_loop(fluid_sample_t *sample, unsigned int max_end);
 FLUIDSYNTH_API int fluid_sample_decompress_vorbis(fluid_sample_t *sample);
+
 /*
  * Utility macros to access soundfonts, presets, and samples
  */