stb_vorbis: fix interleaved appends silence at end #97

ericoporto · 2024-03-05T00:10:09Z

fix #92

alternative to #95.

sezero · 2024-03-05T00:23:52Z

CC: @Wohlstand

icculus · 2024-03-05T05:00:01Z

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

icculus · 2024-03-05T05:03:15Z

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

sezero · 2024-03-05T07:56:59Z

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 · 2024-03-05T12:06:38Z

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 · 2024-03-06T13:27:50Z

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 · 2024-03-06T13:37:50Z

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

ericoporto · 2024-03-06T14:40:57Z

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 · 2024-03-06T18:03:34Z

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 🤔

Wohlstand · 2024-03-06T18:15:12Z

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!

Wohlstand · 2024-03-06T18:20:07Z

src/stb_vorbis.h

@@ -5656,6 +5657,11 @@ int stb_vorbis_get_samples_float_interleaved(stb_vorbis *f, int channels, float
         break;
   }
   f->current_playback_loc += n;
+   if(f->current_playback_loc > lgs) {


Please check for the lgs >= 0, otherwise, it may return 0 (on any error) or -1 when it has an invalid duration value.

added a && lgs >= 0 check as requested.

Wait a minute, the return value should be positive as is an unsigned int, how could it be -1?

Dropping the previously added commit until a better rationality exists...

I read the code and "-1 when it has an invalid duration value" doesn't exist, it can only be 0. Using lgs > 0 instead.

It CAN return -1, because of:

#define SAMPLE_unknown 0xffffffff

when it's a signed 32-bit integer, it's -1.

On my end of the fork, I already replaced all the offsets from int into int64_t to match the mainstream Vorbis (otherwise it won't allow play large files, longer than 5 hours, and also depending on the sample rate), and the value of SAMPLE_unknown I simply replaced with -1: simple and easy.: https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/stb_vorbis/stb_vorbis.h

SAMPLE_unknown

Why not to just use the SAMPLE_unknown value directly?

In your code there is a mix of int64_t and uint64_t, shouldn't it be all uint64_t ? I felt that it would be easier to match the eventual 64-bit upgrade by using an existent constant instead of the current manual 0xf...f.

I have a feeling there should be an existing type for this instead like size_t. If one doesn't exist, it should be typedef to ensure it all matches.

btw, I won't change anything right now because GitHub PR is melting: https://www.githubstatus.com/

It appears GitHub is normalizing

Wohlstand · 2024-03-13T01:30:33Z

src/stb_vorbis.h

@@ -5656,6 +5657,11 @@ int stb_vorbis_get_samples_float_interleaved(stb_vorbis *f, int channels, float
         break;
   }
   f->current_playback_loc += n;
+   if(f->current_playback_loc > lgs && lgs > 0 && lgs != UINT_MAX) {


Why not to just use the SAMPLE_unknown directly? It's private code, and you are free to use that here.

if(f->current_playback_loc > lgs && lgs > 0 && lgs != SAMPLE_unknown) {

Adjusted as request : https://github.com/icculus/SDL_sound/compare/fef554e25e79aef3dd24fa964e219f9a5b20a309..945fbbe3a09dd1ae043c2a6b02c9e622cd24f346

I stand-by what I said that stb_vorbis using ints directly is terrible and it should use size_t and SIZE_MAX instead or any other regular c type that is appropriate or typedef things to make sense instead of using naked int/uint mixed around...

I think in the end I would prefer if SDL_sound gave me an options to use external Xiph libraries instead.

sezero · 2025-01-12T20:47:45Z

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

Co-Authored-By: Ozkan Sezer <[email protected]>

ericoporto · 2025-01-12T21:28:26Z

@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 (ericoporto@12fc296, 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 · 2025-01-12T21:32:38Z

@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 · 2025-01-12T21:39:25Z

Sure. Build the rawdecoder from below (or just get this commit ericoporto@12fc296)

#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 · 2025-01-12T22:39:35Z

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

sezero · 2025-01-12T22:44:46Z

I guess we should apply this to SDL_mixer, yes?

sezero · 2025-01-12T22:55:39Z

I guess we should apply this to SDL_mixer, yes?

.. and done.

sezero requested a review from icculus March 5, 2024 00:19

ericoporto mentioned this pull request Mar 5, 2024

Fix ogg with extra buffer at end #95

Closed

Wohlstand reviewed Mar 6, 2024

View reviewed changes

ericoporto force-pushed the fix-stb-vorbis-interleaved branch 2 times, most recently from 91f1206 to fef554e Compare March 7, 2024 12:04

Wohlstand reviewed Mar 13, 2024

View reviewed changes

ericoporto force-pushed the fix-stb-vorbis-interleaved branch from fef554e to 945fbbe Compare March 13, 2024 01:59

sezero mentioned this pull request Apr 2, 2024

2.0.3 release is missing? #96

Open

ericoporto force-pushed the fix-stb-vorbis-interleaved branch from 945fbbe to 833218a Compare January 6, 2025 02:06

ericoporto changed the base branch from main to stable-2.0 January 6, 2025 02:07

ericoporto requested a review from Wohlstand January 12, 2025 13:32

stb_vorbis: fix interleaved appends silence at end

c2ad77b

Co-Authored-By: Ozkan Sezer <[email protected]>

ericoporto force-pushed the fix-stb-vorbis-interleaved branch from 833218a to c2ad77b Compare January 12, 2025 21:23

icculus merged commit 474dbf7 into icculus:stable-2.0 Jan 12, 2025
7 checks passed

sezero mentioned this pull request Jan 12, 2025

Decoding a particular OGG file appends a bit of silence at the end #92

Closed

ericoporto deleted the fix-stb-vorbis-interleaved branch January 12, 2025 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stb_vorbis: fix interleaved appends silence at end #97

stb_vorbis: fix interleaved appends silence at end #97

ericoporto commented Mar 5, 2024

sezero commented Mar 5, 2024

icculus commented Mar 5, 2024

icculus commented Mar 5, 2024 •

edited

Loading

sezero commented Mar 5, 2024

ericoporto commented Mar 5, 2024

sezero commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

ericoporto commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

Wohlstand Mar 6, 2024

ericoporto Mar 7, 2024 •

edited

Loading

ericoporto Mar 7, 2024 •

edited

Loading

ericoporto Mar 7, 2024 •

edited

Loading

Wohlstand Mar 7, 2024

Wohlstand Mar 13, 2024 •

edited

Loading

Wohlstand Mar 13, 2024

ericoporto Mar 13, 2024 •

edited

Loading

ericoporto Mar 13, 2024

ericoporto Mar 13, 2024

Wohlstand Mar 13, 2024 •

edited

Loading

ericoporto Mar 13, 2024 •

edited

Loading

sezero commented Jan 12, 2025

ericoporto commented Jan 12, 2025 •

edited

Loading

sezero commented Jan 12, 2025

ericoporto commented Jan 12, 2025 •

edited

Loading

icculus commented Jan 12, 2025

sezero commented Jan 12, 2025

sezero commented Jan 12, 2025

stb_vorbis: fix interleaved appends silence at end #97

stb_vorbis: fix interleaved appends silence at end #97

Conversation

ericoporto commented Mar 5, 2024

sezero commented Mar 5, 2024

icculus commented Mar 5, 2024

icculus commented Mar 5, 2024 • edited Loading

sezero commented Mar 5, 2024

ericoporto commented Mar 5, 2024

sezero commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

ericoporto commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

Wohlstand commented Mar 6, 2024

Wohlstand Mar 6, 2024

Choose a reason for hiding this comment

ericoporto Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

ericoporto Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

ericoporto Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Wohlstand Mar 7, 2024

Choose a reason for hiding this comment

Wohlstand Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Wohlstand Mar 13, 2024

Choose a reason for hiding this comment

ericoporto Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

ericoporto Mar 13, 2024

Choose a reason for hiding this comment

ericoporto Mar 13, 2024

Choose a reason for hiding this comment

Wohlstand Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

ericoporto Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

sezero commented Jan 12, 2025

ericoporto commented Jan 12, 2025 • edited Loading

sezero commented Jan 12, 2025

ericoporto commented Jan 12, 2025 • edited Loading

icculus commented Jan 12, 2025

sezero commented Jan 12, 2025

sezero commented Jan 12, 2025

icculus commented Mar 5, 2024 •

edited

Loading

ericoporto Mar 7, 2024 •

edited

Loading

ericoporto Mar 7, 2024 •

edited

Loading

ericoporto Mar 7, 2024 •

edited

Loading

Wohlstand Mar 13, 2024 •

edited

Loading

ericoporto Mar 13, 2024 •

edited

Loading

Wohlstand Mar 13, 2024 •

edited

Loading

ericoporto Mar 13, 2024 •

edited

Loading

ericoporto commented Jan 12, 2025 •

edited

Loading

ericoporto commented Jan 12, 2025 •

edited

Loading