From 14ae681e6114d90f3a0104ff344cffb59d921d16 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= <fmang@mg0.fr>
Date: Wed, 5 Dec 2018 18:03:53 -0500
Subject: [PATCH] get rid of ogg_writer::prepare_stream

---
 src/cli.cc     | 36 ++++++++++++++++++------------------
 src/ogg.cc     | 26 ++++----------------------
 src/opustags.h | 46 +++++++++++++++++-----------------------------
 t/ogg.cc       | 10 ++--------
 4 files changed, 41 insertions(+), 77 deletions(-)

diff --git a/src/cli.cc b/src/cli.cc
index 60eec6e..b4cd8c3 100644
--- a/src/cli.cc
+++ b/src/cli.cc
@@ -184,30 +184,26 @@ 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 page_no;
-	for (page_no = 0;; ++page_no) {
-		// Read the next page.
+	/** \todo Become stream-aware instead of counting the pages of all streams together. */
+	int absolute_page_no; /*< page number in the physical stream, not logical */
+	for (absolute_page_no = 0;; ++absolute_page_no) {
 		ot::status rc = reader.read_page();
 		if (rc == ot::st::end_of_stream)
 			break;
 		else if (rc != ot::st::ok)
 			return rc;
-		// Short-circuit when the relevant packets have been read.
-		if (page_no >= 2 && writer) {
-			if ((rc = writer->write_page(reader.page)) != ot::st::ok)
-				return rc;
-			continue;
-		}
 		auto serialno = ogg_page_serialno(&reader.page);
-		if (writer && (rc = writer->prepare_stream(serialno)) != ot::st::ok)
-			return rc;
-		if (page_no == 0) { // Identification header
+		auto pageno = ogg_page_pageno(&reader.page);
+		if (absolute_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
+			if (writer) {
+				rc = writer->write_header_packet(serialno, pageno, reader.packet);
+				if (rc != ot::st::ok)
+					return rc;
+			}
+		} else if (absolute_page_no == 1) { // Comment header
 			ot::opus_tags tags;
 			rc = reader.read_header_packet(
 				[&tags](ogg_packet& p) { return ot::parse_tags(p, tags); });
@@ -217,15 +213,19 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const
 				return rc;
 			if (writer) {
 				auto packet = ot::render_tags(tags);
-				if ((rc = writer->write_header_packet(packet)) != ot::st::ok)
+				rc = writer->write_header_packet(serialno, pageno, packet);
+				if (rc != ot::st::ok)
 					return rc;
 			} else {
 				ot::print_comments(tags.comments, stdout);
 				break;
 			}
-		} /** \todo Move the generic case here, after we've gotten rid of prepare_stream. */
+		} else {
+			if (writer && (rc = writer->write_page(reader.page)) != ot::st::ok)
+				return rc;
+		}
 	}
-	if (page_no < 1)
+	if (absolute_page_no < 1)
 		return {ot::st::error, "Expected at least 2 Ogg pages."};
 	return ot::st::ok;
 }
