From 7ae7a501511e47b0006f36e5619b5b59be0064fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= Date: Tue, 6 Nov 2018 20:46:34 -0500 Subject: [PATCH] store comments in a std::list --- src/opus.cc | 94 +++++++++++++++++-------------------------------- src/opustags.cc | 2 +- src/opustags.h | 28 +++++++++++---- t/cli.t | 2 +- t/unit.cc | 14 ++++---- 5 files changed, 64 insertions(+), 76 deletions(-) diff --git a/src/opus.cc b/src/opus.cc index 06b9be8..28fa397 100644 --- a/src/opus.cc +++ b/src/opus.cc @@ -34,9 +34,6 @@ #define le32toh(x) OSSwapLittleToHostInt32(x) #endif -/** - * \todo Use RAII. Here the allocated objects are not even properly freed on error. - */ int ot::parse_tags(const char *data, long len, opus_tags *tags) { long pos; @@ -52,29 +49,19 @@ int ot::parse_tags(const char *data, long len, opus_tags *tags) if (pos + 4 > len) return -1; // Count - tags->count = le32toh(*((uint32_t*) (data + pos))); - if (tags->count == 0) - return 0; - tags->lengths = static_cast(calloc(tags->count, sizeof(uint32_t))); - if (tags->lengths == NULL) - return -1; - tags->comment = static_cast(calloc(tags->count, sizeof(char*))); - if (tags->comment == NULL) { - free(tags->lengths); - return -1; - } + uint32_t count = le32toh(*((uint32_t*) (data + pos))); pos += 4; - // Comment - uint32_t i; - for (i=0; icount; i++) { - tags->lengths[i] = le32toh(*((uint32_t*) (data + pos))); - tags->comment[i] = data + pos + 4; - pos += 4 + tags->lengths[i]; + // Comments + for (uint32_t i = 0; i < count; ++i) { + uint32_t comment_length = le32toh(*((uint32_t*) (data + pos))); + const char *comment_value = data + pos + 4; + tags->comments.emplace_back(comment_value, comment_length); + pos += 4 + comment_length; if (pos > len) return -1; } // Extra data - tags->extra_data = ot::string_view{data + pos, static_cast(len - pos)}; + tags->extra_data = ot::string_view(data + pos, static_cast(len - pos)); return 0; } @@ -86,9 +73,8 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op) op->granulepos = 0; op->packetno = 1; long len = 8 + 4 + tags->vendor_length + 4; - uint32_t i; - for (i=0; icount; i++) - len += 4 + tags->lengths[i]; + for (const string_view &comment : tags->comments) + len += 4 + comment.size; len += tags->extra_data.size; op->bytes = len; char *data = static_cast(malloc(len)); @@ -101,19 +87,22 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op) memcpy(data+8, &n, 4); memcpy(data+12, tags->vendor_string, tags->vendor_length); data += 12 + tags->vendor_length; - n = htole32(tags->count); + n = htole32(tags->comments.size()); memcpy(data, &n, 4); data += 4; - for (i=0; icount; i++) { - n = htole32(tags->lengths[i]); + for (const string_view &comment : tags->comments) { + n = htole32(comment.size); memcpy(data, &n, 4); - memcpy(data+4, tags->comment[i], tags->lengths[i]); - data += 4 + tags->lengths[i]; + memcpy(data+4, comment.data, comment.size); + data += 4 + comment.size; } memcpy(data, tags->extra_data.data, tags->extra_data.size); return 0; } +/** + * \todo Make the field name case-insensitive? + */ static int match_field(const char *comment, uint32_t len, const char *field) { size_t field_len; @@ -129,52 +118,35 @@ static int match_field(const char *comment, uint32_t len, const char *field) void ot::delete_tags(opus_tags *tags, const char *field) { - uint32_t i; - for (i=0; icount; i++) { - if (match_field(tags->comment[i], tags->lengths[i], field)) { - // We want to delete the current element, so we move the last tag at - // position i, then decrease the array size. We need decrease i to inspect - // at the next iteration the tag we just moved. - tags->count--; - tags->lengths[i] = tags->lengths[tags->count]; - tags->comment[i] = tags->comment[tags->count]; - --i; - // No need to resize the arrays. - } + 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); } } +/** + * \todo Return void. + */ int ot::add_tags(opus_tags *tags, const char **tags_to_add, uint32_t count) { - if (count == 0) - return 0; - uint32_t *lengths = static_cast(realloc(tags->lengths, (tags->count + count) * sizeof(uint32_t))); - const char **comment = static_cast(realloc(tags->comment, (tags->count + count) * sizeof(char*))); - if (lengths == NULL || comment == NULL) - return -1; - tags->lengths = lengths; - tags->comment = comment; - uint32_t i; - for (i=0; ilengths[tags->count + i] = strlen(tags_to_add[i]); - tags->comment[tags->count + i] = tags_to_add[i]; - } - tags->count += count; + for (uint32_t i = 0; i < count; ++i) + tags->comments.emplace_back(tags_to_add[i]); return 0; } void ot::print_tags(opus_tags *tags) { - for (uint32_t i=0; icount; i++) { - fwrite(tags->comment[i], 1, tags->lengths[i], stdout); + for (const string_view &comment : tags->comments) { + fwrite(comment.data, 1, comment.size, stdout); puts(""); } } +/** + * \todo Delete this function. + */ void ot::free_tags(opus_tags *tags) { - if (tags->count > 0) { - free(tags->lengths); - free(tags->comment); - } } diff --git a/src/opustags.cc b/src/opustags.cc index 635a4ca..d79045f 100644 --- a/src/opustags.cc +++ b/src/opustags.cc @@ -244,7 +244,7 @@ int main(int argc, char **argv){ break; } if(delete_all) - tags.count = 0; + tags.comments.clear(); else{ int i; for(i=0; i + #include #include -#include +#include +#include namespace ot { @@ -13,8 +16,15 @@ namespace ot { * Non-owning string, similar to what std::string_view is in C++17. */ struct string_view { - const char *data; - size_t size; + string_view() {}; + string_view(const char *data) : data(data), size(strlen(data)) {}; + string_view(const char *data, size_t size) : data(data), size(size) {} + bool operator==(const string_view &other) const { + return size == other.size && memcmp(data, other.data, size) == 0; + } + bool operator!=(const string_view &other) const { return !(*this == other); } + const char *data = nullptr; + size_t size = 0; }; /** @@ -40,10 +50,16 @@ int write_page(ogg_page *og, FILE *stream); */ struct opus_tags { uint32_t vendor_length; + /** \todo Convert to a string view. */ const char *vendor_string; - uint32_t count; - uint32_t *lengths; - const char **comment; + /** + * Comments. These are a list of string following the NAME=Value format. + * A comment may also be called a field, or a tag. + * + * The field name in vorbis comment is case-insensitive and ASCII, + * while the value can be any valid UTF-8 string. + */ + std::list comments; /** * According to RFC 7845: * > Immediately following the user comment list, the comment header MAY contain diff --git a/t/cli.t b/t/cli.t index 06e88b2..f284ec9 100644 --- a/t/cli.t +++ b/t/cli.t @@ -126,8 +126,8 @@ X=3 EOF is_deeply(opustags("$t/out.opus", qw(-d A -d foo -s X=4 -a TITLE=gobble -d TITLE), undef), [<<'EOF', '', 0], 'dry editing'); -1=2 encoder=whatever +1=2 X=4 TITLE=gobble EOF diff --git a/t/unit.cc b/t/unit.cc index 11c40fc..d582411 100644 --- a/t/unit.cc +++ b/t/unit.cc @@ -46,14 +46,14 @@ static bool parse_standard() throw failure("the vendor string length is invalid"); if (memcmp(tags.vendor_string, "opustags test packet", 20) != 0) throw failure("the vendor string is invalid"); - if (tags.count != 2) + if (tags.comments.size() != 2) throw failure("bad number of comments"); - if (tags.lengths[0] != 9 || tags.lengths[1] != 10) - throw failure("bad comment lengths"); - if (memcmp(tags.comment[0], "TITLE=Foo", tags.lengths[0]) != 0) - throw failure("bad title comment"); - if (memcmp(tags.comment[1], "ARTIST=Bar", tags.lengths[1]) != 0) - throw failure("bad artist comment"); + auto it = tags.comments.begin(); + if (*it != ot::string_view("TITLE=Foo")) + throw failure("bad title"); + ++it; + if (*it != ot::string_view("ARTIST=Bar")) + throw failure("bad artist"); if (tags.extra_data.size != 0) throw failure("found mysterious padding data"); ot::free_tags(&tags);