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.

stb_vorbis: fix interleaved appends silence at end

0187e9a

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

stb_vorbis: ensure stream length in samples is valid

945fbbe

sezero mentioned this pull request Apr 2, 2024

2.0.3 release is missing? #96

Open

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

stb_vorbis: fix interleaved appends silence at end #97

Are you sure you want to change the base?

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

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