Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ogg with extra buffer at end #95

Closed

Conversation

ericoporto
Copy link
Contributor

There is a bug in stb_vorbis that I am not sure how to fix

SDL_sound/src/stb_vorbis.h

Lines 5651 to 5658 in fdcecaf

n += k;
f->channel_buffer_start += k;
if (n == len)
break;
if (!stb_vorbis_get_frame_float(f, NULL, &outputs))
break;
}
f->current_playback_loc += n;

The n that is added here at the end can be bigger than the size of the buffer. The current_playback_loc was added in nothings/stb#1295, it will be wrong at the end, but it's correct before the last call where there are samples, so we can use that to workaround and get the true final length.

The other approach would be to push the truncation in stb_vorbis, inside stb_vorbis_get_samples_float_interleaved, the issue there is it would get stb_vorbis more different from it's upstream. I think the actual issue is stb_vorbis_get_frame_float is called and the result is dismissed, but that result would be the only other way to know the actual remaining length.

fix #92

@sezero
Copy link
Collaborator

sezero commented Jan 20, 2024

CC: @Wohlstand for the sample-accurate seek references.

@sezero sezero requested a review from icculus January 20, 2024 00:19
@Wohlstand
Copy link

Wohlstand commented Jan 20, 2024

The function that I added is stb_vorbis_get_playback_sample_offset(), I didn't changed existing call because of some worries given by some people who reviewed my stuff. So, adding this doesn't changed anything also, except of adding of new variable that holds the sample-accurate position.

@@ -260,6 +260,7 @@ typedef struct __SOUND_SAMPLEINTERNAL__
Uint32 buffer_size;
void *decoder_private;
Sint32 total_time;
Uint32 total_length;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add a new member to the struct here, but only init it in VORBIS_open. I think you're supposed to init it at least to some default value in each function that initializes this struct.

@ericoporto
Copy link
Contributor Author

If anyone wants to compare, here is the same fix in stb_vorbis side.

main...ericoporto:SDL_sound-1:fix-stb-vorbis-interleaved

I think the actual issue is stb_vorbis_get_frame_float is called and the result is dismissed

Ok, I tried this and it gives a different third value and will still be wrong, just slightly less wrong (it will append 2 ms instead of 5 ms)

@sezero
Copy link
Collaborator

sezero commented Jan 21, 2024

If anyone wants to compare, here is the same fix in stb_vorbis side.

main...ericoporto:SDL_sound-1:fix-stb-vorbis-interleaved

If this fixes the issue, stb_vorbis patching should be preferred, IMO.
@Wohlstand, @icculus: Does the above-referenced change look OK?

@Wohlstand
Copy link

On first look seems ok. I'll try to check some after sleep.

@ericoporto
Copy link
Contributor Author

Closing this in favor of #97

@ericoporto ericoporto closed this Mar 5, 2024
@ericoporto ericoporto deleted the fix-ogg-with-extra-buffer-at-end branch March 5, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decoding a particular OGG file appends a bit of silence at the end
4 participants