fix writing large axml chunks

CHANGELOG.md

@@ -8,6 +8,8 @@ This version fixes a number of buffer overruns, integer overflows, and uses of u
 
 - library can now easier be used as a CMake subproject
 - new CMake option `BW64_PACKAGE_AND_INSTALL`
+- `AxmlChunk::data()`; this allows access to the internal string, avoiding a copy when reading
+- `Bw64Writer::close()`; this should be called before destruction to properly catch exceptions
 
 ### Changed
 
@@ -16,11 +18,13 @@ This version fixes a number of buffer overruns, integer overflows, and uses of u
 - Renamed CMake option `EXAMPLES` to `BW64_EXAMPLES`
 - `FormatInfoChunk::formatTag` now matches the formatTag in the file, rather than always returning 1
 - fmt parsing is stricter -- the chunk size must match the use of cbSize, and the presence if extra data is checked against the formatTag
+- strings can be moved into `AxmlChunk` with `std::make_shared<AxmlChunk>(std:move(some_str))`, to avoid a copy when writing
 
 ### Fixed
 
 - Fix sample rate parameter type in `writeFile()` and `BW64Writer` ctor to support 96k samplerates
 - fmt extra data is now written correctly
+- axml chunks greater than 4GB are now written correctly
 
 ## 0.10.0 - (January 18, 2019)
 ### Added

docs/tutorial.rst

@@ -53,6 +53,8 @@ table.
 
 Note that we don't need to close our file at the end of the programme. This will
 be done automatically when ``inFile`` is destroyed at the end of the programme.
+In real applications :cpp:func:`bw64::Bw64Reader::close()` should be calles
+before destruction to be able to properly catch exceptions.
 
 Write files
 -----------
@@ -93,6 +95,10 @@ passed to the :cpp:func:`bw64::Bw64Reader::read()` in the
 So also the :cpp:func:`bw64::Bw64Writer::write()` expects the order of the
 samples to be interleaved as described above.
 
+As with reading, the file will be automatically closed during destruction, but
+in real applications :cpp:func:`bw64::Bw64Writer::close()` should be called
+before destruction to be able to properly catch exceptions.
+
 Add signal processing
 ---------------------
 

flake.nix

@@ -0,0 +1,38 @@
+{
+  inputs.flake-utils.url = "github:numtide/flake-utils";
+  inputs.nixpkgs.url = "nixpkgs/nixos-23.05";
+
+  outputs = { self, nixpkgs, flake-utils }:
+    flake-utils.lib.eachDefaultSystem
+      (system:
+        let
+          pkgs = nixpkgs.legacyPackages.${system};
+
+          libbw64_pkg = { stdenv, cmake, ninja }:
+            stdenv.mkDerivation {
+              name = "libbw64";
+              src = ./.;
+              nativeBuildInputs = [ cmake ninja ];
+              doCheck = true;
+            };
+        in
+        rec {
+          packages = rec {
+            libbw64 = pkgs.callPackage libbw64_pkg { };
+
+            default = libbw64;
+          };
+
+          devShells = rec {
+            libbw64 = packages.libbw64.overrideAttrs (attrs: {
+              nativeBuildInputs = attrs.nativeBuildInputs ++ [
+                pkgs.clang-tools
+                pkgs.nixpkgs-fmt
+              ];
+            });
+
+            default = libbw64;
+          };
+        });
+}
+

include/bw64/chunks.hpp

