revamp the Ogg decoding logic; much more stable now

src/libOggFLAC/include/private/ogg_decoder_aspect.h

@@ -46,6 +46,11 @@ typedef struct OggFLAC__OggDecoderAspect {
 	ogg_stream_state stream_state;
 	ogg_sync_state sync_state;
 	FLAC__bool need_serial_number;
+	FLAC__bool end_of_stream;
+	FLAC__bool have_working_page; /* only if true will the following vars be valid */
+	ogg_page working_page;
+	FLAC__bool have_working_packet; /* only if true will the following vars be valid */
+	ogg_packet working_packet; /* as we work through the packet we will move working_packet.packet forward and working_packet.bytes down */
 } OggFLAC__OggDecoderAspect;
 
 void OggFLAC__ogg_decoder_aspect_set_serial_number(OggFLAC__OggDecoderAspect *aspect, long value);
@@ -55,16 +60,17 @@ void OggFLAC__ogg_decoder_aspect_finish(OggFLAC__OggDecoderAspect *aspect);
 void OggFLAC__ogg_decoder_aspect_flush(OggFLAC__OggDecoderAspect *aspect);
 void OggFLAC__ogg_decoder_aspect_reset(OggFLAC__OggDecoderAspect *aspect);
 
-typedef FLAC__StreamDecoderReadStatus (*OggFLAC__OggDecoderAspectReadCallbackProxy)(const void *decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data);
-
 typedef enum {
 	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK = 0,
 	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM,
+	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_LOST_SYNC,
 	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT,
 	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ERROR,
 	OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_MEMORY_ALLOCATION_ERROR
 } OggFLAC__OggDecoderAspectReadStatus;
 
+typedef OggFLAC__OggDecoderAspectReadStatus (*OggFLAC__OggDecoderAspectReadCallbackProxy)(const void *decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data);
+
 OggFLAC__OggDecoderAspectReadStatus OggFLAC__ogg_decoder_aspect_read_callback_wrapper(OggFLAC__OggDecoderAspect *aspect, FLAC__byte buffer[], unsigned *bytes, OggFLAC__OggDecoderAspectReadCallbackProxy read_callback, void *decoder, void *client_data);
 
 #endif

src/libOggFLAC/ogg_decoder_aspect.c

@@ -33,10 +33,10 @@
 #include "FLAC/assert.h"
 #include "private/ogg_decoder_aspect.h"
 
-#ifdef min
-#undef min
+#ifdef max
+#undef max
 #endif
-#define min(x,y) ((x)<(y)?(x):(y))
+#define max(x,y) ((x)>(y)?(x):(y))
 
 /***********************************************************************
  *
@@ -46,8 +46,6 @@
 
 FLAC__bool OggFLAC__ogg_decoder_aspect_init(OggFLAC__OggDecoderAspect *aspect)
 {
-	aspect->need_serial_number = aspect->use_first_serial_number;
-
 	/* we will determine the serial number later if necessary */
 	if(ogg_stream_init(&aspect->stream_state, aspect->serial_number) != 0)
 		return false;
@@ -55,6 +53,11 @@ FLAC__bool OggFLAC__ogg_decoder_aspect_init(OggFLAC__OggDecoderAspect *aspect)
 	if(ogg_sync_init(&aspect->sync_state) != 0)
 		return false;
 
+	aspect->need_serial_number = aspect->use_first_serial_number;
+
+	aspect->end_of_stream = false;
+	aspect->have_working_page = false;
+
 	return true;
 }
 
