update valid-soundfont fix after discussion with upstreams:
authormirabilos <tg@debian.org>
Sat, 5 Jun 2021 16:05:12 +0000 (18:05 +0200)
committermirabilos <mirabilos@evolvis.org>
Sat, 5 Jun 2021 16:05:12 +0000 (18:05 +0200)
• change “SoundFont 2.04 spec chapter 7.10 recommendation” warning
  from qWarning to qDebug so it doesn’t show up in upstream mu͒ 3.x
  release builds
• change “loop is fowled??” check-and-fixup code to match recent
  FluidSynth and real-existing soundfonts better:
  – permit loopstart==loopend to avoid looping
    (fix them up as loopstart=loopend=start, so it’s within the
    sample always) which FluidSynth can do, even here:
fluid/voice.cpp:#define FLUID_MIN_LOOP_SIZE 2
fluid/voice.cpp:#define FLUID_MIN_LOOP_PAD  0
  – permit loopstart==start since FLUID_MIN_LOOP_PAD is 0 and
    this is used but do ensure loopstart and loopend are both
    within the sample, at least (and warn)
  – if loopstart < loopend, swap them around, with warning
  – stop forcibly padding the loop when not necessary, fixes
    playback of some bad/old samples
• more unsigned int (XXX should be size_t?) for sample offsets

debian/changelog
debian/patches/experiments/valid-soundfont.diff