@@ -256,23 +256,20 @@ namespace bw64 {
    public:
     static uint32_t Id() { return utils::fourCC("axml"); }
 
-    AxmlChunk(const std::string& axml) {
-      std::copy(axml.begin(), axml.end(), std::back_inserter(data_));
-    }
+    AxmlChunk(std::string axml) : data_(std::move(axml)) {}
 
     uint32_t id() const override { return AxmlChunk::Id(); }
     uint64_t size() const override { return data_.size(); }
 
     /*
      * @brief Write the AxmlChunk to a stream
      */
-    void write(std::ostream& stream) const override {
-      std::copy(data_.begin(), data_.end(),
-                std::ostreambuf_iterator<char>(stream));
-    }
+    void write(std::ostream& stream) const override { stream << data_; }
+
+    const std::string& data() const { return data_; }
 
    private:
-    std::vector<char> data_;
+    std::string data_;
   };
 
   /**

include/bw64/parser.hpp

@@ -120,12 +120,11 @@ namespace bw64 {
       errorString << "chunkId != 'axml'";
       throw std::runtime_error(errorString.str());
     }
-    std::string data;
-    data.resize(size);
+    std::string data(size, 0);
     // since c++11, std::string[0] returns a valid reference to a null byte for
     // size==0
     utils::readChunk(stream, &data[0], size);
-    return std::make_shared<AxmlChunk>(data);
+    return std::make_shared<AxmlChunk>(std::move(data));
   }
 
   ///@brief Parse AudioId from input stream

include/bw64/reader.hpp

@@ -81,13 +81,22 @@ namespace bw64 {
       seek(0);
     }
 
-    /**
-     * @brief Bw64Reader destructor
-     *
-     * The destructor will automatically close the file opened in the
-     * constructor.
-     */
-    ~Bw64Reader() { fileStream_.close(); }
+    /// close the file
+    ///
+    /// It is recommended to call this before the destructor, to handle
+    /// exceptions.
+    void close() {
+      if (!fileStream_.is_open()) return;
+
+      fileStream_.close();
+
+      if (!fileStream_.good())
+        throw std::runtime_error("file error detected when closing");
+    }
+
+    /// destructor; this will close the file if it has not already been done,
+    /// but it is recommended to call close() first to handle exceptions
+    ~Bw64Reader() { close(); }
 
     /// @brief Get file format (RIFF, BW64 or RF64)
     uint32_t fileFormat() const { return fileFormat_; }

include/bw64/writer.hpp

@@ -57,7 +57,8 @@ namespace bw64 {
         throw std::runtime_error(errorString.str());
       }
       writeRiffHeader();
-      writeChunkPlaceholder(utils::fourCC("JUNK"), 28u);
+      // 28 byte ds64 header + 12 byte entry for axml
+      writeChunkPlaceholder(utils::fourCC("JUNK"), 40u);
       auto formatChunk =
           std::make_shared<FormatInfoChunk>(channels, sampleRate, bitDepth);
       writeChunk(formatChunk);
@@ -73,22 +74,40 @@ namespace bw64 {
       writeChunk(dataChunk);
     }
 
