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

Fixed bug when seeking to pos < dataoffset #71

Closed
wants to merge 1 commit into from
Closed

Conversation

ed95
Copy link

@ed95 ed95 commented Aug 13, 2020

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 #60.

Copy link
Contributor

@sezero sezero left a comment

Choose a reason for hiding this comment

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

I suggest removing the C99'ism for initialBegin

@ed95
Copy link
Author

ed95 commented Aug 24, 2020

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

@sezero
Copy link
Contributor

sezero commented Aug 24, 2020

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
Copy link
Author

ed95 commented Aug 24, 2020

I see - I've made that change now.

@sezero
Copy link
Contributor

sezero commented Jun 23, 2021

What is the status of this patch?

@ed95
Copy link
Author

ed95 commented Jun 23, 2021

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

This pull request was closed.
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.

None yet

2 participants