1
\$\begingroup\$

I'm unsure whether the way I'm performing initialization is recommended, and whether there is a simpler way to save out a struct like the one I'm using. The main points of feedback I'm looking for are around the use of memcpys and reinterpret_casts or (char*)& casts and whether they are unavoidable.

Additional background for a reader is I needed to convert from my system's little endian format to bigendian which is also required for mp4 by the spec I'm reading. Unfortunately this reverses things like the uint32 made from "ftyp", and so I have written "pytf" instead as a temporary work around.

struct Box
{
    uint32_t size;
    uint32_t boxtype; // this is the fourcc type so it's a 32 because I believe it is actually 4 8chars together
};
struct FTBox : Box
{
    uint32_t brand;
    uint32_t minor;
    uint32_t compatible_brands[2] = {0,0};
};

void run_functions()
{

    stringstream tt;
    FTBox r{};
    cout << sizeof(r) << endl;
    string mytype = "pytf";
    memcpy(&r.boxtype, mytype.data(), 4);
    cout << "type is  " << r.boxtype << endl;
    cout << sizeof(r) << endl;
    r.minor = 0;
    cout << sizeof(r) << endl;
    memcpy(&r.brand, (char *)&"14pm", 4);
    cout << "major brand is " << r.brand << endl;
    cout << sizeof(r) << endl;
    vector<uint32_t> brands;
    brands.reserve(2);
    memcpy(&brands[0], (char *)&"mosi", 4);
    memcpy(&brands[1], (char *)&"xxxx", 4);
    memcpy(&r.compatible_brands, &brands[0], 8);
    cout << "brand one is " << r.compatible_brands[0] << endl;
    cout << "brand two is " << r.compatible_brands[1] << endl;
    cout << sizeof(r) << endl;
    r.size = sizeof(r);
    cout << sizeof(r) << endl;


    uint32_t *rptr = reinterpret_cast<uint32_t *>(&r);
   
    size_t number = sizeof(r) / sizeof(uint32_t);
    cout << "number is " << number << endl;
    for (int i = 0; i < number; i++)
    {
        cout << "step " << i << "is " << *(rptr + i) << endl;

        uint32_t bigend = __bswap_32(*(rptr + i));
        // make bigend into bytes
        char *bytes = reinterpret_cast<char *>(&bigend);
        // should stil be 4 bytes
        for (int j = 0; j < sizeof(bigend); j++)
        {
            tt << *(bytes + j);
        }
    }
    ofstream f{};
    f.open("test.mp4", ios::binary);
    f.write(tt.str().data(), sizeof(r));
   
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

When I teach C++, the mantra I repeat over and over is: When you get the types right, everything Just Works. What’s going on here is a good example of how code becomes messy and terrible and inefficient when you don’t get the types right.

C++ is a strongly-typed language. It’s probably the most strongly-typed language you’re ever going to use. If you’re not going to use the type system, you’re going to be swimming upstream the whole time, fighting the language. All those ugly casts are a symptom of that.

Here’s a pretty glaring example: the fact that you have to manually reverse those strings because you’re forcing them into 32-bit integers. If you think about what you’re trying to do, it’s just silly. They’re not numbers; they’re strings. Jamming them into an integer just because they technically “fit” is ridiculous. This just makes no sense: uint32_t s = "ftyp";.

The correct kind of type you should be using for this is something like std::array<char, 4>. (You could use std::string, but that’s overkill when you know exactly how large the string will always be, and it’s so tiny.) If you’d done that, writing it out would be as simple as: f.write(boxtype.data(), boxtype.size()).

But even a bare array like that isn’t great. As usual, you need to get the type right. Here’s one example of how:

class box_type_t
{
public:
    constexpr explicit box_type_t(std::string_view sv)
        : _data{'\0', '\0', '\0', '\0'}
    {
        if (sv.size() > 4)
            throw std::invalid_argument{"string is too long for box type"};

        std::ranges::copy(sv.substr(0, 4), _data.begin());
    }

    explicit operator std::string() const
    {
        return std::string{_data.data(), _data.size()};
    }

    constexpr explicit operator std::string_view() const noexcept
    {
        return std::string_view{_data.data(), _data.size()};
    }

    constexpr auto operator<=>(box_type_t const&) const noexcept = default;

    friend constexpr auto operator<=>(box_type_t const& lhs, std::string_view rhs) noexcept
    {
        return std::string_view{lhs} <=> rhs;
    }

    template <typename Char, typename Traits>
    friend auto operator<<(std::basic_ostream<Char, Traits>& o, box_type_t const& bt)
        -> std::basic_ostream<Char, Traits>&
    {
        // Only need this temporary buffer so it's NUL terminated.
        //
        // Alternately, you could make _data a *5* char array, and NUL
        // terminate it there. Or you could widen each character and put them
        // in the stream one at a time (but that's problematic).
        auto buf = std::array<char, 5>{'\0', '\0', '\0', '\0', '\0'};
        std::ranges::copy_n(bt._data.data(), bt._data.size(), buf.data());

        return o << buf.data();
    }

private:
    std::array<char, 4> _data;
};

Now usage of this type is simple, clean, elegant, and idiot-proof:

// constructing a box type
auto bt = box_type_t{"ftyp"};

// checking a box type
bt == "ftyp";
//or:
auto bt2 = box_type_t{"ftyp"};
bt == bt2;

// printing a box type
// (note, doesn't reverse the string!)
std::cout << bt;

// using box types with other libraries
auto sv = std::string_view{bt}; // convert to a string view
auto s = std::string{bt};       // or convert to a string

Once again: When you get the types right, everything Just Works.

(One thing I haven’t accounted for anywhere is if the system isn’t using UTF-8 or ASCII for char. In that case 'a' may not be 0x61, so "ftyp" may not be translating to the correct sequence of numeric values. What you should do is always use u8"ftyp", and have the box_type_t constructor insist on only accepting std::basic_string_view<char8_t> (even if it holds chars internally). But that’s a complication that adds a lot of noise, so I’ve just ignored it.)

Now, your plan is to write to MP4 files, which has its own standards and quirks, so you don’t want to be using the standard functions. Whenever the standard functions do the job, then it’s fine to defer to them, but you want control.

So what you should probably do is create a namespace for all your MP4 stuff (that’s something you should do anyway), and create a set of file writing functions:

namespace mp4 {

namespace _detail {

// This tag will be handy to make write functions for classes in the mp4 namespace.
struct mp4_write_tag {};

// To write 32-bit ints:
auto write(std::ostream&, std::uint_fast32_t) -> std::ostream&;
auto write(std::ostream&, std::uint_least32_t) -> std::ostream&;

// Note: uint32_t is OPTIONAL, so you can't assume it exists. You need to
// check if it's supported.
#ifdef UINT32_MAX
    auto write(std::ostream&, std::uint32_t) -> std::ostream&;
#endif

// Any other functions you want....
//
// For example, if you want to be able to write strings:
//  auto write(std::ostream&, std::string_view) -> std::ostream&;
//
// Or, if you want to write random byte data:
//  template <std::size_t Extent>
//  auto write(std::ostream&, std::span<std::byte, Extent>) -> std::ostream&;
//  template <std::size_t Extent>
//  auto write(std::ostream&, std::span<unsigned char, Extent>) -> std::ostream&;

// This function transforms non-member calls from
//      write(o, t);
// to:
//      t.write(mp4::_detail::mp4_write_tag{}, o);
// So classes in the mp4 namespace can have non-member write() functions
// without conflicting with the write() niebloid.
template <typename T>
auto write(std::ostream& o, T&& t) -> decltype(std::forward<T>(t).write(mp4_write_tag{}, o))
{
    return std::forward<T>(t).write(mp4_write_tag{}, o);
}

// This struct, and the inline variable below, are to make write() a
// customization point object, using a niebloid.
//
// See:
//  *   https://ericniebler.com/2014/10/21/customization-point-design-in-c11-and-beyond/
//  *   https://brevzin.github.io/c++/2020/12/19/cpo-niebloid/
struct write_func
{
    template <typename T>
    constexpr auto write(std::ostream& o, T&& t) const
        -> decltype(write(o, std::forward<T>(t)))
        requires std::same_as<decltype(write(o, std::forward<T>(t))), std::ostream&>
    {
        write(o, std::forward<T>(t));
        return o;
    }
};

} // namespace _detail

inline constexpr auto write = _detail::write_func{};

} // namespace mp4

Now you can add a write() function to the box_type_t class above like so:

// In the mp4 namespace, of course.
namespace mp4 {


class box_type_t
{
    // ... [everything else as above] ...

public:
    auto write(_detail::mp4_write_tag, std::ostream& o) const -> std::ostream&
    {
        // Same implementation as the normal stream inserter, but doesn't
        // have to be. (For example, the normal stream inserter could handle
        // conversions to non-UTF8/ASCII char types.)
        auto buf = std::array<char, 5>{'\0', '\0', '\0', '\0', '\0'};
        std::ranges::copy_n(bt._data.data(), bt._data.size(), buf.data());

        return o << buf.data();
    }
};

} // namepsace mp4

And now, writing a chunk like the one in your code would be as simple as this:

// The raw chunk data:
auto size = std::uint_fast32_t{24}; // yes, this is a magic number, and thus bad, but bear with me
auto type = mp4::box_type_t{"ftyp"};
auto brand = mp4::brand_t{"mp41"};  // brand_t is similar to box_type_t
auto minor = std::uint_fast32_t{0};
auto compatible_brands = std::array{
    mp4::brand_t{"isom"},
    mp4::brand_t{"xxxx"}
};

// Writing the data:
mp4::write(f, size);
mp4::write(f, type);
mp4::write(f, brand);
mp4::write(f, minor);
for (auto&& b : compatible_brands)
    mp4::write(f, b);

And of course, if you made a proper type for the chunk:

// Initializing the chunk (using a proper constructor).
//
// Only non-default data needs to be provided
auto ftbox = ftbox_t{
    mp4::box_type_t{"mp41"},
    std::array{
        mp4::box_type_t{"isom"},
        mp4::box_type_t{"xxxx"}
    };
}

// Writing the data:
mp4::write(f, ftbox);

Safe, clean, elegant, and idiot-proof. That’s the payoff from getting the types right.

In fact, let's go a step further, and presume we have an abstract base class box_t, from which ftbox_t is derived:

namespace mp4 {

class box_t
{
public:
    virtual ~box_t() = default;

    // Assume no extra data in the chunk by default. You decide whether that's
    // wise, and if not, just make the function pure virtual.
    virtual auto payload_size() const noexcept -> std::uint_fast32_t { return 0; }

    virtual auto type() const noexcept -> box_type_t = 0;

    // This function and _do_write() use the non-virtual interface pattern.
    //
    // See: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Non-Virtual_Interface
    auto write(_detail::mp4_write_tag, std::ostream& o) const -> std::ostream&
    {
        // The chunk size:
        mp4::write(o, std::uint_fast32_t(payload_size() + 8));

        // The chunk type:
        mp4::write(o, type());

        // The chunk's data:
        do_write(o);

        return o;

        // Note that there is another, safer, though less efficient way to
        // do this.
        //
        // Simply don't have a payload_size() function, and instead do:
        //
        //      // Write the chunk data into a buffer.
        //      auto buffer = std::ostringstream{};
        //      do_write(buffer);
        //
        //      // Get the size of that data written.
        //      auto const payload_size = buffer.view().size();
        //
        //      // Now write the actual chunk size:
        //      mp4::write(o, std::uint_fast32_t(payload_size + 8));
        //
        //      // The chunk type:
        //      mp4::write(o, type());
        //
        //      // The chunk's data:
        //      o << buffer.rdbuf();
        //
        //      return o;
        //
        // The extra cost is that additional buffer. But it’s guaranteed to
        // have the correct size, so... you have to weigh the trade-offs.
    }

private:
    virtual auto do_write(std::ostream&) const -> std::ostream& = 0;
};

class ftbox_t : public box_t
{
public:
    // Obviously make some sensible constructors....

    auto payload_size() const noexcept -> std::uint_fast32_t override
    {
        return std::uint_fast32_t(
            4   // size of brand
            + 4 // size of minor
            + (_compatible_brands.size() * 4)
        );

        // Note that I've hardcoded the size of each thing. A better solution
        // might be to have a mp4::write_size() function that returns the
        // correct written size for whatever. So this function would be:
        //
        //      return mp4::write_size(_brand)
        //          + mp4::write_size(_minor)
        //          + mp4::write_size(_compatible_brands)
        //      ;
    }

    auto type() const noexcept -> box_type_t override { return box_type_t{"ftyp"}; }

private:
    auto do_write(std::ostream& o) const -> std::ostream& override
    {
        mp4::write(o, _brand);
        mp4::write(o, _minor);
        for (auto&& b : _compatible_brands)
            mp4::write(o, b);
    }

    brand_t _brand;
    minor_t _minor;
    std::array<brand_t, 2> _compatible_brands;

    // I just realized I've been assuming there can always only be 2
    // compatible brands. If the number of compatible brands can vary, then
    // instead of using an array, you could use a vector.
};

} // namespace mp4

If you make derived classes for all chunk types, then creating and writing an entire MP4 file would be as simple as:

auto chunks = std::vector<std::unique_ptr<mp4::box_t>>{};

// Make as many chunks as you like. For example:
chunks.push_back(std::make_unique<mp4::ftbox_t>(
    mp4::brand_t{"mp41"},
    std::array{
        mp4::brand_t{"isom"},
        mp4::brand_t{"xxxx"}
    }
));

// Now write all the chunks:
for (auto&& p_chunk : chunks)
    mp4::write(f, *p_chunk);

// Or make a function that writes a whole MP4 in one shot:
//      mp4::write_file(f, chunks);

You might be concerned about the efficiency of all the above. That’s usually the concern that drives people to writing all the type-punning shenanigans involving casts to raw memory and such.

Well, here’s the punchline that might blow your mind: Your code will be significantly slower than the code I write above.

Why? Because all the work you do to create a buffer—that string stream—and then write all the items into that buffer so you ultimately byte-blast it into the final output stream… all of that is just wasted effort. Streams are already buffered. (That includes the C streams, which most people don’t know.) So all the manual work you do to create that buffer, and then blast the buffer contents into the stream… that’s already happening. You’re just creating an unnecessary, extra buffer. Instead of going “data → buffer → stream”, you’re going “data → buffer 1 → buffer 2 → stream”.

All you need to do to get maximally efficient output is get the data into the stream as efficiently as possible. After that, the library will handle buffering it, and then blasting it into the file as quickly as possible, probably in multi-kilobyte chunks at a time.

The trick, then becomes getting the data into the stream as efficiently as possible and that… that’s a complex topic that could take entire books to really cover. But one trick will you get a long way there: Avoid formatted output functions… especially those that use locales. Basically, write() is your friend… never use operator<< (well, it’s okay for strings, because they're unformatted output… it’s also fine for copying from one stream to another via the stream buffer).

For example, consider writing 32-bit integers. What you could do is this:

namespace mp4 {

namespace _detail {

auto write(std::ostream& o, std::uint_fast32_t i) -> std::ostream&
{
    auto const buf = std::array<char, 4>{
        static_cast<char>((i >> 24) & 0xFFu),
        static_cast<char>((i >> 16) & 0xFFu),
        static_cast<char>((i >> 8) & 0xFFu),
        static_cast<char>(i & 0xFFu)
    };

    return o.write(buf.data(), buf.size());
}

} // namespace _detail

} // namepsace mp4

You can’t get any faster than that.

Wait, you may be thinking… creating an array buffer, then manually copying the integer byte-by-byte while doing all those arithmetic operations… surely that can’t be as efficient as a __bswap_32 compiler intrinsic?!

Wanna bet?

There are two functions in that code, blast() and write(). blast() just reinterpret_casts the number to bytes, and then copies those to the stream. As you might expect, this causes issues with endianness (see the output on the bottom right). It’s as ultimately fast as you can hope for… aside from giving the wrong result, but slipping a __bswap_32 in there would fix that, right?

Take a look at the assembly generated for the two functions over on the upper right. blast() is lines 2–8. write() is lines 10–17. Notice that the assembly generated for the two functions is identical

… except write() does the bswap correctly (on line 11)!

That’s GCC on O2. Clang does the same optimization on O1.

There’s a moral here, and it is this: When programming in C++, trust the compiler. Don’t try to outsmart it. Don’t try to work around it. It is smarter than you are. If you try to fight it, or do things to outsmart it, it is you who will end up looking silly in the end.

In this case, it is smart enough to recognize that what we’re doing with that temporary array is a 32-bit bswap. So… it just does it.

And this is perfectly portable code (well, I mean, once you static assert that char is 8 bits), that will work on any system… regardless of endianness—even on bizarre “PDP-endian” systems—and regardless of the size of uint_fast32_t (for the record, it’s actually 64 bits on the system Compiler Explorer is using… if you tried to cast a 64-bit number to raw bytes then do a 32-bit bswap on a big-endian machine… well, you’d get grief).

The power of C++ is at compile time. It’s in the type system, and it’s in the stupidly-powerful compiler technologies that optimize the bejeezus out of your code. Leverage that power; don’t “work around” it, and don’t fight it. Write what you mean—the reason Stroustrup invented C++ in the first place was explicitly to meaningfully model stuff, and translate those models to efficient machine code. Focus on correctly modelling your problem, and let the compiler sort out transforming what you mean to the most efficient machine code possible.

Remember these mantras:

  • The power of C++ is at compile time.
  • When you get the types right, everything Just Works.
  • Write what you mean; let the compiler handle translating to efficient machine code.
  • Trust the compiler… but always check if it matters.*

And to summarize the review:

  • Make proper types for the elements of your problem.

    For example, you have a “ftbox”—which is a specific type of “box”—that has a “brand”, a “minor” (whatever that means), and a list of compatible “brands”. That’s a list of classes to make, right there: you need a class ftbox, which is derived from box, that has data members with type brand, minor, and an array/vector of brand. Each of those types has to support being written to files/streams, so you probably need a generic/overloaded write() function. And so on….

    In every case, is you write those types well, they will be maximally ergonomic and efficient, and using them and combining them will be effortless. When you get the types right, everything Just Works.

  • Don’t write shitty code because you think it’s efficient.

    For starters, it often isn’t. It happens regularly that I see C++ beginners twist themselves in knots writing almost illegible, low-level, C-like code… that is slower—sometimes by orders of magnitude—than a single line of high-level C++.

    But even in the “best” case, where that low-level, C-like code is extremely efficient, it is almost always true that high-level C++ code can match its performance. Or, if not, the C++ code is only slower by an amount that doesn’t actually matter in practice. (In this case, an example would be bending over backwards to make reversing the endianness of a number, even though, in the slowest case, that cost will be absolutely dwarfed by the cost of actually writing the bytes to a file. So if your byte-swapping is slow, no-one will ever notice.)

    Here, for example, there are not only no benefits to all the gymnastics to efficiently write byte sequences to a buffer, then writing that buffer to a stream… because the stream is already buffered. You could avoid the extra costs by doing the simple and obvious thing: just write the byte sequences directly to the stream.

    Good, clean, well-written, high-level C++ code will be just as fast as even the most well-done juggling of bytes in raw memory… and it will be much easier to read, understand, and maintain.

\$\endgroup\$
8
  • \$\begingroup\$ 1. Any reason not to use the std::string_view conversion in operator<< in the first code block: return o << std::string_view{ bt };? (and later in write). 2. For some reason I have it in my head that it's unsafe to write stream operators that return in one line like return os << foo;, and the return should always be a separate statement... but I'm no longer sure why, or where I got this idea from? Was there ever any reason not to do that? \$\endgroup\$
    – user673679
    Apr 7, 2022 at 10:42
  • 1
    \$\begingroup\$ 1. Yes (in operator<<, but no in write()). The operator<< is templated on Char and Traits, so it will work for ostream, wostream, basic_ostream<char8_t>, etc.. basic_ostream<Char, Traits> has an overload for basic_string_view<Char, Traits>, so wostream can take a wstring_view… but not string_view. The trick is that basic_ostream<Char, Traits> has overloads for Char const* and char const*, so wostream can take wchar_t const*and char const*. … \$\endgroup\$
    – indi
    Apr 7, 2022 at 15:37
  • 1
    \$\begingroup\$ … The char const* overload automatically widens the string (when necessary; it doesn’t when Char is char), so you don’t have to. It’s a neat trick that makes it easy to write a stream inserter that supports all Char and Traits variants. That’s what makes those shenanigans to create a NUL-terminated char string worth it. (However, there’s no point in the case of write(), because that’s just using ostream, not bothering about different Char or Traits types. You could use the string_view conversion there just fine. Honestly, I just copy-pasted. 😛) \$\endgroup\$
    – indi
    Apr 7, 2022 at 15:37
  • 1
    \$\begingroup\$ 2. I’ve swung back and forth on this. The issue is that someone could write operator<< for their type to return a different stream. So if you do return o << x; you can’t be sure the stream you’re returning is actually o. On one hand, you really can’t be held responsible if someone writes an inserter that does something that Machiavellian. On the other, simply doing o << x; return o; avoids the problem altogether… though it arguably prevents possibly legit stream substitution tricks (but I can’t think of any). … \$\endgroup\$
    – indi
    Apr 7, 2022 at 15:38
  • 1
    \$\begingroup\$ … I mean, there are also other arguments, like that return o << x; is a tail call that can be transformed into a jump, but… meh. And there’s long, deep rabbit hole about how stuff like cout << a; cout << b; is not the same as cout << a << b; (because in the former, both a and b will know they’re being streamed into a cout stream specifically, while in the latter, b will only know it’s going into a generic ostream)… but even more meh. The key concern is defending against someone returning a different stream. \$\endgroup\$
    – indi
    Apr 7, 2022 at 15:38

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.