diff --git a/src/cli.cc b/src/cli.cc index 510c6f0..05de3d8 100644 --- a/src/cli.cc +++ b/src/cli.cc @@ -357,14 +357,7 @@ static void process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::op { bool focused = false; /*< the stream on which we operate is defined */ int focused_serialno; /*< when focused, the serialno of the focused stream */ - for (;;) { - ot::status rc = reader.next_page(); - if (rc == ot::st::end_of_stream) - break; - else if (rc == ot::st::bad_stream && reader.absolute_page_no == (size_t) -1) - throw ot::status {ot::st::bad_stream, "Input is not a valid Ogg file."}; - else if (rc != ot::st::ok) - throw rc; + while (reader.next_page()) { auto serialno = ogg_page_serialno(&reader.page); auto pageno = ogg_page_pageno(&reader.page); if (!focused) { @@ -377,17 +370,17 @@ static void process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::op if (reader.absolute_page_no == 0) { // Identification header if (!ot::is_opus_stream(reader.page)) throw ot::status {ot::st::error, "Not an Opus stream."}; - if (writer) { - rc = writer->write_page(reader.page); - if (rc != ot::st::ok) - throw rc; - } + if (writer) + writer->write_page(reader.page); } else if (reader.absolute_page_no == 1) { // Comment header ot::opus_tags tags; - rc = reader.process_header_packet( - [&tags](ogg_packet& p) { return ot::parse_tags(p, tags); }); - if (rc != ot::st::ok) - throw rc; + reader.process_header_packet( + [&tags](ogg_packet& p) { + ot::status rc = ot::parse_tags(p, tags); + if (rc != ot::st::ok) + throw rc; + } + ); edit_tags(tags, opt); if (writer) { if (opt.edit_interactively) { @@ -395,16 +388,13 @@ static void process(ot::ogg_reader& reader, ot::ogg_writer* writer, const ot::op edit_tags_interactively(tags, writer->path, opt.raw); } auto packet = ot::render_tags(tags); - rc = writer->write_header_packet(serialno, pageno, packet); - if (rc != ot::st::ok) - throw rc; + writer->write_header_packet(serialno, pageno, packet); } else { ot::print_comments(tags.comments, stdout, opt.raw); break; } - } else { - if (writer && (rc = writer->write_page(reader.page)) != ot::st::ok) - throw rc; + } else if (writer) { + writer->write_page(reader.page); } } if (reader.absolute_page_no < 1) diff --git a/src/ogg.cc b/src/ogg.cc index 5ce260f..068b99c 100644 --- a/src/ogg.cc +++ b/src/ogg.cc @@ -24,94 +24,95 @@ bool ot::is_opus_stream(const ogg_page& identification_header) return (memcmp(identification_header.body, "OpusHead", 8) == 0); } -ot::status ot::ogg_reader::next_page() +bool ot::ogg_reader::next_page() { int rc; while ((rc = ogg_sync_pageout(&sync, &page)) != 1) { - if (rc == -1) - return {st::bad_stream, "Unsynced data in stream."}; + if (rc == -1) { + throw status {st::bad_stream, + absolute_page_no == (size_t) -1 ? "Input is not a valid Ogg file." + : "Unsynced data in stream."}; + } if (ogg_sync_check(&sync) != 0) - return {st::libogg_error, "ogg_sync_check signalled an error."}; + throw status {st::libogg_error, "ogg_sync_check signalled an error."}; if (feof(file)) { if (sync.fill != sync.returned) - return {st::bad_stream, "Unsynced data at end of stream."}; - return {st::end_of_stream, "End of stream was reached."}; + throw status {st::bad_stream, "Unsynced data at end of stream."}; + return false; // end of sream } char* buf = ogg_sync_buffer(&sync, 65536); if (buf == nullptr) - return {st::libogg_error, "ogg_sync_buffer failed."}; + throw status {st::libogg_error, "ogg_sync_buffer failed."}; size_t len = fread(buf, 1, 65536, file); if (ferror(file)) - return {st::standard_error, "fread error: "s + strerror(errno)}; + throw status {st::standard_error, "fread error: "s + strerror(errno)}; if (ogg_sync_wrote(&sync, len) != 0) - return {st::libogg_error, "ogg_sync_wrote failed."}; + throw status {st::libogg_error, "ogg_sync_wrote failed."}; } ++absolute_page_no; - return st::ok; + return true; } -ot::status ot::ogg_reader::process_header_packet(const std::function& f) +void ot::ogg_reader::process_header_packet(const std::function& f) { if (ogg_page_continued(&page)) - return {ot::st::error, "Unexpected continued header page."}; + throw status {ot::st::error, "Unexpected continued header page."}; ogg_logical_stream stream(ogg_page_serialno(&page)); stream.pageno = ogg_page_pageno(&page); if (ogg_stream_pagein(&stream, &page) != 0) - return {st::libogg_error, "ogg_stream_pagein failed."}; + throw status {st::libogg_error, "ogg_stream_pagein failed."}; + ogg_packet packet; int rc = ogg_stream_packetout(&stream, &packet); if (ogg_stream_check(&stream) != 0 || rc == -1) - return {ot::st::libogg_error, "ogg_stream_packetout failed."}; + throw status {ot::st::libogg_error, "ogg_stream_packetout failed."}; else if (rc == 0) - return {ot::st::error, - "Reading header packets spanning multiple pages are not yet supported. " - "Please file an issue to make your wish known."}; - ot::status f_rc = f(packet); - if (f_rc != ot::st::ok) - return f_rc; + throw status {ot::st::error, + "Reading header packets spanning multiple pages are not yet supported. " + "Please file an issue to make your wish known."}; + + f(packet); + /* Ensure that there are no other segments left in the packet using the lacing state of the * stream. These are the relevant variables, as far as I understood them: * - lacing_vals: extensible array containing the lacing values of the segments, * - lacing_fill: number of elements in lacing_vals (not the capacity), * - lacing_returned: index of the next segment to be processed. */ if (stream.lacing_returned != stream.lacing_fill) - return {ot::st::error, "Header page contains more than a single packet."}; - return ot::st::ok; + throw status {ot::st::error, "Header page contains more than a single packet."}; } -ot::status ot::ogg_writer::write_page(const ogg_page& page) +void ot::ogg_writer::write_page(const ogg_page& page) { if (page.header_len < 0 || page.body_len < 0) - return {st::int_overflow, "Overflowing page length"}; + throw status {st::int_overflow, "Overflowing page length"}; auto header_len = static_cast(page.header_len); auto body_len = static_cast(page.body_len); if (fwrite(page.header, 1, header_len, file) < header_len) - return {st::standard_error, "fwrite error: "s + strerror(errno)}; + throw status {st::standard_error, "fwrite error: "s + strerror(errno)}; if (fwrite(page.body, 1, body_len, file) < body_len) - return {st::standard_error, "fwrite error: "s + strerror(errno)}; - return st::ok; + throw status {st::standard_error, "fwrite error: "s + strerror(errno)}; } -ot::status ot::ogg_writer::write_header_packet(int serialno, int pageno, ogg_packet& packet) +void ot::ogg_writer::write_header_packet(int serialno, int pageno, ogg_packet& packet) { ogg_logical_stream stream(serialno); stream.b_o_s = (pageno != 0); stream.pageno = pageno; if (ogg_stream_packetin(&stream, &packet) != 0) - return {ot::st::libogg_error, "ogg_stream_packetin failed"}; + throw status {ot::st::libogg_error, "ogg_stream_packetin failed"}; + ogg_page page; - if (ogg_stream_flush(&stream, &page) != 0) { - ot::status rc = write_page(page); - if (rc != ot::st::ok) - return rc; - } else { - return {ot::st::libogg_error, "ogg_stream_flush failed"}; - } if (ogg_stream_flush(&stream, &page) != 0) - return {ot::st::error, - "Writing header packets spanning multiple pages are not yet supported. " - "Please file an issue to make your wish known."}; + write_page(page); + else + throw status {ot::st::libogg_error, "ogg_stream_flush failed"}; + + if (ogg_stream_flush(&stream, &page) != 0) + throw status {ot::st::error, + "Writing header packets spanning multiple pages are not yet supported. " + "Please file an issue to make your wish known."}; + if (ogg_stream_check(&stream) != 0) - return {st::libogg_error, "ogg_stream_check failed"}; - return ot::st::ok; + throw status {st::libogg_error, "ogg_stream_check failed"}; } diff --git a/src/opustags.h b/src/opustags.h index 595b05a..3f7cc86 100644 --- a/src/opustags.h +++ b/src/opustags.h @@ -68,7 +68,6 @@ enum class st { child_process_failed, /* Ogg */ bad_stream, - end_of_stream, libogg_error, /* Opus */ bad_magic_number, @@ -239,9 +238,9 @@ struct ogg_reader { * is made available in the #page field, is owned by the Ogg reader, and is valid until the * next call to #read_page. * - * After the last page was read, return #status::end_of_stream. + * Return true if a page was read, false on end of stream. */ - status next_page(); + bool next_page(); /** * Read the single packet contained in the last page read, assuming it's a header page, and * call the function f on it. This function has no side effect, and calling it twice on the @@ -250,7 +249,7 @@ struct ogg_reader { * It is currently limited to packets that fit on a single page, and should be later * extended to support packets spanning multiple pages. */ - status process_header_packet(const std::function& f); + void process_header_packet(const std::function& f); /** * Current page from the sync state. * @@ -298,12 +297,12 @@ struct ogg_writer { * * This is a basic I/O operation and does not even require libogg, or the stream. */ - status write_page(const ogg_page& page); + void write_page(const ogg_page& page); /** * Write a header packet and flush the page. Header packets are always placed alone on their * pages. */ - status write_header_packet(int serialno, int pageno, ogg_packet& packet); + void write_header_packet(int serialno, int pageno, ogg_packet& packet); /** * Output file. It should be opened in binary mode. We use it to write whole pages, * represented as a block of data and a length. diff --git a/t/ogg.cc b/t/ogg.cc index 1c170d8..d7504de 100644 --- a/t/ogg.cc +++ b/t/ogg.cc @@ -11,37 +11,27 @@ static void check_ref_ogg() ot::ogg_reader reader(input.get()); - ot::status rc = reader.next_page(); - if (rc != ot::st::ok) + if (reader.next_page() != true) throw failure("could not read the first page"); if (!ot::is_opus_stream(reader.page)) throw failure("failed to identify the stream as opus"); - rc = reader.process_header_packet([](ogg_packet& p) { + reader.process_header_packet([](ogg_packet& p) { if (p.bytes != 19) throw failure("unexpected length for the first packet"); - return ot::st::ok; }); - if (rc != ot::st::ok) - throw failure("could not read the first packet"); - rc = reader.next_page(); - if (rc != ot::st::ok) + if (reader.next_page() != true) throw failure("could not read the second page"); - rc = reader.process_header_packet([](ogg_packet& p) { + reader.process_header_packet([](ogg_packet& p) { if (p.bytes != 62) throw failure("unexpected length for the second packet"); - return ot::st::ok; }); - if (rc != ot::st::ok) - throw failure("could not read the second packet"); while (!ogg_page_eos(&reader.page)) { - rc = reader.next_page(); - if (rc != ot::st::ok) + if (reader.next_page() != true) throw failure("failure reading a page"); } - rc = reader.next_page(); - if (rc != ot::st::end_of_stream) + if (reader.next_page() != false) throw failure("did not correctly detect the end of stream"); } @@ -67,7 +57,6 @@ static void check_memory_ogg() ogg_packet second_packet = make_packet("Second"); std::vector my_ogg(128); size_t my_ogg_size; - ot::status rc; { ot::file output = fmemopen(my_ogg.data(), my_ogg.size(), "w"); @@ -75,11 +64,7 @@ static void check_memory_ogg() throw failure("could not open the output stream"); ot::ogg_writer writer(output.get()); writer.write_header_packet(1234, 0, first_packet); - if (rc != ot::st::ok) - throw failure("could not write the first packet"); writer.write_header_packet(1234, 1, second_packet); - if (rc != ot::st::ok) - throw failure("could not write the second packet"); my_ogg_size = ftell(output.get()); if (my_ogg_size != 67) throw failure("unexpected output size"); @@ -90,28 +75,19 @@ static void check_memory_ogg() if (input == nullptr) throw failure("could not open the input stream"); ot::ogg_reader reader(input.get()); - rc = reader.next_page(); - if (rc != ot::st::ok) + if (reader.next_page() != true) throw failure("could not read the first page"); - rc = reader.process_header_packet([&first_packet](ogg_packet &p) { + reader.process_header_packet([&first_packet](ogg_packet &p) { if (!same_packet(p, first_packet)) throw failure("unexpected content in the first packet"); - return ot::st::ok; }); - if (rc != ot::st::ok) - throw failure("could not read the first packet"); - rc = reader.next_page(); - if (rc != ot::st::ok) + if (reader.next_page() != true) throw failure("could not read the second page"); - rc = reader.process_header_packet([&second_packet](ogg_packet &p) { + reader.process_header_packet([&second_packet](ogg_packet &p) { if (!same_packet(p, second_packet)) throw failure("unexpected content in the second packet"); - return ot::st::ok; }); - if (rc != ot::st::ok) - throw failure("could not read the second packet"); - rc = reader.next_page(); - if (rc != ot::st::end_of_stream) + if (reader.next_page() != false) throw failure("unexpected third page"); } } @@ -121,9 +97,13 @@ void check_bad_stream() auto err_msg = "did not detect the stream is not an ogg stream"; ot::file input = fmemopen((void*) err_msg, 20, "r"); ot::ogg_reader reader(input.get()); - ot::status rc = reader.next_page(); - if (rc != ot::st::bad_stream) - throw failure(err_msg); + try { + reader.next_page(); + throw failure("did not raise an error"); + } catch (const ot::status& rc) { + if (rc != ot::st::bad_stream) + throw failure(err_msg); + } } void check_identification()