From 0980b35ecd26d2c0448a61496e8a8ff1b249dc0d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= <fmang@mg0.fr>
Date: Sat, 17 Nov 2018 17:34:51 -0500
Subject: [PATCH] polish the interface of the opus module

---
 src/cli.cc     |  4 ++--
 src/opus.cc    | 39 ++++++++++++++++++++++++++-------------
 src/opustags.h | 17 +++++++++++++++--
 t/opus.cc      | 36 ++++++++++++++++++++++++++----------
 4 files changed, 69 insertions(+), 27 deletions(-)

diff --git a/src/cli.cc b/src/cli.cc
index f91ad36..849531d 100644
--- a/src/cli.cc
+++ b/src/cli.cc
@@ -211,14 +211,14 @@ std::list<std::string> ot::read_comments(FILE* input)
 static ot::status process_tags(const ogg_packet& packet, const ot::options& opt, ot::ogg_writer* writer)
 {
 	ot::opus_tags tags;
-	if(ot::parse_tags((char*) packet.packet, packet.bytes, &tags) != ot::status::ok)
+	if(ot::parse_tags(packet, tags) != ot::status::ok)
 		return ot::status::bad_comment_header;
 
 	if (opt.delete_all) {
 		tags.comments.clear();
 	} else {
 		for (const std::string& name : opt.to_delete)
-			ot::delete_tags(&tags, name.c_str());
+			ot::delete_comments(tags, name.c_str());
 	}
 
 	if (opt.set_all)
diff --git a/src/opus.cc b/src/opus.cc
index 5f35aad..d795202 100644
--- a/src/opus.cc
+++ b/src/opus.cc
@@ -46,16 +46,24 @@ ot::status ot::validate_identification_header(const ogg_packet& packet)
 	return ot::status::ok;
 }
 
-ot::status ot::parse_tags(const char *data, long len, opus_tags *tags)
+/**
+ * \todo See if the packet's data could be casted more nicely into a string.
+ */
+ot::status ot::parse_tags(const ogg_packet& packet, opus_tags& tags)
 {
-	if (len < 0)
+	if (packet.bytes < 0)
 		return status::int_overflow;
-	size_t size = static_cast<size_t>(len);
+	size_t size = static_cast<size_t>(packet.bytes);
+	const char* data = reinterpret_cast<char*>(packet.packet);
 	size_t pos = 0;
+	opus_tags my_tags;
+
+	// Magic number
 	if (8 > size)
 		return status::overflowing_magic_number;
 	if (memcmp(data, "OpusTags", 8) != 0)
 		return status::bad_magic_number;
+
 	// Vendor
 	pos = 8;
 	if (pos + 4 > size)
@@ -63,14 +71,16 @@ ot::status ot::parse_tags(const char *data, long len, opus_tags *tags)
 	size_t vendor_length = le32toh(*((uint32_t*) (data + pos)));
 	if (pos + 4 + vendor_length > size)
 		return status::overflowing_vendor_data;
-	tags->vendor = std::string(data + pos + 4, vendor_length);
-	pos += 4 + tags->vendor.size();
-	// Count
+	my_tags.vendor = std::string(data + pos + 4, vendor_length);
+	pos += 4 + my_tags.vendor.size();
+
+	// Comment count
 	if (pos + 4 > size)
 		return status::overflowing_comment_count;
 	uint32_t count = le32toh(*((uint32_t*) (data + pos)));
 	pos += 4;
-	// Comments
+
+	// Comments' data
 	for (uint32_t i = 0; i < count; ++i) {
 		if (pos + 4 > size)
 			return status::overflowing_comment_length;
@@ -78,11 +88,14 @@ ot::status ot::parse_tags(const char *data, long len, opus_tags *tags)
 		if (pos + 4 + comment_length > size)
 			return status::overflowing_comment_data;
 		const char *comment_value = data + pos + 4;
-		tags->comments.emplace_back(comment_value, comment_length);
+		my_tags.comments.emplace_back(comment_value, comment_length);
 		pos += 4 + comment_length;
 	}
+
 	// Extra data
-	tags->extra_data = std::string(data + pos, size - pos);
+	my_tags.extra_data = std::string(data + pos, size - pos);
+
+	tags = std::move(my_tags);
 	return status::ok;
 }
 
@@ -136,12 +149,12 @@ static int match_field(const char *comment, uint32_t len, const char *field)
 	return 1;
 }
 
-void ot::delete_tags(opus_tags *tags, const char *field)
+void ot::delete_comments(opus_tags& tags, const char* field_name)
 {
-	auto it = tags->comments.begin(), end = tags->comments.end();
+	auto it = tags.comments.begin(), end = tags.comments.end();
 	while (it != end) {
 		auto current = it++;
-		if (match_field(current->data(), current->size(), field))
-			tags->comments.erase(current);
+		if (match_field(current->data(), current->size(), field_name))
+			tags.comments.erase(current);
 	}
 }