index 8201a66..36e6e5f 100644 (file)
@@ -2,7 +2,7 @@ musescore2 (2.3.2+dfsg4-15) unstable; urgency=medium
 
   * Fix soundfont-related crashes or audible artefacts (Closes: #985129)
 
- -- Thorsten Glaser <tg@mirbsd.de>  Sun, 14 Mar 2021 18:17:41 +0100
+ -- Thorsten Glaser <tg@mirbsd.de>  Sat, 05 Jun 2021 18:04:54 +0200
 
 musescore2 (2.3.2+dfsg4-14) unstable; urgency=medium
 
index da4d9e1..280aa80 100644 (file)
@@ -4,7 +4,9 @@ Description: Fix multiple possible causes of crashes or audible artefacts
  - Mark sample as invalid if Ogg Vorbis decompression (SF3) fails
  - Do all sanity checks on {,loop}{start,end} with SF2 semantics for end;
    only switch end to point to the last sample afterwards in only one place
- - Add sanity check provided by the SoundFont spec as extra warning
+ - Adapt sanity checks and corrections to current FluidSynth, which matches
+   real-existing soundfonts better
+ - Add sanity check provided by the SoundFont spec as extra diagnostic
  - Do not crash if there is no data[]
  - Issue diagnostics if disabling a sample
  - Swap two members to improve structure packing/alignment while there
@@ -72,7 +74,7 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
 +      // now add some extra sanity checks from the SF2 spec (biased so they work with unsigned int);
 +      // just a warning, they probably work with Fluid, possibly with audible artefacts though...
 +      if (!(start + 7 < loopstart) || !(loopstart + 31 < loopend) || !(loopend + 7 < end))
-+            qWarning("SoundFont(%s) Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) smaller than SoundFont 2.04 spec chapter 7.10 recommendation",
++            qDebug("SoundFont(%s) Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) smaller than SoundFont 2.04 spec chapter 7.10 recommendation",
 +                     qPrintable(sf->get_name()), name, start, loopstart, loopend, end);
 +
 +      // this used to cause a crash
@@ -108,7 +110,7 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
              READD (p->start);
              READD (p->end);         /* - end, loopstart and loopend */
              READD (p->loopstart);     /* - will be checked and turned into */
-@@ -1616,16 +1644,19 @@ void SFont::load_shdr (int size)
+@@ -1616,28 +1644,47 @@ void SFont::load_shdr (int size)
                    continue;
                    }
              if ((p->end > getSamplesize()) || (p->start > (p->end - 4))) {
@@ -122,18 +124,46 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
 +                  // done in Sample::decompressOggVorbis in sfont3.cpp
                    }
              else {
-                   // loop is fowled?? (cluck cluck :)
-                   if (p->loopend > p->end || p->loopstart >= p->loopend || p->loopstart <= p->start) {
-+                        qWarning("Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) fowled (broken soundfont?), fixing up",
-+                                 p->name, p->start, p->loopstart, p->loopend, p->end);
-                         /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
-                         if ((p->end - p->start) >= 20) {
-                               p->loopstart = p->start + 8;
-@@ -1636,8 +1667,10 @@ void SFont::load_shdr (int size)
-                               p->loopend   = p->end - 1;
-                               }
+-                  // loop is fowled?? (cluck cluck :)
+-                  if (p->loopend > p->end || p->loopstart >= p->loopend || p->loopstart <= p->start) {
+-                        /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
+-                        if ((p->end - p->start) >= 20) {
+-                              p->loopstart = p->start + 8;
+-                              p->loopend   = p->end - 8;
+-                              }
+-                        else {      // loop is fowled, sample is tiny (can't pad 8 samples)
+-                              p->loopstart = p->start + 1;
+-                              p->loopend   = p->end - 1;
+-                              }
++                  // try to fixup start<loopstart<loopend<end similar to recent FluidSynth;
++                  // some of these are technically invalid but exist in the field; see the
++                  // SoundFont 2.04 spec-related diagnostic about this in sfont.cpp
++                  if (p->loopstart == p->loopend) {
++                        // normalise to ensure they are within the sample, even non-looped
++                        p->loopstart = p->loopend = p->start;
                          }
 -                  if ((p->end - p->start) < 8)
++                  else if (p->loopstart < p->loopend) {
++                        unsigned int looptmp;
++
++                        qWarning("Sample(%s) swapping reversed loopstart<=>loopend",
++                                 p->name);
++                        looptmp = p->loopstart;
++                        p->loopstart = p->loopend;
++                        p->loopend = looptmp;
++                        }
++                  // ensure they are at least within the sample
++                  if (p->loopstart < p->start || p->loopstart > p->end) {
++                        qWarning("Sample(%s) loopstart(%u) out of bounds [start=%u end=%u], setting to start",
++                                 p->name, p->loopstart, p->start, p->end);
++                        p->loopstart = p->start;
++                        }
++                  if (p->loopend < p->start || p->loopend > p->end) {
++                        qWarning("Sample(%s) loopend(%u) out of bounds [start=%u end=%u], setting to end",
++                                 p->name, p->loopend, p->start, p->end);
++                        p->loopstart = p->end;
++                        }
++                  // ensure it can even play
 +                  if ((p->end - p->start) < 8) {
 +                        qWarning("Sample(%s) too small, disabling", p->name);
                          p->setValid(false);
@@ -175,7 +205,7 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
        {
        AudioFile af;
        QByteArray ba(src, size);
-@@ -18,32 +18,37 @@ bool Sample::decompressOggVorbis(char* s
+@@ -18,32 +18,51 @@ bool Sample::decompressOggVorbis(char* s
        start = 0;
        end   = 0;
        if (!af.open(ba)) {
@@ -193,29 +223,49 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
              delete[] data;
              data = 0;
 +            return false;
-             }
--      end = frames - 1;
++            }
 +      // cf. https://musescore.org/en/node/89216#comment-1068379 and following
 +      end = frames;
++
++      // try to fixup start<loopstart<loopend<end similar to recent FluidSynth;
++      // some of these are technically invalid but exist in the field; see the
++      // SoundFont 2.04 spec-related diagnostic about this in sfont.cpp
++      if (loopstart == loopend) {
++            // normalise to ensure they are within the sample, even non-looped
++            loopstart = loopend = start;
+             }
+-      end = frames - 1;
++      else if (loopstart < loopend) {
++            unsigned int looptmp;
  
 -      if (loopend > end ||loopstart >= loopend || loopstart <= start) {
-+      // loop is fowled?? (cluck cluck :)
-+      if (loopend > end || loopstart >= loopend || loopstart <= start) {
-+            qWarning("SoundFont(%s) Sample(%s) start(%u) startloop(%u) endloop(%u) end(%u) fowled (broken soundfont?), fixing up",
-+                     qPrintable(sf->get_name()), name, start, loopstart, loopend, end);
-             /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
-             if ((end - start) >= 20) {
-                   loopstart = start + 8;
+-            /* can pad loop by 8 samples and ensure at least 4 for loop (2*8+4) */
+-            if ((end - start) >= 20) {
+-                  loopstart = start + 8;
 -                  loopend = end - 8;
-+                  loopend   = end - 8;
-                   }
+-                  }
 -            else { // loop is fowled, sample is tiny (can't pad 8 samples)
-+            else {      // loop is fowled, sample is tiny (can't pad 8 samples)
-                   loopstart = start + 1;
+-                  loopstart = start + 1;
 -                  loopend = end - 1;
-+                  loopend   = end - 1;
-                   }
+-                  }
++            qWarning("SoundFont(%s) Sample(%s) swapping reversed loopstart<=>loopend",
++                     qPrintable(sf->get_name()), name);
++            looptmp = loopstart;
++            loopstart = loopend;
++            loopend = looptmp;
              }
++      // ensure they are at least within the sample
++      if (loopstart < start || loopstart > end) {
++            qWarning("SoundFont(%s) Sample(%s) loopstart(%u) out of bounds [start=%u end=%u], setting to start",
++                     qPrintable(sf->get_name()), name, loopstart, start, end);
++            loopstart = start;
++            }
++      if (loopend < start || loopend > end) {
++            qWarning("SoundFont(%s) Sample(%s) loopend(%u) out of bounds [start=%u end=%u], setting to end",
++                     qPrintable(sf->get_name()), name, loopend, start, end);
++            loopstart = end;
++            }
++      // ensure it can even play
        if ((end - start) < 8) {
 -            qDebug("invalid sample");
 -            setValid(false);
@@ -226,6 +276,124 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
        return true;
 --- a/fluid/voice.cpp
 +++ b/fluid/voice.cpp
+@@ -1170,8 +1170,8 @@ void Voice::update_param(int _gen)
+             case GEN_STARTADDRCOARSEOFS:        /* SF2.01 section 8.1.3 # 4 */
+                   if (sample != 0) {
+                         start = (sample->start
+-                               + (int) GEN(GEN_STARTADDROFS)
+-                               + 32768 * (int) GEN(GEN_STARTADDRCOARSEOFS));
++                               + (unsigned int) GEN(GEN_STARTADDROFS)
++                               + 32768U * (unsigned int) GEN(GEN_STARTADDRCOARSEOFS));
+                         check_sample_sanity_flag = FLUID_SAMPLESANITY_CHECK;
+                         }
+                   break;
+@@ -1180,8 +1180,8 @@ void Voice::update_param(int _gen)
+             case GEN_ENDADDRCOARSEOFS:           /* SF2.01 section 8.1.3 # 12 */
+                   if (sample != 0) {
+                         end = (sample->end
+-                               + (int) GEN(GEN_ENDADDROFS)
+-                               + 32768 * (int) GEN(GEN_ENDADDRCOARSEOFS));
++                               + (unsigned int) GEN(GEN_ENDADDROFS)
++                               + 32768U * (unsigned int) GEN(GEN_ENDADDRCOARSEOFS));
+                         check_sample_sanity_flag = FLUID_SAMPLESANITY_CHECK;
+                         }
+                   break;
+@@ -1190,8 +1190,8 @@ void Voice::update_param(int _gen)
+             case GEN_STARTLOOPADDRCOARSEOFS:     /* SF2.01 section 8.1.3 # 45 */
+                   if (sample != 0) {
+                         loopstart = (sample->loopstart
+-                                + (int) GEN(GEN_STARTLOOPADDROFS)
+-                                + 32768 * (int) GEN(GEN_STARTLOOPADDRCOARSEOFS));
++                                + (unsigned int) GEN(GEN_STARTLOOPADDROFS)
++                                + 32768U * (unsigned int) GEN(GEN_STARTLOOPADDRCOARSEOFS));
+                         check_sample_sanity_flag = FLUID_SAMPLESANITY_CHECK;
+                         }
+                   break;
+@@ -1200,8 +1200,8 @@ void Voice::update_param(int _gen)
+             case GEN_ENDLOOPADDRCOARSEOFS:       /* SF2.01 section 8.1.3 # 50 */
+                   if (sample != 0) {
+                         loopend = (sample->loopend
+-                                 + (int) GEN(GEN_ENDLOOPADDROFS)
+-                                 + 32768 * (int) GEN(GEN_ENDLOOPADDRCOARSEOFS));
++                                 + (unsigned int) GEN(GEN_ENDLOOPADDROFS)
++                                 + 32768U * (unsigned int) GEN(GEN_ENDLOOPADDRCOARSEOFS));
+                         check_sample_sanity_flag = FLUID_SAMPLESANITY_CHECK;
+                         }
+                   break;
+@@ -1636,22 +1636,22 @@ float Voice::get_lower_boundary_for_atte
+  */
+ void Voice::check_sample_sanity()
+       {
+-      int min_index_nonloop=(int) sample->start;
+-      int max_index_nonloop=(int) sample->end;
++      unsigned int min_index_nonloop = sample->start;
++      unsigned int max_index_nonloop = sample->end;
+       /* make sure we have enough samples surrounding the loop */
+-      int min_index_loop=(int) sample->start + FLUID_MIN_LOOP_PAD;
+-      int max_index_loop=(int) sample->end - FLUID_MIN_LOOP_PAD;
++      unsigned int min_index_loop = sample->start + FLUID_MIN_LOOP_PAD;
++      unsigned int max_index_loop = sample->end - FLUID_MIN_LOOP_PAD;
+       fluid_check_fpe("voice_check_sample_sanity start");
+       if (!check_sample_sanity_flag)
+             return;
+ #if 0
+-      printf("Sample from %i to %i\n", sample->start, sample->end);
+-      printf("Sample loop from %i %i\n", sample->loopstart, sample->loopend);
+-      printf("Playback from %i to %i\n", start, end);
+-      printf("Playback loop from %i to %i\n", loopstart, loopend);
++      printf("Sample from %u to %u\n", sample->start, sample->end);
++      printf("Sample loop from %u to %u\n", sample->loopstart, sample->loopend);
++      printf("Playback from %u to %u\n", start, end);
++      printf("Playback loop from %u to %u\n", loopstart, loopend);
+ #endif
+       /* Keep the start point within the sample data */
+@@ -1668,7 +1668,7 @@ void Voice::check_sample_sanity()
+       /* Keep start and end point in the right order */
+       if (start > end) {
+-            int temp = start;
++            unsigned int temp = start;
+             start    = end;
+             end      = temp;
+             }
+@@ -1694,7 +1694,7 @@ void Voice::check_sample_sanity()
+             /* Keep loop start and end point in the right order */
+             if (loopstart > loopend){
+-                  int temp  = loopstart;
++                  unsigned int temp  = loopstart;
+                   loopstart = loopend;
+                   loopend   = temp;
+                   }
+@@ -1706,7 +1706,7 @@ void Voice::check_sample_sanity()
+             /* The loop points may have changed. Obtain a new estimate for the loop volume. */
+             /* Is the voice loop within the sample loop?
+              */
+-            if ((int)loopstart >= (int)sample->loopstart && (int)loopend <= (int)sample->loopend){
++            if (loopstart >= sample->loopstart && loopend <= sample->loopend){
+                   /* Is there a valid peak amplitude available for the loop? */
+                   if (sample->amplitude_that_reaches_noise_floor_is_valid) {
+                         amplitude_that_reaches_noise_floor_loop = sample->amplitude_that_reaches_noise_floor;
+@@ -1745,13 +1745,13 @@ void Voice::check_sample_sanity()
+             * the sample, enter the loop and proceed as expected => no
+             * actions required.
+             */
+-            int index_in_sample = phase.index();
++            unsigned int index_in_sample = (unsigned int)phase.index();
+             if (index_in_sample >= loopend) {
+                   /* FLUID_LOG(FLUID_DBG, "Loop / sample sanity check: Phase after 2nd loop point!"); */
+                   phase.setInt(loopstart);
+                   }
+             }
+-/*    FLUID_LOG(FLUID_DBG, "Loop / sample sanity check: Sample from %i to %i, loop from %i to %i", voice->start, voice->end, voice->loopstart, voice->loopend); */
++/*    FLUID_LOG(FLUID_DBG, "Loop / sample sanity check: Sample from %u to %u, loop from %u to %u", voice->start, voice->end, voice->loopstart, voice->loopend); */
+     /* Sample sanity has been assured. Don't check again, until some
+        sample parameter is changed by modulation.
 @@ -1795,7 +1795,6 @@ void Sample::optimize()
        signed short peak;
        float normalized_amplitude_during_loop;
@@ -243,3 +411,20 @@ Forwarded: https://github.com/musescore/MuseScore/pull/7728
                    signed short val = s->data[i];
                    if (val > peak_max)
                          peak_max = val;
+--- a/fluid/voice.h
++++ b/fluid/voice.h
+@@ -134,10 +134,10 @@ public:
+       float root_pitch, root_pitch_hz;
+       /* sample and loop start and end points (offset in sample memory).  */
+-      int start;
+-      int end;
+-      int loopstart;
+-      int loopend;
++      unsigned int start;
++      unsigned int end;
++      unsigned int loopstart;
++      unsigned int loopend;
+       /* vol env */
+       fluid_env_data_t volenv_data[FLUID_VOICE_ENVLAST];