encapsulate ogg_writer

src/cli.cc

@@ -226,15 +226,13 @@ static ot::status process_tags(const ogg_packet& packet, const ot::options& opt,
 	for (const std::string& comment : opt.to_add)
 		tags.comments.emplace_back(comment);
 
-	if (writer.file) {
+	if (writer) {
 		auto packet = ot::render_tags(tags);
-		if(ogg_stream_packetin(&writer.stream, &packet) == -1)
-			return ot::status::libogg_error;
+		return writer.write_packet(packet);
 	} else {
 		ot::print_comments(tags.comments, stdout);
+		return ot::status::ok;
 	}
-
-	return ot::status::ok;
 }
 
 /**
@@ -244,7 +242,7 @@ static ot::status process_tags(const ogg_packet& packet, const ot::options& opt,
 ot::status ot::process(ogg_reader& reader, ogg_writer& writer, const ot::options &opt)
 {
 	const char *error = nullptr;
-	int packet_count = -1;
+	int packet_count = 0;
 	while (error == nullptr) {
 		// Read the next page.
 		ot::status rc = reader.read_page();
@@ -258,22 +256,16 @@ ot::status ot::process(ogg_reader& reader, ogg_writer& writer, const ot::options
 			break;
 		}
 		// Short-circuit when the relevant packets have been read.
-		if (packet_count >= 2 && writer.file) {
-			if (ot::write_page(&reader.page, writer.file) == -1) {
-				error = "write_page: fwrite error";
+		if (packet_count >= 2 && writer) {
+			if (writer.write_page(reader.page) != ot::status::ok) {
+				error = "error writing ogg page";
 				break;
 			}
 			continue;
 		}
-		// Initialize the streams from the first page.
-		if (packet_count == -1) {
-			if (writer.file) {
-				if (ogg_stream_init(&writer.stream, ogg_page_serialno(&reader.page)) == -1) {
-					error = "ogg_stream_init: couldn't create an encoder";
-					break;
-				}
-			}
-			packet_count = 0;
+		if (writer && writer.prepare_stream(ogg_page_serialno(&reader.page)) != ot::status::ok) {
+			error = "ogg_stream_init: could not prepare the ogg stream";
+			break;
 		}
 		// Read all the packets.
 		while ((rc = reader.read_packet()) == ot::status::ok) {
@@ -290,30 +282,24 @@ ot::status ot::process(ogg_reader& reader, ogg_writer& writer, const ot::options
 					error = ot::error_message(rc);
 					break;
 				}
-				if (!writer.file)
+				if (!writer)
 					break; /* nothing else to do */
 				else
 					continue; /* process_tags wrote the new packet */
 			}
-			if (writer.file) {
-				if (ogg_stream_packetin(&writer.stream, &reader.packet) == -1) {
-					error = "ogg_stream_packetin: internal error";
-					break;
-				}
+			if (writer && writer.write_packet(reader.packet) != ot::status::ok) {
+				error = "error feeding the packet to the ogg stream";
+				break;
 			}
 		}
 		if (rc != ot::status::ok && rc != ot::status::end_of_page) {
 			error = "error reading the ogg packets";
 			break;
 		}
-		// Write the page.
-		if (writer.file) {
-			ogg_page page;
-			ogg_stream_flush(&writer.stream, &page);
-			if (ot::write_page(&page, writer.file) == -1)
-				error = "write_page: fwrite error";
-			else if (ogg_stream_check(&writer.stream) != 0)
-				error = "ogg_stream_check: internal error (encoder)";
+		// Write the assembled page.
+		if (writer && writer.flush_page() != ot::status::ok) {
+			error = "error flushing the ogg page";
+			break;
 		}
 	}
 	if (!error && packet_count < 2)

src/ogg.cc

