stb_vorbis: fix interleaved appends silence at end

[ericoporto]

fix #92

alternative to #95.

[sezero]

CC: @Wohlstand

[icculus]

I'm fine with this; if no one has an objection (@Wohlstand?), let's merge it.

[icculus]

We should also probably point Sean Barrett at this if it's useful in a general sense to stb_vorbis.

[sezero]

We should also probably point Sean Barrett at this if it's useful in a general sense to stb_vorbis.

stb_vorbis_stream_length_in_samples is a PR in stb_vorbis not yet merged. Only thing that can be done with mainstream is that maybe @Wohlstand can amend this to his PR if the patch is correct.

[ericoporto]

stb_vorbis_stream_length_in_samples is a PR in stb_vorbis not yet merged

Just for reference, it actually is in other PR that depends on the mentioned PR being merged.

[sezero]

To me, this looks fine -- still waiting @Wohlstand to make a comment.

I guess the same change is needed in stb_vorbis_get_samples_float() and stb_vorbis_get_samples_short*(), no? (Haven't tested..) I'm willing to apply it to my fork after all is cleared (and it will be applied to all users of my fork.)

[Wohlstand]

Hello! I'm not at home yet, yesterday I was very busy, I'll reply as soon as I'll get my home.

[ericoporto]

I guess the same change is needed in stb_vorbis_get_samples_float() and stb_vorbis_get_samples_short*(), no? (Haven't tested..) I'm willing to apply it to my fork after all is cleared (and it will be applied to all users of my fork.)

I don't have an audio file that causes an issue with those for me to test, would have to come up with something in Audacity for this, the one in the issue was from the user of ags that noticed the issue and reported.

[Wohlstand]

To me, this looks fine -- still waiting @Wohlstand to make a comment.

I guess the same change is needed in stb_vorbis_get_samples_float() and stb_vorbis_get_samples_short*(), no? (Haven't tested..) I'm willing to apply it to my fork after all is cleared (and it will be applied to all users of my fork.)

Yea, I took a small look, and ye, as you said, the change is needed not just in one interleaved function: this problem exists in all other functions too. Mainstream Mixer as well as my MixerX fork doesn't use interleaved function (except of my experimental "turn into multi-track" feature to interpret extra channels as stereo/mono/etc. concurrent tracks with ability to mute each of them), and I guess, somebody reported this problem at the Mixer too, or I confuse some :thinking:

[Wohlstand]

Also, pay attention that stb_vorbis_stream_length_in_samples function may return the SAMPLE_unknown (which is -1), or 0 (when any error ocurrs), so, be careful!

[sezero]

Any changes to stb_vorbis_get_samples_float_interleaved must actually be applied to stb_vorbis_get_samples_float, stb_vorbis_get_samples_short and stb_vorbis_get_samples_short_interleaved as well.

@ericoporto: Is the following refactored version good? (I build-tested only) @icculus, @Wohlstand: please review.

diff --git a/src/stb_vorbis.h b/src/stb_vorbis.h
index 8686bdf..7d2418d 100644
--- a/src/stb_vorbis.h
+++ b/src/stb_vorbis.h
@@ -5512,6 +5512,21 @@ int stb_vorbis_get_frame_short_interleaved(stb_vorbis *f, int num_c, short *buff
    return len;
 }
 
+static int fixup_current_playback_loc(stb_vorbis *f, int n)
+{
+   unsigned int lgs;
+
+   f->current_playback_loc += n;
+   lgs = stb_vorbis_stream_length_in_samples(f);
+   if (f->current_playback_loc > lgs && lgs > 0 && lgs != SAMPLE_unknown) {
+       int r = n - (f->current_playback_loc - (int)lgs);
+       f->current_playback_loc = lgs;
+       return r;
+   }
+
+   return n;
+}
+
 int stb_vorbis_get_samples_short_interleaved(stb_vorbis *f, int channels, short *buffer, int num_shorts)
 {
    float **outputs;
@@ -5528,8 +5543,7 @@ int stb_vorbis_get_samples_short_interleaved(stb_vorbis *f, int channels, short
       if (n == len) break;
       if (!stb_vorbis_get_frame_float(f, NULL, &outputs)) break;
    }
-   f->current_playback_loc += n;
-   return n;
+   return fixup_current_playback_loc(f, n);
 }
 
 int stb_vorbis_get_samples_short(stb_vorbis *f, int channels, short **buffer, int len)
@@ -5546,8 +5560,7 @@ int stb_vorbis_get_samples_short(stb_vorbis *f, int channels, short **buffer, in
       if (n == len) break;
       if (!stb_vorbis_get_frame_float(f, NULL, &outputs)) break;
    }
-   f->current_playback_loc += n;
-   return n;
+   return fixup_current_playback_loc(f, n);
 }
 
 #ifndef STB_VORBIS_NO_STDIO
@@ -5655,8 +5668,7 @@ int stb_vorbis_get_samples_float_interleaved(stb_vorbis *f, int channels, float
       if (!stb_vorbis_get_frame_float(f, NULL, &outputs))
          break;
    }
-   f->current_playback_loc += n;
-   return n;
+   return fixup_current_playback_loc(f, n);
 }
 
 int stb_vorbis_get_samples_float(stb_vorbis *f, int channels, float **buffer, int num_samples)
@@ -5682,8 +5694,7 @@ int stb_vorbis_get_samples_float(stb_vorbis *f, int channels, float **buffer, in
       if (!stb_vorbis_get_frame_float(f, NULL, &outputs))
          break;
    }
-   f->current_playback_loc += n;
-   return n;
+   return fixup_current_playback_loc(f, n);
 }
 #endif // STB_VORBIS_NO_PULLDATA_API
 

