[opus] Opusfile seeking bug

[Simon Jackson]

Hi,

I seem to have found a bug with seeking in opusfile in some situations, though this might be better directed to the ogg list. I’ve included a short description below and a test case C program that describes it more detail (comments) and reproduces the issue when used with a file which contains packets that have been split over a page boundary (less common than I thought). The test program is not the cleanest code as I chopped up a much longer C++ test program I wrote to only include the relevant parts to reproduce the bug and converted it to C89 as I thought this would be preferred. I would try and fix it and submit a patch myself, but this is all on the companies time and I’ve got managers pressuring me to get on with fixing other things, sorry.

Short Description:
When a file contains ogg packets which have been split over ogg page boundaries, using op_pcm_seek to seek to a position that falls into the packet that has been split results in an OP_EBADLINK error being returned. This seems to be because the library doesn’t account for the fact that packets that do not finish on the page, do not count towards its granule position, even if they start on it. The seek function notices that the target position is greater than the granule position of the page that the packet starts on and seeks to the next page. It then re-calculates the previous packets GP by subtracting the new page duration from the new page GP which gives it the GP of the packet after the one which has been split. Seeing that it s now past the target position, it returns an OP_EBADLINK error.


To reproduce with the test program, you need a file which definitely has packets split over page boundaries. I can provide an opus file which has split packets if that is easier for you. I wrote this in VS, so sorry in clang or GCC moan about something, I wasn’t sure if it was a good idea to attach a file so I’ve just pasted it in below.


#include <stdio.h>
#include <ogg/ogg.h>
#include <opus.h>
#include <opusfile.h>
#include <stdint.h>

int64_t FindBrokenSeekPoint(char* filename)
{
	// Visual studio doesn't do C99 and I'm converting from C++
	// so dump all the decelerations here to make it build as C89.
	static const long BufferSize = 8192;
	ogg_sync_state m_syncState;
	ogg_stream_state m_streamState;
	int64_t m_lastGranulePos = 0;
	int64_t failingSeekPoint = -1;
	int32_t serialno = -1;

	int fpp, spf;
	int decoding = 1;
	ogg_page page;
	char* buffer = NULL;
	long bytesRead = 0;
	uint8_t headerFLags = 0;
	int pageContinuesPacket = 0;
	int firstPacketInPage = 1;
	int packetsToFetch = 1;
	int packetOutRet;

	FILE* fp = fopen(filename, "rb");
	ogg_sync_init(&m_syncState);

	while(decoding)
	{
		while(ogg_sync_pageout(&m_syncState, &page) != 1)
		{
			buffer = ogg_sync_buffer(&m_syncState, BufferSize);
			bytesRead = fread(buffer, sizeof(char), BufferSize, fp);
			ogg_sync_wrote(&m_syncState, bytesRead);
		}

		// If this is the last page, then don't loop again.
		if(ogg_page_eos(&page) > 0)
			decoding = 0;

		if(serialno < 0)
		{
			serialno = ogg_page_serialno(&page);;
			ogg_stream_init(&m_streamState, serialno);
		}

		headerFLags = page.header[5];
		pageContinuesPacket = headerFLags & 1;
		ogg_stream_pagein(&m_streamState, &page);

		firstPacketInPage = 1;
		packetsToFetch = 1;
		while(packetsToFetch)
		{
			ogg_packet packet;
			packetOutRet = ogg_stream_packetout(&m_streamState, &packet);

			// This is the test case. If you try and seek, using op_pcm_seek,
			// to a position, that when adjusted by 80ms for the discarded data
			// lies within the first half of a packet that has been split over 
			// an ogg page boundary, op_pcm_seek will return OP_EBADLINK. This 
			// seems to be because the library doesn't take into account that
			// split packets are not counted towards the granule position of 
			// the page they begin on. The library finds the page that the 
			// packet beings on and compares the target position to the page 
			// granule position and finds that it is larger. It then seeks to 
			// the beginning of the next page and is now past the point where
			// the audio we want is and doesn't search backwards ever. It then 
			// re-calculates the granule position of the previous packet by 
			// subtracting the new page duration from the new page granule 
			// position and checks to see if the target position is greater
			// than the last packet granule position, which it now is due to
			// the re-calculation and thus throws the OP_EBADLINK error.
			if(packetOutRet == 1 && firstPacketInPage && pageContinuesPacket)
			{
				// Seek to just after the beginning of the split packet, which
				// should begin at m_lastGranulePos.
				decoding = 0;

				// Seeking discards 80ms so the actual returned seek point needs to be 80ms
				// ahead of the failing point to trigger the bug. I'm assuming 20ms frames
				// (960 samples) to make life easy for this test case. So we need to add
				// 960 * 4 to m_lastGranulePos and then a small amount to put it in the
				// the first half of the split packet, 10 should do.
				failingSeekPoint = m_lastGranulePos + (960*4) + 10;
				break;
			}
			else if(packetOutRet == 1)
			{
				if(packet.packetno < 2)
					continue;

				// Calculate packet granule positions
				fpp = opus_packet_get_nb_frames(packet.packet, packet.bytes);
				spf = opus_packet_get_samples_per_frame(packet.packet, 48000);
				spf *= fpp;
				m_lastGranulePos += spf;
			}
			else
			{
				// Need more data or unrecoverable error.
				packetsToFetch = 0;
			}

			firstPacketInPage = 0;
		}
	}

	fclose(fp);
	ogg_stream_clear(&m_streamState);
	ogg_sync_clear(&m_syncState);
	return failingSeekPoint;
}

