Skip to content

Commit

Permalink
WavReader: Add a custom move constructor.
Browse files Browse the repository at this point in the history
  - The compiler-provided default move constructor is buggy because the "moved-from" class has the file closed in the destructor.
  - Not used in codebase right now, but better to implement it properly or delete it.
  - Similar to `WavWriter`.

PiperOrigin-RevId: 641883579
  • Loading branch information
jwcullen committed Jun 10, 2024
1 parent 156a944 commit 64c37a2
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 0 deletions.
34 changes: 34 additions & 0 deletions iamf/cli/tests/wav_reader_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@

#include <cstddef>
#include <cstdint>
#include <cstring>
#include <filesystem>
#include <string>
#include <utility>
#include <vector>

// Placeholder for get runfiles header.
Expand Down Expand Up @@ -168,5 +170,37 @@ TEST(WavReader, OneFrame32BitLittleEndian) {
EXPECT_EQ(wav_reader.buffers_, expected_frame);
}

TEST(WavReader, IsSafeToCallReadFrameAfterMove) {
const size_t kNumSamplesPerFrame = 1;
auto wav_reader =
InitAndValidate("stereo_8_samples_48khz_s16le.wav", kNumSamplesPerFrame);
auto wav_reader_moved = std::move(wav_reader);

EXPECT_EQ(wav_reader_moved.ReadFrame(), 2);
const std::vector<std::vector<int32_t>> kExpectedFrame = {
{0x00010000, static_cast<int32_t>(0xffff0000)}};
EXPECT_EQ(wav_reader_moved.buffers_, kExpectedFrame);
}

template <typename T>
std::vector<uint8_t> GetRawBytes(const T& object) {
std::vector<uint8_t> raw_object(sizeof(object));
std::memcpy(raw_object.data(), static_cast<const void*>(&object),
raw_object.size());
return raw_object;
}

TEST(WavReader, IsByteEquivalentAfterMoving) {
const size_t kNumSamplesPerFrame = 1;
auto wav_reader =
InitAndValidate("stereo_8_samples_48khz_s16le.wav", kNumSamplesPerFrame);
const auto raw_wav_reader_before_move = GetRawBytes(wav_reader);

const auto wav_reader_moved = std::move(wav_reader);
const auto raw_wav_reader_after_move = GetRawBytes(wav_reader_moved);

EXPECT_EQ(raw_wav_reader_before_move, raw_wav_reader_after_move);
}

} // namespace
} // namespace iamf_tools
11 changes: 11 additions & 0 deletions iamf/cli/wav_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <cstddef>
#include <cstdio>
#include <string>
#include <utility>
#include <vector>

#include "absl/log/check.h"
Expand Down Expand Up @@ -54,6 +55,16 @@ WavReader::WavReader(const std::string& wav_filename,
}
}

WavReader::WavReader(WavReader&& original)
: buffers_(std::move(original.buffers_)),
num_samples_per_frame_(original.num_samples_per_frame_),
file_(original.file_),
info_(original.info_) {
// Invalidate the file pointer on the original copy to prevent it from being
// closed on destruction.
original.file_ = nullptr;
}

WavReader::~WavReader() {
if (file_ != nullptr) {
std::fclose(file_);
Expand Down
3 changes: 3 additions & 0 deletions iamf/cli/wav_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class WavReader {
*/
WavReader(const std::string& wav_filename, size_t num_samples_per_frame);

/*!\brief Moves the `WavReader` without closing the underlying file.*/
WavReader(WavReader&& original);

/*!\brief Destructor. */
~WavReader();

Expand Down

0 comments on commit 64c37a2

Please sign in to comment.