@@ -84,62 +87,133 @@ void OggFLAC__ogg_decoder_aspect_reset(OggFLAC__OggDecoderAspect *aspect)
 {
 	(void)ogg_stream_reset(&aspect->stream_state);
 	(void)ogg_sync_reset(&aspect->sync_state);
+	aspect->end_of_stream = false;
+	aspect->have_working_page = false;
 }
 
 OggFLAC__OggDecoderAspectReadStatus OggFLAC__ogg_decoder_aspect_read_callback_wrapper(OggFLAC__OggDecoderAspect *aspect, FLAC__byte buffer[], unsigned *bytes, OggFLAC__OggDecoderAspectReadCallbackProxy read_callback, void *decoder, void *client_data)
 {
 	static const unsigned OGG_BYTES_CHUNK = 8192;
-	unsigned ogg_bytes_to_read, ogg_bytes_read;
-	ogg_page page;
-	char *oggbuf;
+	const unsigned bytes_requested = *bytes;
 
 	/*
-	 * We have to be careful not to read in more than the
-	 * FLAC__StreamDecoder says it has room for.  We know
-	 * that the size of the decoded data must be no more
-	 * than the encoded data we will read.
+	 * The FLAC decoding API uses pull-based reads, whereas Ogg decoding
+	 * is push-based.  In libFLAC, when you ask to decode a frame, the
+	 * decoder will eventually call the read callback to supply some data,
+	 * but how much it asks for depends on how much free space it has in
+	 * its internal buffer.  It does not try to grow its internal buffer
+	 * to accomodate a whole frame because then the internal buffer size
+	 * could not be limited, which is necessary in embedded applications.
+	 *
+	 * Ogg however grows its internal buffer until a whole page is present;
+	 * only then can you get decoded data out.  So we can't just ask for
+	 * the same number of bytes from Ogg, then pass what's decoded down to
+	 * libFLAC.  If what libFLAC is asking for will not contain a whole
+	 * page, then we will get no data from ogg_sync_pageout(), and at the
+	 * same time cannot just read more data from the client for the purpose
+	 * of getting a whole decoded page because the decoded size might be
+	 * larger than libFLAC's internal buffer.
+	 *
+	 * Instead, whenever this read callback wrapper is called, we will
+	 * continually request data from the client until we have at least one
+	 * page, and manage pages internally so that we can send pieces of
+	 * pages down to libFLAC in such a way that we obey its size
+	 * requirement.  To limit the amount of callbacks, we will always try
+	 * to read in enough pages to return the full number of bytes
+	 * requested.
 	 */
-	ogg_bytes_to_read = min(*bytes, OGG_BYTES_CHUNK);
-	oggbuf = ogg_sync_buffer(&aspect->sync_state, ogg_bytes_to_read);
-
-	if(0 == oggbuf)
-		return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_MEMORY_ALLOCATION_ERROR;
-
-	ogg_bytes_read = ogg_bytes_to_read;
-
-	switch(read_callback(decoder, (FLAC__byte*)oggbuf, &ogg_bytes_read, client_data)) {
-		case FLAC__STREAM_DECODER_READ_STATUS_CONTINUE:
-			break;
-		case FLAC__STREAM_DECODER_READ_STATUS_END_OF_STREAM:
-			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM;
-		case FLAC__STREAM_DECODER_READ_STATUS_ABORT:
-			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
-		default:
-			FLAC__ASSERT(0);
-	}
-
-	if(ogg_sync_wrote(&aspect->sync_state, ogg_bytes_read) < 0)
-		return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ERROR;
-
 	*bytes = 0;
-	while(ogg_sync_pageout(&aspect->sync_state, &page) == 1) {
-		/* grab the serial number if necessary */
-		if(aspect->need_serial_number) {
-			aspect->stream_state.serialno = aspect->serial_number = ogg_page_serialno(&page);
-			aspect->need_serial_number = false;
+	while (*bytes < bytes_requested && !aspect->end_of_stream) {
+		if (aspect->have_working_page) {
+			if (aspect->have_working_packet) {
+				unsigned n = bytes_requested - *bytes;
+				if ((unsigned)aspect->working_packet.bytes <= n) {
+					/* the rest of the packet will fit in the buffer */
+					n = aspect->working_packet.bytes;
+					memcpy(buffer, aspect->working_packet.packet, n);
+					*bytes += n;
+					buffer += n;
+					aspect->have_working_packet = false;
+				}
+				else {
+					/* only n bytes of the packet will fit in the buffer */
+					memcpy(buffer, aspect->working_packet.packet, n);
+					*bytes += n;
+					buffer += n;
+					aspect->working_packet.packet += n;
+					aspect->working_packet.bytes -= n;
+				}
+			}
+			else {
+				/* try and get another packet */
+				const int ret = ogg_stream_packetout(&aspect->stream_state, &aspect->working_packet);
+				if (ret > 0) {
+					aspect->have_working_packet = true;
+				}
+				else if (ret == 0) {
+					aspect->have_working_page = false;
+				}
+				else { /* ret < 0 */
+					/* lost sync, we'll leave the working page for the next call */
+					return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_LOST_SYNC;
+				}
+			}
 		}
-		if(ogg_stream_pagein(&aspect->stream_state, &page) == 0) {
-			ogg_packet packet;
-
-			while(ogg_stream_packetout(&aspect->stream_state, &packet) == 1) {
-				memcpy(buffer, packet.packet, packet.bytes);
-				*bytes += packet.bytes;
-				buffer += packet.bytes;
+		else {
+			/* try and get another page */
+			const int ret = ogg_sync_pageout(&aspect->sync_state, &aspect->working_page);
+			if (ret > 0) {
+				/* got a page, grab the serial number if necessary */
+				if(aspect->need_serial_number) {
+					aspect->stream_state.serialno = aspect->serial_number = ogg_page_serialno(&aspect->working_page);
+					aspect->need_serial_number = false;
+				}
+				if(ogg_stream_pagein(&aspect->stream_state, &aspect->working_page) == 0) {
+					aspect->have_working_page = true;
+					aspect->have_working_packet = false;
+				}
+				/* else do nothing, could be a page from another stream */
+			}
+			else if (ret == 0) {
+				/* need more data */
+				const unsigned ogg_bytes_to_read = max(bytes_requested - *bytes, OGG_BYTES_CHUNK);
+				char *oggbuf = ogg_sync_buffer(&aspect->sync_state, ogg_bytes_to_read);
+
+				if(0 == oggbuf) {
+					return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_MEMORY_ALLOCATION_ERROR;
+				}
+				else {
+					unsigned ogg_bytes_read = ogg_bytes_to_read;
+
+					switch(read_callback(decoder, (FLAC__byte*)oggbuf, &ogg_bytes_read, client_data)) {
+						case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK:
+							break;
+						case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM:
+							aspect->end_of_stream = true;
+							break;
+						case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT:
+							return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
+						default:
+							FLAC__ASSERT(0);
+					}
+
+					if(ogg_sync_wrote(&aspect->sync_state, ogg_bytes_read) < 0) {
+						/* double protection; this will happen if the read callback returns more bytes than the max requested, which would overflow Ogg's internal buffer */
+						FLAC__ASSERT(0);
+						return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ERROR;
+					}
+				}
+			}
+			else { /* ret < 0 */
+				/* lost sync, bail out */
+				return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_LOST_SYNC;
 			}
-		} else {
-			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ERROR;
 		}
 	}
 
+	if (aspect->end_of_stream && *bytes == 0) {
+		return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM;
+	}
+
 	return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK;
 }

src/libOggFLAC/seekable_stream_decoder.c

@@ -48,6 +48,7 @@ static FLAC__bool eof_callback_(const FLAC__SeekableStreamDecoder *decoder, void
 static FLAC__StreamDecoderWriteStatus write_callback_(const FLAC__SeekableStreamDecoder *decoder, const FLAC__Frame *frame, const FLAC__int32 * const buffer[], void *client_data);
 static void metadata_callback_(const FLAC__SeekableStreamDecoder *decoder, const FLAC__StreamMetadata *metadata, void *client_data);
 static void error_callback_(const FLAC__SeekableStreamDecoder *decoder, FLAC__StreamDecoderErrorStatus status, void *client_data);
+static OggFLAC__OggDecoderAspectReadStatus read_callback_proxy_(const void *void_decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data);
 
 
 /***********************************************************************
@@ -604,9 +605,15 @@ FLAC__SeekableStreamDecoderReadStatus read_callback_(const FLAC__SeekableStreamD
 
 	(void)unused;
 
-	switch(OggFLAC__ogg_decoder_aspect_read_callback_wrapper(&decoder->protected_->ogg_decoder_aspect, buffer, bytes, (OggFLAC__OggDecoderAspectReadCallbackProxy)decoder->private_->read_callback, decoder, decoder->private_->client_data)) {
+	switch(OggFLAC__ogg_decoder_aspect_read_callback_wrapper(&decoder->protected_->ogg_decoder_aspect, buffer, bytes, read_callback_proxy_, decoder, decoder->private_->client_data)) {
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK:
 			return FLAC__SEEKABLE_STREAM_DECODER_READ_STATUS_OK;
+		/* we don't really have a way to handle lost sync via read
+		 * callback so we'll let it pass and let the underlying
+		 * FLAC decoder catch the error
+		 */
+		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_LOST_SYNC:
+			return FLAC__SEEKABLE_STREAM_DECODER_READ_STATUS_OK;
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM:
 			return FLAC__SEEKABLE_STREAM_DECODER_READ_STATUS_OK;
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT:
@@ -675,3 +682,22 @@ void error_callback_(const FLAC__SeekableStreamDecoder *unused, FLAC__StreamDeco
 	(void)unused;
 	decoder->private_->error_callback(decoder, status, decoder->private_->client_data);
 }
+
+OggFLAC__OggDecoderAspectReadStatus read_callback_proxy_(const void *void_decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data)
+{
+	OggFLAC__SeekableStreamDecoder *decoder = (OggFLAC__SeekableStreamDecoder*)void_decoder;
+
+	switch(decoder->private_->read_callback(decoder, buffer, bytes, client_data)) {
+		case FLAC__SEEKABLE_STREAM_DECODER_READ_STATUS_OK:
+			if (*bytes == 0)
+				return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM;
+			else
+				return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK;
+		case FLAC__SEEKABLE_STREAM_DECODER_READ_STATUS_ERROR:
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
+		default:
+			/* double protection: */
+			FLAC__ASSERT(0);
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
+	}
+}

src/libOggFLAC/stream_decoder.c

@@ -44,6 +44,7 @@ static FLAC__StreamDecoderReadStatus read_callback_(const FLAC__StreamDecoder *d
 static FLAC__StreamDecoderWriteStatus write_callback_(const FLAC__StreamDecoder *decoder, const FLAC__Frame *frame, const FLAC__int32 * const buffer[], void *client_data);
 static void metadata_callback_(const FLAC__StreamDecoder *decoder, const FLAC__StreamMetadata *metadata, void *client_data);
 static void error_callback_(const FLAC__StreamDecoder *decoder, FLAC__StreamDecoderErrorStatus status, void *client_data);
+static OggFLAC__OggDecoderAspectReadStatus read_callback_proxy_(const void *void_decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data);
 
 
 /***********************************************************************
@@ -494,9 +495,15 @@ FLAC__StreamDecoderReadStatus read_callback_(const FLAC__StreamDecoder *unused,
 
 	(void)unused;
 
-	switch(OggFLAC__ogg_decoder_aspect_read_callback_wrapper(&decoder->protected_->ogg_decoder_aspect, buffer, bytes, (OggFLAC__OggDecoderAspectReadCallbackProxy)decoder->private_->read_callback, decoder, decoder->private_->client_data)) {
+	switch(OggFLAC__ogg_decoder_aspect_read_callback_wrapper(&decoder->protected_->ogg_decoder_aspect, buffer, bytes, read_callback_proxy_, decoder, decoder->private_->client_data)) {
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK:
 			return FLAC__STREAM_DECODER_READ_STATUS_CONTINUE;
+		/* we don't really have a way to handle lost sync via read
+		 * callback so we'll let it pass and let the underlying
+		 * FLAC decoder catch the error
+		 */
+		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_LOST_SYNC:
+			return FLAC__STREAM_DECODER_READ_STATUS_CONTINUE;
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM:
 			return FLAC__STREAM_DECODER_READ_STATUS_END_OF_STREAM;
 		case OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT:
@@ -533,3 +540,21 @@ void error_callback_(const FLAC__StreamDecoder *unused, FLAC__StreamDecoderError
 	(void)unused;
 	decoder->private_->error_callback(decoder, status, decoder->private_->client_data);
 }
+
+OggFLAC__OggDecoderAspectReadStatus read_callback_proxy_(const void *void_decoder, FLAC__byte buffer[], unsigned *bytes, void *client_data)
+{
+	OggFLAC__StreamDecoder *decoder = (OggFLAC__StreamDecoder*)void_decoder;
+
+	switch(decoder->private_->read_callback(decoder, buffer, bytes, client_data)) {
+		case FLAC__STREAM_DECODER_READ_STATUS_CONTINUE:
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_OK;
+		case FLAC__STREAM_DECODER_READ_STATUS_END_OF_STREAM:
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_END_OF_STREAM;
+		case FLAC__STREAM_DECODER_READ_STATUS_ABORT:
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
+		default:
+			/* double protection: */
+			FLAC__ASSERT(0);
+			return OggFLAC__OGG_DECODER_ASPECT_READ_STATUS_ABORT;
+	}
+}