perf: 5.4x faster recompression (11.6x single-worker), byte-identical output

CMakeLists.txt

@@ -33,5 +33,12 @@ target_link_libraries(mp3packercpp PRIVATE mp3packercpp_core)
 target_include_directories(mp3packercpp PRIVATE src/cli)
 target_compile_definitions(mp3packercpp PRIVATE MP3PACKERCPP_VERSION="${PROJECT_VERSION}")
 
+# --- Tests ---
+option(MP3PACKERCPP_BUILD_TESTS "Build the test programs" ${PROJECT_IS_TOP_LEVEL})
+if(MP3PACKERCPP_BUILD_TESTS)
+    enable_testing()
+    add_subdirectory(tests)
+endif()
+
 include(GNUInstallDirs)
 install(TARGETS mp3packercpp DESTINATION ${CMAKE_INSTALL_BINDIR})

src/core/include/bitstream.hpp

@@ -1,22 +1,95 @@
 #ifndef MP3PACKERCPP_BITSTREAM_HPP
 #define MP3PACKERCPP_BITSTREAM_HPP
 
-#include <vector>
-#include <stdexcept>
+#include <algorithm>
 #include <cstdint>
+#include <cstring>
+#include <stdexcept>
+#include <vector>
+
+#if defined(_MSC_VER)
+#include <intrin.h> // _byteswap_uint64
+#endif
 
 namespace mp3packer {
 
+/**
+ * @brief Loads eight bytes most-significant-byte first.
+ */
+inline uint64_t load_be64(const uint8_t* p) {
+    uint64_t v;
+    std::memcpy(&v, p, sizeof v);
+#if defined(_MSC_VER)
+    return _byteswap_uint64(v);
+#else
+    return __builtin_bswap64(v);
+#endif
+}
+
+/**
+ * @brief Stores eight bytes most-significant-byte first.
+ */
+inline void store_be64(uint8_t* p, const uint64_t v) {
+#if defined(_MSC_VER)
+    const uint64_t be = _byteswap_uint64(v);
+#else
+    const uint64_t be = __builtin_bswap64(v);
+#endif
+    std::memcpy(p, &be, sizeof be);
+}
+
 /**
  * @brief Utility class for reading arbitrary bits from a byte stream.
+ *
+ * Both directions of the bitstream work through a single 64-bit access per
+ * operation rather than walking bytes: no MP3 field is wider than 32 bits, and a
+ * whole coefficient pair fits in 47, so one load or read-modify-write covers any
+ * of them.
  */
 class BitstreamReader {
 public:
     /**
      * @brief Constructs a BitstreamReader wrapping the provided data vector.
      * @param data Reference to the vector of bytes to read from.
      */
-    explicit BitstreamReader(const std::vector<uint8_t>& data) : data_(data) {}
+    explicit BitstreamReader(const std::vector<uint8_t>& data)
+        : BitstreamReader(data.data(), data.size()) {}
+
+    /**
+     * @brief Constructs a BitstreamReader over a plain buffer.
+     * @param data Start of the bytes to read.
+     * @param size How many of them there are.
+     */
+    BitstreamReader(const uint8_t* data, const size_t size)
+        : data_(data), size_(size) {
+        // The tail is mirrored into a zero-filled buffer so that a read near the
+        // end of the data is the same load as any other: reads past the end yield
+        // zero bits, which is how the byte-walking reader behaved, and what a
+        // truncated final frame needs.
+        last_word_ = static_cast<ptrdiff_t>(size_) - 8;
+        tail_from_ = size_ > 7 ? size_ - 7 : 0;
+        std::memset(pad_, 0, sizeof pad_);
+        if (size_ > tail_from_) std::memcpy(pad_, data_ + tail_from_, size_ - tail_from_);
+    }
+
+    /**
+     * @brief Returns the 64 bits at an explicit bit position without consuming
+     *        them, most significant bit first, zero-filled past the end.
+     *
+     * Only the top 57 bits are guaranteed to be present, which is more than the 47
+     * a single coefficient pair can occupy. Taking a position rather than using the
+     * stored one lets a caller decoding a run of symbols keep its position in a
+     * register instead of round-tripping it through the reader.
+     */
+    [[nodiscard]] uint64_t peek_at(const size_t pos) const {
+        size_t idx = pos >> 3;
+        const uint8_t* b = data_;
+        if (static_cast<ptrdiff_t>(idx) > last_word_) {
+            b = pad_;
+            idx = std::min(idx - tail_from_, static_cast<size_t>(7));
+        }
+        return load_be64(b + idx) << (pos & 7);
+    }
 
     /**
      * @brief Reads a specified number of bits from the stream.
@@ -27,25 +100,26 @@ class BitstreamReader {
         if (num_bits == 0) return 0;
         if (num_bits > 32) throw std::invalid_argument("Cannot read more than 32 bits");
 
-        uint32_t result = 0;
-        for (int i = 0; i < num_bits; ++i) {
-            const size_t byte_idx = bit_pos_ / 8;
-            const int bit_idx = 7 - (bit_pos_ % 8);
-
-            if (byte_idx >= data_.size()) {
-                result <<= (num_bits - i);
-                bit_pos_ += static_cast<size_t>(num_bits - i);
-                return result;
-            }
-
-            if (data_[byte_idx] & (1 << bit_idx)) {
-                result |= (1 << (num_bits - 1 - i));
-            }
-            bit_pos_++;
+        uint32_t result = static_cast<uint32_t>(peek_at(bit_pos_) >> (64 - num_bits));
+        const size_t total = size_ * 8;
+        if (bit_pos_ + static_cast<size_t>(num_bits) > total) {
+            // A read that runs off the end of the data keeps the byte-walking
+            // reader's behaviour: the bits it did find stay at the top of the
+            // field and are then shifted up again by the number it did not, which
+            // is not the same as zero-filling them. Nothing downstream depends on
+            // the value — a frame reaching past the reservoir cannot be recompressed
+            // either way — but the bytes it eventually writes do.
+            const size_t avail = bit_pos_ < total ? total - bit_pos_ : 0;
+            const int missing = num_bits - static_cast<int>(avail);
+            result = missing >= 32 ? 0 : result << missing;
         }
+        bit_pos_ += static_cast<size_t>(num_bits);
         return result;
     }
 
+    /// Total number of bits in the underlying data.
+    [[nodiscard]] size_t size_bits() const { return size_ * 8; }
+
     /**
      * @brief Seeks to a specific bit position in the stream.
      * @param pos Absolute bit position to seek to.
@@ -59,12 +133,20 @@ class BitstreamReader {
     [[nodiscard]] size_t tell_bit() const { return bit_pos_; }
 
 private:
-    const std::vector<uint8_t>& data_; ///< Reference to the byte stream being read
-    size_t bit_pos_ = 0;               ///< Current absolute bit position
+    const uint8_t* data_;  ///< Start of the byte stream being read
+    size_t size_;          ///< Length of the byte stream
+    size_t bit_pos_ = 0;   ///< Current absolute bit position
+    ptrdiff_t last_word_;  ///< Highest byte index a whole word can be loaded from
+    size_t tail_from_;     ///< Byte index that pad_[0] stands for
+    uint8_t pad_[16];      ///< Zero-filled copy of the final bytes
 };
 
 /**
  * @brief Utility class for writing arbitrary bits to a dynamic byte stream.
+ *
+ * Pending bits are held in a register-sized accumulator and reach the buffer
+ * eight bytes at a time, so appending a field costs a shift and an or; nothing is
+ * ever read back out of the buffer.
  */
 class BitstreamWriter {
 public:
@@ -78,37 +160,111 @@ class BitstreamWriter {
     void write_bits(const uint32_t value, const int num_bits) {
         if (num_bits == 0) return;
         if (num_bits > 32) throw std::invalid_argument("Cannot write more than 32 bits");
+        put(static_cast<uint64_t>(value), num_bits);
+    }
+
+    /**
+     * @brief Appends the low num_bits bits of value, most significant first.
+     *
+     * A store leaves under eight bits pending, so anything up to 57 bits lands in a
+     * single accumulator window; that covers every field in the bitstream, a
+     * coefficient pair included.
+     */
+    void put(const uint64_t value, const int num_bits) {
+        if (accumulated_ + num_bits > 64) store(accumulator_, accumulated_);
+        accumulator_ |= (value & ((uint64_t{1} << num_bits) - 1)) << (64 - accumulated_ - num_bits);
+        accumulated_ += num_bits;
+    }
 
-        for (int i = 0; i < num_bits; ++i) {
-            const size_t byte_idx = bit_pos_ / 8;
-            const int bit_idx = 7 - (bit_pos_ % 8);
+    /**
+     * @brief Moves n bits from a reader to this writer without interpreting them.
+     *
+     * Used for scalefactors, which are re-emitted verbatim: their layout depends on
+     * scalefac_compress tables we only need to size, never to decode. The caller
+     * must have established that the whole span is inside the reader's data, since
+     * a read past the end has a value only field-by-field reading reproduces.
+     */
+    void copy_from(BitstreamReader& reader, size_t num_bits) {
+        while (num_bits >= 32) {
+            put(reader.read_bits(32), 32);
+            num_bits -= 32;
+        }
+        if (num_bits > 0) put(reader.read_bits(static_cast<int>(num_bits)), static_cast<int>(num_bits));
+    }
 
-            if (byte_idx >= data_.size()) {
-                data_.push_back(0);
-            }
+    /**
+     * @brief Takes the pending bits so a caller can append to them in registers.
+     *
+     * put() keeps its state in memory, so a long run of fields pays a load and a
+     * store of the accumulator per field, plus the store-to-load turnaround between
+     * one field and the next. A caller that takes the pending bits here, spills with
+     * store() whenever the next field would not fit, and hands them back with
+     * resume() pays none of that. The writer's own bits are not valid in between, so
+     * nothing else may write to it.
+     */
+    void pending(uint64_t& acc, int& nacc) const { acc = accumulator_; nacc = accumulated_; }
 
-            if (value & (1 << (num_bits - 1 - i))) {
-                data_[byte_idx] |= (1 << bit_idx);
-            }
-            bit_pos_++;
+    /**
+     * @brief Commits the accumulator's whole bytes, leaving under eight bits in it.
+     *
+     * Eight bytes are always stored and only the complete ones committed, so the
+     * next store overwrites the remainder: every byte the writer returns is one it
+     * wrote itself, which is why the buffer needs slack but needs no zeroing.
+     */
+    void store(uint64_t& acc, int& nacc) {
+        if (buf_.size() < committed_ + kSlack) {
+            buf_.resize(std::max(committed_ + kSlack, buf_.size() * 2));
         }
+        store_be64(buf_.data() + committed_, acc);
+        const int whole = nacc >> 3;
+        committed_ += static_cast<size_t>(whole);
+        // A field that exactly fills the accumulator leaves nothing over, and
+        // shifting a 64-bit value by 64 is undefined rather than zero.
+        acc = whole == 8 ? 0 : acc << whole * 8;
+        nacc -= whole * 8;
     }
 
+    /**
+     * @brief Returns the accumulator to the writer.
+     */
+    void resume(const uint64_t acc, const int nacc) { accumulator_ = acc; accumulated_ = nacc; }
+
     /**
      * @brief Gets the underlying byte vector containing the written data.
-     * @return Constant reference to the data vector.
+     *
+     * The pending bits are materialised without being committed, so a caller can
+     * read back what it has written so far and then carry on writing — which the
+     * verification pass does, once per granule.
+     *
+     * @return Constant reference to the data vector, zero-padded to a whole byte.
      */
-    [[nodiscard]] const std::vector<uint8_t>& data() const { return data_; }
+    [[nodiscard]] const std::vector<uint8_t>& data() {
+        if (accumulated_ > 0) {
+            uint64_t acc = accumulator_;
+            int nacc = accumulated_;
+            const size_t committed = committed_;
+            store(acc, nacc);
+            committed_ = committed; // the partial byte stays pending
+        }
+        buf_.resize((tell_bit() + 7) / 8);
+        return buf_;
+    }
 
     /**
      * @brief Gets the current bit position.
      * @return The absolute bit index the writer is currently at.
      */
-    [[nodiscard]] size_t tell_bit() const { return bit_pos_; }
+    [[nodiscard]] size_t tell_bit() const { return committed_ * 8 + static_cast<size_t>(accumulated_); }
 
 private:
-    std::vector<uint8_t> data_; ///< Dynamic byte stream being written
-    size_t bit_pos_ = 0;        ///< Current absolute bit position
+    /// A store always writes a whole word, of which it commits only the complete
+    /// bytes, so the buffer carries this much room past the output.
+    static constexpr size_t kSlack = 8;
+
+    std::vector<uint8_t> buf_;   ///< Committed bytes, plus slack for the next store
+    uint64_t accumulator_ = 0;   ///< Pending bits, most significant first
+    int accumulated_ = 0;        ///< How many of the accumulator's bits are pending
+    size_t committed_ = 0;       ///< Bytes already committed to buf_
 };
 
 } // namespace mp3packer

src/core/include/huffman.hpp

@@ -2,11 +2,23 @@
 #define MP3PACKERCPP_HUFFMAN_HPP
 
 #include "bitstream.hpp"
+#include <array>
 #include <vector>
 #include <cstdint>
 
 namespace mp3packer {
 
+/// Number of quantized spectral values in one granule.
+constexpr int kNumCoefficients = 576;
+
+/**
+ * @brief One granule's quantized coefficients.
+ *
+ * A fixed array rather than a vector: the coder runs once per granule, hundreds
+ * of thousands of times per second, and the caller reuses one buffer per worker.
+ */
+using Spectrum = std::array<int16_t, kNumCoefficients>;
+
 /**
  * @brief Represents the configuration of Huffman regions and tables for a granule.
  */
@@ -36,15 +48,16 @@ class HuffmanOptimizer {
      * The part2_3 bit limit is checked only at the start of each big-values pair and
      * each count1 quad, never mid-symbol. This matches mp3packer's decode behaviour.
      *
+     * @param coeffs Destination for the 576 coefficients, zero-filled past the
+     *               last decoded value.
      * @param config The original Huffman configuration from the side info.
      * @param reader Bitstream reader positioned at the start of the Huffman data
      *               (i.e. after scalefactors, at bit offset part2_length).
      * @param samplerate Sampling rate of the frame in Hz.
      * @param max_huffman_bits Maximum bits to consume (part2_3_length - part2_length).
      *                         Pass -1 to decode until big_values/count1 are exhausted.
-     * @return A vector of 576 integer coefficients (zero-padded beyond the last decoded value).
      */
-    static std::vector<int16_t> decode_quantized_coefficients(const HuffmanConfig& config, BitstreamReader& reader, uint32_t samplerate, int max_huffman_bits = -1);
+    static void decode_quantized_coefficients(Spectrum& coeffs, const HuffmanConfig& config, BitstreamReader& reader, uint32_t samplerate, int max_huffman_bits = -1);
 
     /**
      * @brief Performs brute-force search to find the optimal Huffman table combination.
@@ -53,7 +66,7 @@ class HuffmanOptimizer {
      * @param samplerate Sampling rate of the frame in Hz.
      * @return A new HuffmanConfig that yields the smallest bit size for the coefficients.
      */
-    static HuffmanConfig find_best_config(const std::vector<int16_t>& coeffs, const HuffmanConfig& orig_config, uint32_t samplerate);
+    static HuffmanConfig find_best_config(const Spectrum& coeffs, const HuffmanConfig& orig_config, uint32_t samplerate);
 
     /**
      * @brief Re-encodes the coefficients into a bitstream using a given configuration.
@@ -62,7 +75,27 @@ class HuffmanOptimizer {
      * @param writer Bitstream writer to output the compressed bits.
      * @param samplerate Sampling rate of the frame in Hz.
      */
-    static void encode_quantized_coefficients(const std::vector<int16_t>& coeffs, const HuffmanConfig& config, BitstreamWriter& writer, uint32_t samplerate);
+    static void encode_quantized_coefficients(const Spectrum& coeffs, const HuffmanConfig& config, BitstreamWriter& writer, uint32_t samplerate);
+
+    /**
+     * @brief Resolves one prefix of one big-value table exactly as the decoder does.
+     *
+     * Exposed for the tests: the decoder trusts the probe table for 99% of what it
+     * reads, and one wrong entry would decode a pair to the wrong magnitudes or
+     * desynchronise the region, so every entry is held against a walk of the tree.
+     *
+     * @param table_idx Big-value table index (0-31).
+     * @param prefix    The next probe_bits() bits of the stream, most significant first.
+     * @param x         Clamped magnitude of the first value, or unset if deferred.
+     * @param y         Clamped magnitude of the second value, or unset if deferred.
+     * @param length    Codeword length in bits, or unset if deferred.
+     * @param signs     Number of sign bits the pair takes, or unset if deferred.
+     * @return False if the codeword is longer than the probe, which defers to a tree walk.
+     */
+    static bool probe_pair(int table_idx, unsigned prefix, int& x, int& y, int& length, int& signs);
+
+    /** @brief How many bits of the stream index the big-value probe. */
+    static int probe_bits();
 
 };