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

missing samples when doing a partial read of ogg file from index till the end of file #643

Closed
krisfed opened this issue Oct 22, 2020 · 11 comments · Fixed by #709
Closed
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@krisfed
Copy link

krisfed commented Oct 22, 2020

Hi all,

We are in the process of upgrading from 1.0.28 to 1.0.30, and we noticed this issue in the new release.

Looks like it happens when you seek to a start index that is >=88201 before reading. The read data seems to match the data at end of the file, but some samples right after the start index are missing. The number of missing samples seems to always be a power of two.

Here is simple reproduction code with an example:

#include <iostream>
#include "sndfile.h"

int main()
{
	// open file
       SF_INFO fileInfo;

       SNDFILE* file_sf = sf_open("mytest.ogg", SFM_READ, &fileInfo);
	
	// seek
	sf_count_t startSample = 389999;
	sf_count_t framesSeeked = sf_seek(file_sf, startSample, SEEK_SET);

        std::cout << "startSample: " << startSample << std::endl;
	std::cout << "framesSeeked: " << framesSeeked << std::endl;
	
	// read
	sf_count_t framesRead = 0;
	
	size_t frameSize = 65536;
	size_t sizeInBytes = 4 * frameSize * 2;// type size * num samples * num channels
	uint8_t* buffer = new uint8_t[sizeInBytes];
	
	framesRead = sf_readf_float(file_sf, reinterpret_cast<float*>(buffer), frameSize);
	
        std::cout << "frameSize: " << frameSize << std::endl;
	std::cout << "sizeInBytes: " << sizeInBytes << std::endl;
	std::cout << "framesRead: " << framesRead << std::endl;
	
	sf_close(file_sf);

    return 0;

}

For this example, the total number of samples is 396519. If we start at 389999 and try to read a framesize (65536) we should get all the remaining samples in the file - 6520. But with 1.0.30 release I only get 5496 (missing 1024 samples).

Here is the output of the above repro code for 1.0.28:

startSample: 389999
framesSeeked: 389999
frameSize: 65536
sizeInBytes: 524288
framesRead: 6520

And for 1.0.30:

startSample: 389999
framesSeeked: 389999
frameSize: 65536
sizeInBytes: 524288
framesRead: 5496

(both ran on macOS 10.15.3, but we see the issue on Windows too)

The file used in the example is attached - mytest.zip. It was randomly generated, but I see this issue on other .ogg files that are long enough. Here is the snd-info output:

$ sndfile-info mytest.ogg 
========================================
File : mytest.ogg
Length : 504453
Ogg stream data : Vorbis
Stream serialno : 536000084
Vorbis library version : Xiph.Org libVorbis 1.3.6
Bitstream is 2 channel, 44100 Hz
Encoded by : Xiph.Org libVorbis I 20200704 (Reducing Environment)
End

----------------------------------------
Sample Rate : 44100
Frames      : 396519
Channels    : 2
Format      : 0x00200060
Sections    : 1
Seekable    : TRUE
Duration    : 00:00:08.991
Signal Max  : 1.21788 (-88.60 dB)

Does this look familiar to anyone? Is this a bug introduced in 1.0.29 or 1.0.30, or is there something else going on here?

Thanks!

@evpobr
Copy link
Member

evpobr commented Oct 23, 2020

Hi @krisfed.

Can you help us to find buggy commit with git bisect?

@krisfed
Copy link
Author

krisfed commented Oct 23, 2020

Sure thing! Seems like this is the culprit:

ea87e01ca25b41012db18ef3d1acb8e14f2aba1f is the first bad commit
commit ea87e01ca25b41012db18ef3d1acb8e14f2aba1f
Author: Arthur Taylor <art@ified.ca>
Date:   Wed Feb 20 23:03:41 2019 -0800

    Vorbis: Use new ogg functions, add bisection seek support.

 src/ogg_vorbis.c | 649 +++++++++++++++++++------------------------------------
 1 file changed, 217 insertions(+), 432 deletions(-)

Makes sense if something is going awry with seeking..

@evpobr
Copy link
Member

evpobr commented Oct 23, 2020

@arthurt, can you look at this?

@arthurt
Copy link
Member

arthurt commented Oct 23, 2020

Looking

@krisfed
Copy link
Author

krisfed commented Nov 5, 2020

If this helps, one thing I noticed is that the point when the start index becomes "bad" seems to depend on the sample rate of the file. For the above example, for the sample rate of 44100, the starting indices that cause problems are >= 88201. For sample rate of 22050, "bad" starting indices are >= 44101. So it would seem that problematic indices begin from 2*(sample rate) + 1 . I did not look at the source code to see why it might be, but just in case this is useful.

@arthurt
Copy link
Member

arthurt commented Nov 9, 2020

This would suggest that it is the bisection seek search code which is causing an issue. For a seek that is less than 2 seconds from the last read position, the seek is performed by reading and throwing away decoded samples instead of searching. (The negative condition in the following code snippit.)

src/ogg_vorbis.c:812
	        /*
		** If the end of the file is know, and the seek isn't for the near
		** future, do a search of the file for a good place to start.
		*/
		ret = 0 ;
		if ((vdata->pcm_end != (uint64_t) -1) &&
			(target < vdata->loc || target - vdata->loc > (2 * psf->sf.samplerate)))
		{	
                    /* Bisection search code here */

I guess this wasn't picked up by the test suite because it must not test seeks of more than 2 seconds.

@arthurt
Copy link
Member

arthurt commented Nov 10, 2020

I don't think there is a fault with the bisection search code in ogg_stream_seek_page_search(). The returned Ogg page is just before the granule position target.

It all acts as if there is an unaccounted for decoder delay. The first 20ms or thereabouts of samples after vorbis_synthesis_restart() go missing. libvorbis documentation says nothing on the subject, and libvorbisfile seeking code doesn't seem to have provisions for it either.

Still investigating.

@evpobr
Copy link
Member

evpobr commented Nov 10, 2020

Thanks for explaination.

@evpobr evpobr added the Bug Something isn't working label Nov 21, 2020
@arthurt
Copy link
Member

arthurt commented Nov 30, 2020

As an update, the issue is caused by not taking into account the full-lapped nature of vorbis blocks. When seeking to a page the sample offset in the file needs to be calculated from the page's granule position. The code in master currently over-counts how many samples are in a page after a seek.

See https://xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-230001.3.2 about window shapes.

I'll have a PR up soon.

@Be-ing
Copy link

Be-ing commented Feb 9, 2021

ping @arthurt

Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 9, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 10, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 14, 2021
@evpobr evpobr added this to the v1.1.0 milestone Feb 16, 2021
@arthurt
Copy link
Member

arthurt commented Feb 16, 2021

The fix merged in #709 should have addressed this issue. The provided test-case works correctly now.

GitHub auto-closes the ticket on merge. If you are still experiencing issues with seeking in Ogg/Vorbis, please let us know.

Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 16, 2021
Be-ing added a commit to Be-ing/mixxx that referenced this issue Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants