process the streams by page instead of packets

src/cli.cc

@@ -184,8 +184,8 @@ static ot::status edit_tags(ot::opus_tags& tags, const ot::options& opt)
  */
 static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::options &opt)
 {
-	int packet_count = 0;
-	for (;;) {
+	int page_no;
+	for (page_no = 0;; ++page_no) {
 		// Read the next page.
 		ot::status rc = reader.read_page();
 		if (rc == ot::st::end_of_stream)
@@ -193,7 +193,7 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 		else if (rc != ot::st::ok)
 			return rc;
 		// Short-circuit when the relevant packets have been read.
-		if (packet_count >= 2 && writer) {
+		if (page_no >= 2 && writer) {
 			if ((rc = writer->write_page(reader.page)) != ot::st::ok)
 				return rc;
 			continue;
@@ -201,42 +201,32 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 		auto serialno = ogg_page_serialno(&reader.page);
 		if (writer && (rc = writer->prepare_stream(serialno)) != ot::st::ok)
 			return rc;
-		// Read all the packets.
-		for (;;) {
-			rc = reader.read_packet();
-			if (rc == ot::st::end_of_page)
+		if (page_no == 0) { // Identification header
+			rc = reader.read_header_packet(ot::validate_identification_header);
+			if (rc != ot::st::ok)
+				return rc;
+			if (writer && (rc = writer->write_header_packet(reader.packet)) != ot::st::ok)
+				return rc;
+		} else if (page_no == 1) { // Comment header
+			ot::opus_tags tags;
+			rc = reader.read_header_packet(
+				[&tags](ogg_packet& p) { return ot::parse_tags(p, tags); });
+			if (rc != ot::st::ok)
+				return rc;
+			if ((rc = edit_tags(tags, opt)) != ot::st::ok)
+				return rc;
+			if (writer) {
+				auto packet = ot::render_tags(tags);
+				if ((rc = writer->write_header_packet(packet)) != ot::st::ok)
+					return rc;
+			} else {
+				ot::print_comments(tags.comments, stdout);
 				break;
-			else if (rc != ot::st::ok)
-				return rc;
-			packet_count++;
-			if (packet_count == 1) { // Identification header
-				rc = ot::validate_identification_header(reader.packet);
-				if (rc != ot::st::ok)
-					return rc;
-			} else if (packet_count == 2) { // Comment header
-				ot::opus_tags tags;
-				if ((rc = ot::parse_tags(reader.packet, tags)) != ot::st::ok)
-					return rc;
-				if ((rc = edit_tags(tags, opt)) != ot::st::ok)
-					return rc;
-				if (writer) {
-					auto packet = ot::render_tags(tags);
-					if ((rc = writer->write_packet(packet)) != ot::st::ok)
-						return rc;
-					continue;
-				} else {
-					ot::print_comments(tags.comments, stdout);
-				}
 			}
-			if (writer && (rc = writer->write_packet(reader.packet)) != ot::st::ok)
-				return rc;
-		}
-		// Write the assembled page.
-		if (writer && (rc = writer->flush_page()) != ot::st::ok)
-			return rc;
+		} /** \todo Move the generic case here, after we've gotten rid of prepare_stream. */
 	}
-	if (packet_count < 2)
-		return {ot::st::error, "Expected at least 2 Ogg packets"};
+	if (page_no < 1)
+		return {ot::st::error, "Expected at least 2 Ogg pages."};
 	return ot::st::ok;
 }
 

src/ogg.cc

@@ -71,6 +71,23 @@ ot::status ot::ogg_reader::read_packet()
 		return {st::libogg_error, "ogg_stream_packetout failed"};
 }
 
+ot::status ot::ogg_reader::read_header_packet(const std::function<status(ogg_packet&)>& f)
+{
+	ot::status rc = read_packet();
+	if (rc == ot::st::end_of_page)
+		return {ot::st::error, "Header pages must not be empty."};
+	else if (rc != ot::st::ok)
+		return rc;
+	if ((rc = f(packet)) != ot::st::ok)
+		return rc;
+	rc = read_packet();
+	if (rc == ot::st::ok)
+		return {ot::st::error, "Unexpected second packet in header page."};
+	else if (rc != ot::st::end_of_page)
+		return rc;
+	return ot::st::ok;
+}
+
 ot::ogg_writer::ogg_writer(FILE* output)
 	: file(output)
 {
@@ -113,6 +130,17 @@ ot::status ot::ogg_writer::write_packet(const ogg_packet& packet)
 		return st::ok;
 }
 
+ot::status ot::ogg_writer::write_header_packet(const ogg_packet& packet)
+{
+	ot::status rc;
+	if ((rc = write_packet(packet)) != ot::st::ok)
+		return rc;
+	return flush_page();
+}
+
+/**
+ * \todo What if the packet is too big to fit a single page?
+ */
 ot::status ot::ogg_writer::flush_page()
 {
 	ogg_page page;

src/opustags.h

@@ -26,6 +26,7 @@
 #include <ogg/ogg.h>
 #include <stdio.h>
 
+#include <functional>
 #include <list>
 #include <memory>
 #include <string>
@@ -173,8 +174,14 @@ public:
 	 * return #status::stream_not_ready.
 	 *
 	 * After the last packet was read, return #status::end_of_page.
+	 *
+	 * \todo Merge into #read_header_packet.
 	 */
 	status read_packet();
+	/**
+	 * Read the single packet contained in a header page, and call the function f on it.
+	 */
+	status read_header_packet(const std::function<status(ogg_packet&)>& f);
 	/**
 	 * Current page from the sync state.
 	 *
@@ -187,6 +194,8 @@ public:
 	 *
 	 * Its memory is managed by libogg, inside the stream state, and is valid until the next
 	 * call to ogg_stream_packetout, wrapped by #read_packet.
+	 *
+	 * \todo Merge into #read_header_packet.
 	 */
 	ogg_packet packet;
 private:
@@ -210,12 +219,16 @@ private:
 	 *
 	 * To initialize it properly, we need the serialno of the stream, which is available only
 	 * after the first page was read.
+	 *
+	 * \todo Merge into #read_header_packet.
 	 */
 	bool stream_ready = false;
 	/**
 	 * Indicates if the stream's last fed page is the current one.
 	 *
 	 * Its state is irrelevant if the stream is not ready.
+	 *
+	 * \todo Merge into #read_header_packet.
 	 */
 	bool stream_in_sync;
 	/**
@@ -226,6 +239,8 @@ private:
 	 * better ensure there are no extra packets anyway.
 	 *
 	 * After we've read OpusHead and OpusTags, we don't need the stream layer anymore.
+	 *
+	 * \todo Merge into #read_header_packet.
 	 */
 	ogg_stream_state stream;
 };
@@ -264,6 +279,8 @@ public:
 	 * and is cheap to call.
 	 *
 	 * If the stream contains unflushed packets, they will be lost.
+	 *
+	 * \todo Merge into #write_header_packet.
 	 */
 	status prepare_stream(long serialno);
 	/**
@@ -275,11 +292,20 @@ public:
 	 * When the page is complete, you should call #flush_page to finalize the page.
 	 *
 	 * You must not call #write_page after it, until you call #flush_page.
+	 *
+	 * \todo Merge into #write_header_packet.
 	 */
 	status write_packet(const ogg_packet& packet);
+	/**
+	 * Write a header packet and flush the page. Header packets are always placed alone on their
+	 * pages.
+	 */
+	status write_header_packet(const ogg_packet& packet);
 	/**
 	 * Write the page under assembly. Future calls to #write_packet will be written in a new
 	 * page.
+	 *
+	 * \todo Merge into #write_header_packet.
 	 */
 	status flush_page();
 private:
@@ -288,6 +314,8 @@ private:
 	 *
 	 * In our specific use case, we only need it to put the OpusHead and OpusTags packets into
 	 * their own pages. The other pages are naively written to the output stream.
+	 *
+	 * \todo Merge into #write_header_packet.
 	 */
 	ogg_stream_state stream;
 	/**