xiph / vorbis

Reference implementation of the Ogg Vorbis audio format.
BSD 3-Clause "New" or "Revised" License
450 stars 183 forks source link

Fixed bug when seeking to pos < dataoffset #71

Closed ed95 closed 2 years ago

ed95 commented 3 years ago

When calling ov_pcm_seek() to a sample offset smaller than the calculated data offset for the file's stream, ov_pcm_seek_page() can fail to find the correct page due to the begin pos for the page search being set to the data offset. This causes audible glitches in some .ogg files, for example the example file from the Ogg Vorbis Wikipedia page.

This PR adds a special case for when pos < vf->dataoffsets[link] and adds a check for 'looking for data in first page' special case. I'm not sure if it's entirely robust, but it fixes the issue in the above file and some others. This may also be related to https://github.com/xiph/vorbis/issues/60.

ed95 commented 3 years ago

What would you suggest? We need to store it before begin is modified below in order for the check later to work.

sezero commented 3 years ago

What would you suggest?

E.g., the first hunk can be like:

diff --git a/lib/vorbisfile.c b/lib/vorbisfile.c
index 9219c2f..1429bdf 100644
--- a/lib/vorbisfile.c
+++ b/lib/vorbisfile.c
@@ -1439,9 +1439,12 @@ int ov_pcm_seek_page(OggVorbis_File *vf,ogg_int64_t pos){
     ogg_int64_t target=pos-total+begintime;
     ogg_int64_t best=-1;
     int         got_page=0;
-
+    ogg_int64_t initialBegin;
     ogg_page og;

+    if (pos < begin) begin = pos;
+    initialBegin = begin;
+
     /* if we have only one page, there will be no bisection.  Grab the page here */
     if(begin==end){
       result=_seek_helper(vf,begin);
ed95 commented 3 years ago

I see - I've made that change now.

sezero commented 3 years ago

What is the status of this patch?

ed95 commented 3 years ago

We made this change in our local copy of vorbis in JUCE in commit https://github.com/juce-framework/JUCE/commit/35d0a8c8. We've had no complaints from users so far so it'd be great if it could get merged into main.