3
\$\begingroup\$

For a personal project (reading media file metadata) I created a small/fast utility class for working with RIFF chunks.

Support:

  • defining chunks by 32 bit integral value
  • defining chunks using 4 ASCII characters
  • splitting chunks into 2 16 bit integral values (useful for parsing RIFF headers)
  • comparing chunks for equality

Not supported (could be added but I didn't need it for a windows-only home project):

  • endian-ness changes

header file:

#include <array>
#include <cstdint>
#include <stdexcept>
#include <string>

namespace wpcs::riff
{
    /// @brief chunk of data (as specified by RIFF format)
    ///
    /// @see http://soundfile.sapp.org/doc/WaveFormat/
    class chunk final
    {
        std::array<std::byte, 4> data_;

    public:
        explicit chunk(std::uint32_t data = {});

        std::string string() const;
        std::uint32_t uint32() const;
        std::array<std::uint16_t, 2> uint16s() const;

        inline bool operator==(chunk const &x) const
        {
            return data_ == x.data_;
        }

        inline bool operator!=(chunk const &x) const
        {
            return data_ != x.data_;
        }
    };

    inline namespace literals
    {
        inline auto operator""_chunk(char const *const str, std::size_t size)
        {
            if (size != 4)
            {
                std::string msg{"Bad chunk: "};
                std::copy(str, str + size, msg.end());
                throw std::logic_error{msg};
            }
            auto value = std::uint32_t{};
            std::copy(str, str + size, reinterpret_cast<char *>(&value));
            return chunk{value};
        }

        inline auto operator""_chunk(unsigned long long x)
        {
            return chunk{static_cast<std::uint32_t>(x)};
        }
    } // namespace literals

} // namespace wpcs::riff

implementation file:

#include <riff/chunk.hpp>

namespace wpcs::riff
{
    chunk::chunk(std::uint32_t data)
        : data_{}
    {
        auto start = reinterpret_cast<std::byte *>(&data);
        auto stop = start + sizeof(data);
        std::copy(start, stop, begin(data_));
    }

    std::uint32_t chunk::uint32() const
    {
        return *reinterpret_cast<std::uint32_t const *const>(&data_[0]);
    }

    std::string chunk::string() const
    {
        constexpr auto const zero = std::byte{0};
        constexpr auto const null_char = '.';

        auto result = std::string(4, null_char);
        std::transform(begin(data_), end(data_), begin(result), [&](std::byte b) {
            if (b == zero)
                return null_char;
            return static_cast<char>(b);
        });
        return result;
    }

    std::array<std::uint16_t, 2> chunk::uint16s() const
    {
        auto result = std::array<std::uint16_t, 2>{};

        result[0] = static_cast<std::uint8_t>(data_[1]);
        result[0] <<= 8;
        result[0] += static_cast<std::uint8_t>(data_[0]);

        result[1] = static_cast<std::uint8_t>(data_[3]);
        result[1] <<= 8;
        result[1] += static_cast<std::uint8_t>(data_[2]);

        return result;
    }

} // namespace wpcs::riff

usage (tests written using catch2):

#define CATCH_CONFIG_MAIN
#include <catch2/catch.hpp>

#include <riff/chunk.hpp>

TEST_CASE("wpcs::riff::chunk", "[chunk]")
{
    GIVEN("an empty chunk")
    {
        auto const c = wpcs::riff::chunk{};

        WHEN("a string representation is created")
        {
            THEN("the string is filled with dots (representing null clars)")
            {
                REQUIRE(c.string() == "....");
            }
        }

        WHEN("a unit32 representation is created")
        {
            THEN("the representation is filled with zeros")
            {
                auto const value = c.uint32();
                REQUIRE(value == std::uint32_t{});
            }
        }

        WHEN("a uint16 representation is created")
        {
            THEN("the representation contains two zeros")
            {
                auto const values = c.uint16s();
                REQUIRE(values.size() == 2);
                REQUIRE(values[0] == std::uint16_t{});
                REQUIRE(values[1] == std::uint16_t{});
            }
        }
    }

    GIVEN("a user defined string literal representation of a chunk")
    {
        using namespace wpcs::riff::literals;

        auto const c = "WAVE"_chunk;

        THEN("it's representations are correct")
        {
            REQUIRE(c.uint32() == 0x45564157);
            REQUIRE(c.string() == "WAVE");

            auto const short_ints = c.uint16s();
            REQUIRE(short_ints[0] == 0x4157);
            REQUIRE(short_ints[1] == 0x4556);
        }
    }

    GIVEN("a user defined numeric literal representation of a chunk")
    {
        using namespace wpcs::riff::literals;

        auto const c = 1163280727_chunk;

        THEN("it's representations are correct")
        {
            REQUIRE(c.uint32() == 1163280727);
            REQUIRE(c.string() == "WAVE");

            auto const short_ints = c.uint16s();
            REQUIRE(short_ints[0] == 0x4157);
            REQUIRE(short_ints[1] == 0x4556);
        }
    }
}

Please review for possible code improvements and code style.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

This does not represent RIFF chunks

A RIFF chunk is more than just the 4 byte identifier. A chunk consists of a header, data and padding bytes. Furthermore, the header contains the identifier as well as the size of the data.

Is the identifier an uin32_t or an array of 4 std::bytes?

It is a bit weird to store the identifier internally as a std::array<std::byte, 4>, but none of the public member functions use that format. A lot of the code is unnecessarily complex just because you have to convert to and from that format.

I would either just store it as a uint32_t internally, to match what you pass into the constructor, or alternatively, since a RIFF chunk identifier consists of 4 ASCII characters, I would just define it as char id[4].

Unnecessary use of user-defined literals

Instead of declaring user-defined literals, I would just overload the constructor to take a C string as an argument. This way, instead of "WAVE"_chunk, you can write chunk("WAVE"), which is about the same length, and much less surprising.

Unaligned access

You are using type punning to convert std::array<std::byte, 4> to uint32_t, but there is no guarantee that std::array<std::byte, 4> has the same alignment as uint32_t. While a chunk declared on the stack might be properly aligned in practice, it is not that hard to have a situation where a chunk is not aligned as you expect. Consider for example:

struct foo {
    char x;
    wcps::riff::chunk chunk;
};

You will find that sizeof(struct foo) is 5, and offsetoff(struct foo, chunk) is 1. Converting to uint32_t using type-punning is undefined behavior, and might crash on platforms that do not support unaligned loads and stores.

Writing out of bounds

In this code:

std::string msg{"Bad chunk: "};
std::copy(str, str + size, msg.end());
throw std::logic_error{msg};

You are explicitly copying str to after the end of msg. This is a buffer overflow. Instead, I would just write:

throw std::logic_error(std::string("Bad chunk: ") + str::string(str, size));
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.