diff --git a/src/opus.cc b/src/opus.cc index 971ab61..06b9be8 100644 --- a/src/opus.cc +++ b/src/opus.cc @@ -34,6 +34,9 @@ #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; @@ -70,10 +73,8 @@ int ot::parse_tags(const char *data, long len, opus_tags *tags) if (pos > len) return -1; } - - if (pos < len) - fprintf(stderr, "warning: %ld unused bytes at the end of the OpusTags packet\n", len - pos); - + // Extra data + tags->extra_data = ot::string_view{data + pos, static_cast(len - pos)}; return 0; } @@ -88,6 +89,7 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op) uint32_t i; for (i=0; icount; i++) len += 4 + tags->lengths[i]; + len += tags->extra_data.size; op->bytes = len; char *data = static_cast(malloc(len)); if (!data) @@ -108,6 +110,7 @@ int ot::render_tags(opus_tags *tags, ogg_packet *op) memcpy(data+4, tags->comment[i], tags->lengths[i]); data += 4 + tags->lengths[i]; } + memcpy(data, tags->extra_data.data, tags->extra_data.size); return 0; } diff --git a/src/opustags.h b/src/opustags.h index 0b3a752..d68aa4b 100644 --- a/src/opustags.h +++ b/src/opustags.h @@ -9,6 +9,14 @@ namespace ot { +/** + * Non-owning string, similar to what std::string_view is in C++17. + */ +struct string_view { + const char *data; + size_t size; +}; + /** * \defgroup ogg Ogg * \brief Helpers to work with libogg. @@ -29,8 +37,6 @@ int write_page(ogg_page *og, FILE *stream); /** * Represent all the data in an OpusTags packet. - * - * \todo The comment list may be followed by arbitrary binary data, which we should keep here. */ struct opus_tags { uint32_t vendor_length; @@ -38,6 +44,18 @@ struct opus_tags { uint32_t count; uint32_t *lengths; const char **comment; + /** + * According to RFC 7845: + * > Immediately following the user comment list, the comment header MAY contain + * > zero-padding or other binary data that is not specified here. + * + * The first byte is supposed to indicate whether this data should be kept or not, but let's + * assume it's here for a reason and always keep it. Better safe than sorry. + * + * In the future, we could add options to manipulate this data: view it, edit it, truncate + * it if it's marked as padding, truncate it unconditionally. + */ + string_view extra_data; }; int parse_tags(const char *data, long len, opus_tags *tags); diff --git a/t/unit.cc b/t/unit.cc index c405353..11c40fc 100644 --- a/t/unit.cc +++ b/t/unit.cc @@ -54,6 +54,8 @@ static bool parse_standard() throw failure("bad title comment"); if (memcmp(tags.comment[1], "ARTIST=Bar", tags.lengths[1]) != 0) throw failure("bad artist comment"); + if (tags.extra_data.size != 0) + throw failure("found mysterious padding data"); ot::free_tags(&tags); return true; } @@ -94,6 +96,11 @@ static bool recode_padding() int rc = ot::parse_tags(padded_OpusTags.data(), padded_OpusTags.size(), &tags); if (rc != 0) throw failure("ot::parse_tags did not return 0"); + if (tags.extra_data.size != 6) + throw failure("unexpected amount of extra bytes"); + if (memcmp(tags.extra_data.data, "\0hello", 6) != 0) + throw failure("corrupted extra data"); + // recode the packet and ensure it's exactly the same ogg_packet packet; rc = ot::render_tags(&tags, &packet); if (packet.bytes < padded_OpusTags.size())