From 1ff5284b605a4a08b0d280935d5b0bc13da161bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Mangano-Tarumi?= Date: Mon, 3 Dec 2018 20:07:00 -0500 Subject: [PATCH] process the streams by page instead of packets --- src/cli.cc | 62 +++++++++++++++++++++----------------------------- src/ogg.cc | 28 +++++++++++++++++++++++ src/opustags.h | 28 +++++++++++++++++++++++ 3 files changed, 82 insertions(+), 36 deletions(-) diff --git a/src/cli.cc b/src/cli.cc index 0139112..60eec6e 100644 --- a/src/cli.cc +++ b/src/cli.cc @@ -184,8 +184,8 @@ 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 packet_count = 0; - for (;;) { + int page_no; + for (page_no = 0;; ++page_no) { // Read the next page. ot::status rc = reader.read_page(); if (rc == ot::st::end_of_stream) @@ -193,7 +193,7 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const else if (rc != ot::st::ok) return rc; // Short-circuit when the relevant packets have been read. - if (packet_count >= 2 && writer) { + if (page_no >= 2 && writer) { if ((rc = writer->write_page(reader.page)) != ot::st::ok) return rc; continue; @@ -201,42 +201,32 @@ static ot::status process(ot::ogg_reader& reader, ot::ogg_writer* writer, const auto serialno = ogg_page_serialno(&reader.page); if (writer && (rc = writer->prepare_stream(serialno)) != ot::st::ok) return rc; - // Read all the packets. - for (;;) { - rc = reader.read_packet(); - if (rc == ot::st::end_of_page) + if (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 + ot::opus_tags tags; + rc = reader.read_header_packet( + [&tags](ogg_packet& p) { return ot::parse_tags(p, tags); }); + if (rc != ot::st::ok) + return rc; + if ((rc = edit_tags(tags, opt)) != ot::st::ok) + return rc; + if (writer) { + auto packet = ot::render_tags(tags); + if ((rc = writer->write_header_packet(packet)) != ot::st::ok) + return rc; + } else { + ot::print_comments(tags.comments, stdout); break; - else if (rc != ot::st::ok) - return rc; - packet_count++; - if (packet_count == 1) { // Identification header - rc = ot::validate_identification_header(reader.packet); - if (rc != ot::st::ok) - return rc; - } else if (packet_count == 2) { // Comment header - ot::opus_tags tags; - if ((rc = ot::parse_tags(reader.packet, tags)) != ot::st::ok) - return rc; - if ((rc = edit_tags(tags, opt)) != ot::st::ok) - return rc; - if (writer) { - auto packet = ot::render_tags(tags); - if ((rc = writer->write_packet(packet)) != ot::st::ok) - return rc; - continue; - } else { - ot::print_comments(tags.comments, stdout); - } } - if (writer && (rc = writer->write_packet(reader.packet)) != ot::st::ok) - return rc; - } - // Write the assembled page. - if (writer && (rc = writer->flush_page()) != ot::st::ok) - return rc; + } /** \todo Move the generic case here, after we've gotten rid of prepare_stream. */ } - if (packet_count < 2) - return {ot::st::error, "Expected at least 2 Ogg packets"}; + if (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 e7d981a..8948f43 100644 --- a/src/ogg.cc +++ b/src/ogg.cc @@ -71,6 +71,23 @@ ot::status ot::ogg_reader::read_packet() return {st::libogg_error, "ogg_stream_packetout failed"}; } +ot::status ot::ogg_reader::read_header_packet(const std::function& f) +{ + ot::status rc = read_packet(); + if (rc == ot::st::end_of_page) + return {ot::st::error, "Header pages must not be empty."}; + else if (rc != ot::st::ok) + return rc; + if ((rc = f(packet)) != ot::st::ok) + return rc; + rc = read_packet(); + if (rc == ot::st::ok) + return {ot::st::error, "Unexpected second packet in header page."}; + else if (rc != ot::st::end_of_page) + return rc; + return ot::st::ok; +} + ot::ogg_writer::ogg_writer(FILE* output) : file(output) { @@ -113,6 +130,17 @@ ot::status ot::ogg_writer::write_packet(const ogg_packet& packet) return st::ok; } +ot::status ot::ogg_writer::write_header_packet(const ogg_packet& packet) +{ + ot::status rc; + if ((rc = write_packet(packet)) != ot::st::ok) + return rc; + return flush_page(); +} + +/** + * \todo What if the packet is too big to fit a single page? + */ ot::status ot::ogg_writer::flush_page() { ogg_page page; diff --git a/src/opustags.h b/src/opustags.h index e98c5d0..a3b7d60 100644 --- a/src/opustags.h +++ b/src/opustags.h @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -173,8 +174,14 @@ public: * return #status::stream_not_ready. * * After the last packet was read, return #status::end_of_page. + * + * \todo Merge into #read_header_packet. */ status read_packet(); + /** + * Read the single packet contained in a header page, and call the function f on it. + */ + status read_header_packet(const std::function& f); /** * Current page from the sync state. * @@ -187,6 +194,8 @@ public: * * Its memory is managed by libogg, inside the stream state, and is valid until the next * call to ogg_stream_packetout, wrapped by #read_packet. + * + * \todo Merge into #read_header_packet. */ ogg_packet packet; private: @@ -210,12 +219,16 @@ private: * * To initialize it properly, we need the serialno of the stream, which is available only * after the first page was read. + * + * \todo Merge into #read_header_packet. */ bool stream_ready = false; /** * Indicates if the stream's last fed page is the current one. * * Its state is irrelevant if the stream is not ready. + * + * \todo Merge into #read_header_packet. */ bool stream_in_sync; /** @@ -226,6 +239,8 @@ private: * better ensure there are no extra packets anyway. * * After we've read OpusHead and OpusTags, we don't need the stream layer anymore. + * + * \todo Merge into #read_header_packet. */ ogg_stream_state stream; }; @@ -264,6 +279,8 @@ public: * 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); /** @@ -275,11 +292,20 @@ public: * 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. + * + * \todo Merge into #write_header_packet. */ status write_packet(const ogg_packet& packet); + /** + * Write a header packet and flush the page. Header packets are always placed alone on their + * pages. + */ + status write_header_packet(const ogg_packet& packet); /** * Write the page under assembly. Future calls to #write_packet will be written in a new * page. + * + * \todo Merge into #write_header_packet. */ status flush_page(); private: @@ -288,6 +314,8 @@ private: * * 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; /**