int main(int argc, char* argv[])
{
	char* filename = "TestCase.opus";

	// This is the test case. If you try and seek, using op_pcm_seek,
	// to a position, that when adjusted by 80ms for the discarded data
	// lies within the first half of a packet that has been split over 
	// an ogg page boundary, op_pcm_seek will return OP_EBADLINK. This 
	// seems to be because the library doesn't take into account that
	// split packets are not counted towards the granule position of 
	// the page they begin on. The library finds the page that the 
	// packet beings on and compares the target position to the page 
	// granule position and finds that it is larger. It then seeks to 
	// the beginning of the next page and is now past the point where
	// the audio we want is and doesn't search backwards ever. It then 
	// re-calculates the granule position of the previous packet by 
	// subtracting the new page duration from the new page granule 
	// position and checks to see if the target position is greater
	// than the last packet granule position, which it now is due to
	// the re-calculation and thus throws the OP_EBADLINK error.
	int64_t failingSeekPoint = FindBrokenSeekPoint(filename);

	if(failingSeekPoint >= 0)
	{
		OggOpusFile* opfile = op_open_file(filename, NULL);

		// Seeking to somewhere in the first packet should return
		// OP_EBADLINK, however seeking discards 80ms so the actual
		// requested point needs to be 80ms ahead of the failing point.
		// This is handled by FindBrokenSeekPoint which has already
		// adjusted the seek point to account for the 80ms.

		// Adjust the raw GP to pcm pos by subtracting pre-skip amount
		// of 48K.
		int retcode = op_pcm_seek(opfile, failingSeekPoint-4800);

		if(retcode < 0)
			printf("Seek failed with OP_EBADLINK!\n");

		op_free(opfile);
	}
	else
	{
		printf("No packets split over page boundaries in file.");
	}

	return 0;
}


Simon Jackson
Software Developer
Sonocent Ltd.
Tel: 0113 815 0223 
www.sonocent.com <http://www.sonocent.com/>
@AudioNotetaker <https://twitter.com/AudioNotetaker>



-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.xiph.org/pipermail/opus/attachments/20151105/6828266d/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PastedGraphic-1.tiff
Type: image/tiff
Size: 25894 bytes
Desc: not available
Url : http://lists.xiph.org/pipermail/opus/attachments/20151105/6828266d/attachment-0001.tiff