[ericoporto]

@sezero I tested and for the test case of issues I had this does fix it the same. Applied your patch here in the PR and force-pushed.

My test case example I did using my rawdecoder (https://github.com/ericoporto/SDL_sound-1/commit/12fc2961c3cb22e2920b4257b6c4a9edefdcf63a, which requires this audacity import setting).

I also tried to listen a few small ogg files I had here using playsound (only game sfx, nothing fancy), and it appears to still work fine for them too.

[sezero]

@sezero I tested and for the test case of issues I had this does fix it the same.

Good. Waiting an additional green light from @icculus (and one from @Wohlstand wouldn't hurt either.)

My test case example

IIRC it is in #92, yes? Can you attach it here too? (to make the reviewers' job easier.)

[ericoporto]

Sure. Build the rawdecoder from below (or just get this commit https://github.com/ericoporto/SDL_sound-1/commit/12fc2961c3cb22e2920b4257b6c4a9edefdcf63a)

#ifdef _WIN32
#define SDL_MAIN_HANDLED /* this is a console-only app */
#endif
#include <stdio.h>
#include <SDL.h>
#include <SDL_sound.h>

const size_t SampleDefaultBufferSize = 64 * 1024;

int main(int argc, char *argv[])
{
    Sound_Init();

    Sound_Sample * sample = Sound_NewSampleFromFile("VacuumNoise.ogg", NULL, SampleDefaultBufferSize);
    if (!sample)
        return -1;

    FILE *f = fopen("test.raw", "wb");
    if (!f)
    {
        Sound_FreeSample(sample);
        return -2;
    }

    do
    {
        size_t sz = Sound_Decode(sample);
        if (sz > 0)
        {
            fwrite(sample->buffer, 1, sz, f);
        }
    } while ((sample->flags & SOUND_SAMPLEFLAG_EOF) == 0);

    fclose(f);
    Sound_FreeSample(sample);

    return 0;
}

and run it with VacuumNoise.ogg.zip (unzip before). It should generate a test.raw file, which can be imported in Audacity using the settings below

If you don't apply this PR there will be silence in the end of the decoded raw file, if you do apply the patch the file will be properly terminated. An easy way to compare is to add VacuumNoise.ogg file as an additional track in Audacity.

[icculus]

I think this has lived here long enough. If there are other problems, open a new bug report. :)

[sezero]

I guess we should apply this to SDL_mixer, yes?

[sezero]

I guess we should apply this to SDL_mixer, yes?

.. and done.