diff --git a/src/ogg.cc b/src/ogg.cc
index ee4bbf8..87b647b 100644
--- a/src/ogg.cc
+++ b/src/ogg.cc
@@ -91,18 +91,6 @@ ot::status ot::ogg_reader::read_header_packet(const std::function<status(ogg_pac
 	return ot::st::ok;
 }
 
-ot::ogg_writer::ogg_writer(FILE* output)
-	: file(output)
-{
-	if (ogg_stream_init(&stream, 0) != 0)
-		throw std::bad_alloc();
-}
-
-ot::ogg_writer::~ogg_writer()
-{
-	ogg_stream_clear(&stream);
-}
-
 ot::status ot::ogg_writer::write_page(const ogg_page& page)
 {
 	if (page.header_len < 0 || page.body_len < 0)
@@ -116,17 +104,11 @@ ot::status ot::ogg_writer::write_page(const ogg_page& page)
 	return st::ok;
 }
 
-ot::status ot::ogg_writer::prepare_stream(long serialno)
-{
-	if (stream.serialno != serialno) {
-		if (ogg_stream_reset_serialno(&stream, serialno) != 0)
-			return {st::libogg_error, "ogg_stream_reset_serialno failed"};
-	}
-	return st::ok;
-}
-
-ot::status ot::ogg_writer::write_header_packet(ogg_packet& packet)
+ot::status ot::ogg_writer::write_header_packet(int serialno, int pageno, ogg_packet& packet)
 {
+	ogg_stream stream(serialno);
+	stream.b_o_s = (pageno != 0);
+	stream.pageno = pageno;
 	if (ogg_stream_packetin(&stream, &packet) != 0)
 		return {ot::st::libogg_error, "ogg_stream_packetin failed"};
 	ogg_page page;
diff --git a/src/opustags.h b/src/opustags.h
index 2764fed..d6eab44 100644
--- a/src/opustags.h
+++ b/src/opustags.h
@@ -137,6 +137,17 @@ private:
  * \{
  */
 
+/** RAII-aware wrapper around libogg's ogg_stream_state. */
+struct ogg_stream : ogg_stream_state {
+	ogg_stream(int serialno) {
+		if (ogg_stream_init(this, serialno) != 0)
+			throw std::bad_alloc();
+	}
+	~ogg_stream() {
+		ogg_stream_clear(this);
+	}
+};
+
 /**
  * Ogg reader, combining a FILE input, an ogg_sync_state reading the pages, and an ogg_stream_state
  * extracting the packets from the page.
@@ -247,50 +258,27 @@ private:
 
 /**
  * An Ogg writer lets you write ogg_page objets to an output file, and assemble packets into pages.
+ *
+ * Its packet writing facility is limited to writing single-page header packets, because that's all
+ * we need for opustags.
  */
-class ogg_writer {
-public:
+struct ogg_writer {
 	/**
 	 * Initialize the writer with the given output file handle. The caller is responsible for
 	 * keeping the file handle alive, and to close it.
 	 */
-	ogg_writer(FILE* output);
-	/**
-	 * Clears the stream state and any internal memory. Does not close the output file.
-	 */
-	~ogg_writer();
+	explicit ogg_writer(FILE* output) : file(output) {}
 	/**
 	 * Write a whole Ogg page into the output stream.
 	 *
 	 * This is a basic I/O operation and does not even require libogg, or the stream.
 	 */
 	status write_page(const ogg_page& page);
-	/**
-	 * Prepare the stream with the given Ogg serial number.
-	 *
-	 * If the stream is already configured with the right serial number, it doesn't do anything
-	 * 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);
 	/**
 	 * Write a header packet and flush the page. Header packets are always placed alone on their
 	 * pages.
 	 */
-	status write_header_packet(ogg_packet& packet);
-private:
-	/**
-	 * The stream state receives packets and generates pages.
-	 *
-	 * 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;
+	status write_header_packet(int serialno, int pageno, ogg_packet& packet);
 	/**
 	 * Output file. It should be opened in binary mode. We use it to write whole pages,
 	 * represented as a block of data and a length.
diff --git a/t/ogg.cc b/t/ogg.cc
index 5362098..dcbbc44 100644
--- a/t/ogg.cc
+++ b/t/ogg.cc
@@ -74,16 +74,10 @@ static void check_memory_ogg()
 		if (output == nullptr)
 			throw failure("could not open the output stream");
 		ot::ogg_writer writer(output.get());
-		rc = writer.prepare_stream(1234);
-		if (rc != ot::st::ok)
-			throw failure("could not prepare the stream for the first page");
-		writer.write_header_packet(first_packet);
+		writer.write_header_packet(1234, 0, first_packet);
 		if (rc != ot::st::ok)
 			throw failure("could not write the first packet");
-		writer.prepare_stream(1234);
-		if (rc != ot::st::ok)
-			throw failure("could not prepare the stream for the second page");
-		writer.write_header_packet(second_packet);
+		writer.write_header_packet(1234, 1, second_packet);
 		if (rc != ot::st::ok)
 			throw failure("could not write the second packet");
 		my_ogg_size = ftell(output.get());