@@ -62,7 +62,8 @@ ot::status ot::ogg_reader::read_packet()
 ot::ogg_writer::ogg_writer(FILE* output)
 	: file(output)
 {
-	memset(&stream, 0, sizeof(stream));
+	if (ogg_stream_init(&stream, 0) != 0)
+		throw std::bad_alloc();
 }
 
 ot::ogg_writer::~ogg_writer()
@@ -70,11 +71,42 @@ ot::ogg_writer::~ogg_writer()
 	ogg_stream_clear(&stream);
 }
 
-int ot::write_page(ogg_page *og, FILE *stream)
+ot::status ot::ogg_writer::write_page(const ogg_page& page)
 {
-	if((ssize_t) fwrite(og->header, 1, og->header_len, stream) < og->header_len)
-		return -1;
-	if((ssize_t) fwrite(og->body, 1, og->body_len, stream) < og->body_len)
-		return -1;
-	return 0;
+	if (page.header_len < 0 || page.body_len < 0)
+		return status::int_overflow;
+	auto header_len = static_cast<size_t>(page.header_len);
+	auto body_len = static_cast<size_t>(page.body_len);
+	if (fwrite(page.header, 1, header_len, file) < header_len)
+		return status::standard_error;
+	if (fwrite(page.body, 1, body_len, file) < body_len)
+		return status::standard_error;
+	return status::ok;
+}
+
+ot::status ot::ogg_writer::prepare_stream(long serialno)
+{
+	if (stream.serialno != serialno) {
+		if (ogg_stream_reset_serialno(&stream, serialno) != 0)
+			return status::libogg_error;
+	}
+	return status::ok;
+}
+
+ot::status ot::ogg_writer::write_packet(const ogg_packet& packet)
+{
+	if (ogg_stream_packetin(&stream, const_cast<ogg_packet*>(&packet)) != 0)
+		return status::libogg_error;
+	else
+		return status::ok;
+}
+
+ot::status ot::ogg_writer::flush_page()
+{
+	ogg_page page;
+	if (ogg_stream_flush(&stream, &page) != 0)
+		return write_page(page);
+	if (ogg_stream_check(&stream) != 0)
+		return status::libogg_error;
+	return status::ok; /* nothing was done */
 }

src/opustags.h

@@ -166,23 +166,70 @@ private:
 	ogg_stream_state stream;
 };
 
-struct ogg_writer {
+/**
+ * An Ogg writer lets you write ogg_page objets to an output file, and assemble packets into pages.
+ *
+ * It has two modes of operations :
+ *   1. call #write_page, or
+ *   2. call #prepare_stream, then #write_packet one or more times, followed by #flush_page.
+ *
+ * You can switch between the two modes, but must not start writing packets and then pages without
+ * flushing.
+ */
+class ogg_writer {
+public:
 	/**
-	 * Zeroes the stream state. You need to initialize it with the serialno.
-	 *
-	 * Initialize #file with the given handle. The caller is responsible for keeping the file
-	 * handle alive, and to close it.
+	 * 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();
+	/**
+	 * Returns true if the writer was open on a non-null file.
+	 *
+	 * \todo We should not create invalid writers instead.
+	 */
+	operator bool() const { return file != nullptr; }
+	/**
+	 * 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.
+	 */
+	status prepare_stream(long serialno);
+	/**
+	 * Add a packet to the current page under assembly.
+	 *
+	 * If the packet is coming from a different page, make sure the serial number fits by
+	 * calling #prepare_stream.
+	 *
+	 * 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.
+	 */
+	status write_packet(const ogg_packet& packet);
+	/**
+	 * Write the page under assembly. Future calls to #write_packet will be written in a new
+	 * page.
+	 */
+	status flush_page();
+private:
 	/**
 	 * The stream state receives packets and generates pages.
 	 *
-	 * 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.
+	 * 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.
 	 */
 	ogg_stream_state stream;
 	/**
@@ -192,8 +239,6 @@ struct ogg_writer {
 	FILE* file;
 };
 
-int write_page(ogg_page *og, FILE *stream);
-
 /**
  * Ogg packet with dynamically allocated data.
  *