diff --git a/src/opustags.h b/src/opustags.h
index 93c843c..377b3c1 100644
--- a/src/opustags.h
+++ b/src/opustags.h
@@ -301,9 +301,22 @@ struct opus_tags {
  */
 status validate_identification_header(const ogg_packet& packet);
 
-status parse_tags(const char *data, long len, opus_tags *tags);
+/**
+ * Read the given OpusTags packet and extract its content into an opus_tags object.
+ *
+ * On error, the tags object is left unchanged.
+ */
+status parse_tags(const ogg_packet& packet, opus_tags& tags);
+
+/**
+ * Serialize an #opus_tags object into an OpusTags Ogg packet.
+ */
 dynamic_ogg_packet render_tags(const opus_tags& tags);
-void delete_tags(opus_tags *tags, const char *field);
+
+/**
+ * Remove all the comments whose field name is equal to the special one, case-sensitive.
+ */
+void delete_comments(opus_tags& tags, const char* field_name);
 
 /** \} */
 
diff --git a/t/opus.cc b/t/opus.cc
index 83e60b0..1a27c4a 100644
--- a/t/opus.cc
+++ b/t/opus.cc
@@ -41,7 +41,10 @@ static const char standard_OpusTags[] =
 static void parse_standard()
 {
 	ot::opus_tags tags;
-	auto rc = ot::parse_tags(standard_OpusTags, sizeof(standard_OpusTags) - 1, &tags);
+	ogg_packet op;
+	op.bytes = sizeof(standard_OpusTags) - 1;
+	op.packet = (unsigned char*) standard_OpusTags;
+	auto rc = ot::parse_tags(op, tags);
 	if (rc != ot::status::ok)
 		throw failure("ot::parse_tags did not return ok");
 	if (tags.vendor != "opustags test packet")
@@ -69,6 +72,9 @@ static void parse_corrupted()
 	char packet[size];
 	memcpy(packet, standard_OpusTags, size);
 	ot::opus_tags tags;
+	ogg_packet op;
+	op.packet = (unsigned char*) packet;
+	op.bytes = size;
 
 	char* header_data = packet;
 	char* vendor_length = header_data + 8;
@@ -78,36 +84,42 @@ static void parse_corrupted()
 	char* first_comment_data = first_comment_length + 4;
 	char* end = packet + size;
 
-	if (ot::parse_tags(packet, 7, &tags) != ot::status::overflowing_magic_number)
+	op.bytes = 7;
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_magic_number)
 		throw failure("did not detect the overflowing magic number");
-	if (ot::parse_tags(packet, 11, &tags) != ot::status::overflowing_vendor_length)
+	op.bytes = 11;
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_vendor_length)
 		throw failure("did not detect the overflowing vendor string length");
+	op.bytes = size;
 
 	header_data[0] = 'o';
-	if (ot::parse_tags(packet, size, &tags) != ot::status::bad_magic_number)
+	if (ot::parse_tags(op, tags) != ot::status::bad_magic_number)
 		throw failure("did not detect the bad magic number");
 	header_data[0] = 'O';
 
 	*vendor_length = end - vendor_string + 1;
-	if (ot::parse_tags(packet, size, &tags) != ot::status::overflowing_vendor_data)
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_vendor_data)
 		throw failure("did not detect the overflowing vendor string");
 	*vendor_length = end - vendor_string - 3;
-	if (ot::parse_tags(packet, size, &tags) != ot::status::overflowing_comment_count)
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_comment_count)
 		throw failure("did not detect the overflowing comment count");
 	*vendor_length = comment_count - vendor_string;
 
 	++*comment_count;
-	if (ot::parse_tags(packet, size, &tags) != ot::status::overflowing_comment_length)
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_comment_length)
 		throw failure("did not detect the overflowing comment length");
 	*first_comment_length = end - first_comment_data + 1;
-	if (ot::parse_tags(packet, size, &tags) != ot::status::overflowing_comment_data)
+	if (ot::parse_tags(op, tags) != ot::status::overflowing_comment_data)
 		throw failure("did not detect the overflowing comment data");
 }
 
 static void recode_standard()
 {
 	ot::opus_tags tags;
-	auto rc = ot::parse_tags(standard_OpusTags, sizeof(standard_OpusTags) - 1, &tags);
+	ogg_packet op;
+	op.bytes = sizeof(standard_OpusTags) - 1;
+	op.packet = (unsigned char*) standard_OpusTags;
+	auto rc = ot::parse_tags(op, tags);
 	if (rc != ot::status::ok)
 		throw failure("ot::parse_tags did not return ok");
 	auto packet = ot::render_tags(tags);
@@ -131,7 +143,11 @@ static void recode_padding()
 	std::string padded_OpusTags(standard_OpusTags, sizeof(standard_OpusTags));
 	// ^ note: padded_OpusTags ends with a null byte here
 	padded_OpusTags += "hello";
-	auto rc = ot::parse_tags(padded_OpusTags.data(), padded_OpusTags.size(), &tags);
+	ogg_packet op;
+	op.bytes = padded_OpusTags.size();
+	op.packet = (unsigned char*) padded_OpusTags.data();
+
+	auto rc = ot::parse_tags(op, tags);
 	if (rc != ot::status::ok)
 		throw failure("ot::parse_tags did not return ok");
 	if (tags.extra_data != "\0hello"s)