-    /**
-     * @brief Finalize file
-     *
-     * This destructor will write all yet-to-be-written chunks to the file
-     * and will also finalize all required information, i.e. the final chunk
-     * sizes etc.
-     */
-    ~Bw64Writer() {
-      finalizeDataChunk();
-      for (auto chunk : postDataChunks_) {
-        writeChunk(chunk);
+    /// finalise and close the file
+    ///
+    /// Write all yet-to-be-written chunks to the file and finalize all
+    /// required information, i.e. the final chunk sizes etc.
+    ///
+    /// It is recommended to call this before the destructor, to handle
+    /// exceptions. If it does throw, this object may be in an invalid state,
+    /// so do not try again without creating a new object.
+    void close() {
+      if (!fileStream_.is_open()) return;
+
+      try {
+        finalizeDataChunk();
+        for (auto chunk : postDataChunks_) {
+          writeChunk(chunk);
+        }
+        finalizeRiffChunk();
+        fileStream_.close();
+      } catch (...) {
+        // ensure that if an exception is thrown the file is still closed, so
+        // the destructor does not throw the same exception
+        fileStream_.close();
+        throw;
       }
-      finalizeRiffChunk();
-      fileStream_.close();
+
+      if (!fileStream_.good())
+        throw std::runtime_error("file error detected when closing");
     }
 
+    /// destructor; this will finalise and close the file if it has not
+    /// already been done, but it is recommended to call close() first to
+    /// handle exceptions
+    ~Bw64Writer() { close(); }
+
     /// @brief Get format tag
     uint16_t formatTag() const { return formatChunk()->formatTag(); };
     /// @brief Get number of channels
@@ -150,9 +169,10 @@ namespace bw64 {
       if (riffChunkSize() > UINT32_MAX) {
         return true;
       }
-      if (dataChunk()->size() > UINT32_MAX) {
-        return true;
-      }
+
+      for (auto& header : chunkHeaders_)
+        if (header.size > UINT32_MAX) return true;
+
       return false;
     }
 
@@ -222,8 +242,13 @@ namespace bw64 {
     void overwriteJunkWithDs64Chunk() {
       auto ds64Chunk = std::make_shared<DataSize64Chunk>();
       ds64Chunk->bw64Size(riffChunkSize());
+      // write data size even if it's not too big
       ds64Chunk->dataSize(dataChunk()->size());
-      // TODO: add other chunks which are bigger than 4GB
+
+      for (auto& header : chunkHeaders_)
+        if (header.size > UINT32_MAX)
+          ds64Chunk->setChunkSize(header.id, header.size);
+
       overwriteChunk(utils::fourCC("JUNK"), ds64Chunk);
     }
 
@@ -260,6 +285,15 @@ namespace bw64 {
     /// @brief Overwrite chunk template
     template <typename ChunkType>
     void overwriteChunk(uint32_t id, std::shared_ptr<ChunkType> chunk) {
+      if (chunk->size() > chunkHeader(id).size) {
+        std::stringstream errorMsg;
+        errorMsg << utils::fourCCToStr(chunk->id()) << " chunk is too large ("
+                 << chunk->size() << " bytes) to overwrite "
+                 << utils::fourCCToStr(id) << " chunk (" << chunkHeader(id).size
+                 << " bytes)";
+        throw std::runtime_error(errorMsg.str());
+      }
+
       auto last_position = fileStream_.tellp();
       seekChunk(id);
       utils::writeChunk<ChunkType>(fileStream_, chunk, chunkSizeForHeader(id));

submodules/catch2.cmake

@@ -1,3 +1,4 @@
 add_library(catch2 STATIC ${PROJECT_SOURCE_DIR}/submodules/catch2/catch_main.cpp)
 target_include_directories(catch2 PUBLIC ${PROJECT_SOURCE_DIR}/submodules)
 target_compile_features(catch2 PUBLIC cxx_auto_type cxx_trailing_return_types)
+target_compile_definitions(catch2 PUBLIC CATCH_CONFIG_ENABLE_BENCHMARKING)

tests/chunk_tests.cpp

@@ -353,3 +353,57 @@ TEST_CASE("ds64_chunk") {
         std::runtime_error);
   }
 }
+
+TEST_CASE("axml_chunk") {
+  size_t size = 200;
+
+  std::string str_data(size, 0);
+  const char* axml_str = "AXML";
+  for (size_t i = 0; i < size; i++) str_data[i] = axml_str[i % 4];
+
+  // check that null bytes are passed correctly
+  str_data[100] = 0;
+
+  AxmlChunk chunk(str_data);
+
+  REQUIRE(chunk.size() == size);
+  REQUIRE(chunk.data() == str_data);
+
+  std::ostringstream stream;
+  chunk.write(stream);
+
+  REQUIRE(stream.tellp() == size);
+  REQUIRE(stream.str() == str_data);
+}
+
+TEST_CASE("axml_chunk_bench", "[.bench]") {
+  size_t size = 10000000;
+
+  std::string str_data(size, 0);
+  const char* pattern = "AXML";
+  for (size_t i = 0; i < size; i++) str_data[i] = pattern[i % 4];
+
+  BENCHMARK("from string copy") {
+    AxmlChunk chunk(str_data);
+    return chunk.size();
+  };
+
+  BENCHMARK_ADVANCED("from string move")(Catch::Benchmark::Chronometer meter) {
+    std::string str_data_copy(str_data);
+
+    meter.measure([&] {
+      AxmlChunk chunk(std::move(str_data_copy));
+      return chunk.size();
+    });
+  };
+
+  BENCHMARK_ADVANCED("write")(Catch::Benchmark::Chronometer meter) {
+    AxmlChunk chunk(str_data);
+    std::ostringstream stream;
+
+    meter.measure([&] {
+      chunk.write(stream);
+      return stream.tellp();
+    });
+  };
+}