Skip to content

Commit

Permalink
WavWriter::WriteSamples: Return status code, instead of a bool.
Browse files Browse the repository at this point in the history
  - Also, this brings in `[[no_discard]]`, so fix some call sites that were ignoring the return value.
  - Drive-by: Use some named `constexpr` when checking the `int` library return codes.

PiperOrigin-RevId: 714119141
  • Loading branch information
jwcullen committed Jan 13, 2025
1 parent d4fcda0 commit 9a9b0a4
Show file tree
Hide file tree
Showing 8 changed files with 53 additions and 32 deletions.
2 changes: 2 additions & 0 deletions iamf/cli/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,8 @@ cc_library(
"@com_google_absl//absl/log",
"@com_google_absl//absl/log:check",
"@com_google_absl//absl/memory",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
"@com_google_audio_to_tactile//:dsp",
],
)
Expand Down
3 changes: 1 addition & 2 deletions iamf/cli/adm_to_user_metadata/adm/panner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include <cstdint>
#include <cstdio>
#include <cstring>
#include <filesystem>
#include <numbers>
#include <string>
#include <vector>
Expand Down Expand Up @@ -166,7 +165,7 @@ absl::Status PanObjectsToAmbisonics(const std::string& input_filename,
op_buffer_int32[i], ip_wav_bits_per_sample,
/*big_endian=*/false, output_buffer_char.data(), write_position));
}
wav_writer.WriteSamples(output_buffer_char);
RETURN_IF_NOT_OK(wav_writer.WriteSamples(output_buffer_char));

samples_remaining -= samples_read;
}
Expand Down
5 changes: 1 addition & 4 deletions iamf/cli/adm_to_user_metadata/adm/wav_file_splicer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,7 @@ void AbortAllWavWriters(

absl::Status FlushToWavWriter(std::vector<uint8_t>& samples_to_flush,
WavWriter& wav_writer) {
if (!wav_writer.WriteSamples(samples_to_flush)) {
return absl::AbortedError(
"Error while writing the samples from the buffer.");
}
RETURN_IF_NOT_OK(wav_writer.WriteSamples(samples_to_flush));
samples_to_flush.clear();
return absl::OkStatus();
}
Expand Down
2 changes: 1 addition & 1 deletion iamf/cli/rendering_mix_presentation_finalizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ absl::Status WriteRenderedSamples(const std::vector<int32_t>& rendered_samples,
/*big_endian=*/false, native_samples.data(),
write_position));
}
wav_writer.WriteSamples(native_samples);
RETURN_IF_NOT_OK(wav_writer.WriteSamples(native_samples));

return absl::OkStatus();
}
Expand Down
1 change: 1 addition & 0 deletions iamf/cli/tests/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,7 @@ cc_test(
":cli_test_utils",
"//iamf/cli:wav_reader",
"//iamf/cli:wav_writer",
"@com_google_absl//absl/status:status_matchers",
"@com_google_googletest//:gtest_main",
],
)
24 changes: 14 additions & 10 deletions iamf/cli/tests/wav_writer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@
#include <utility>
#include <vector>

#include "absl/status/status_matchers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include "iamf/cli/tests/cli_test_utils.h"
#include "iamf/cli/wav_reader.h"

using ::absl_testing::IsOk;

namespace iamf_tools {
namespace {

Expand Down Expand Up @@ -75,7 +79,7 @@ TEST(WavWriterTest, WriteEmptySamplesSucceeds) {
ASSERT_NE(wav_writer, nullptr);

std::vector<uint8_t> empty_samples(0);
EXPECT_TRUE(wav_writer->WriteSamples(empty_samples));
EXPECT_THAT(wav_writer->WriteSamples(empty_samples), IsOk());
}

TEST(WavWriterTest, WriteIntegerSamplesSucceeds) {
Expand All @@ -85,7 +89,7 @@ TEST(WavWriterTest, WriteIntegerSamplesSucceeds) {

// Bit depth = 16, and writing 6 bytes = 48 bits = 3 samples succeeds.
std::vector<uint8_t> samples(6, 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

TEST(WavWriterTest, WriteNonIntegerNumberOfSamplesFails) {
Expand All @@ -95,7 +99,7 @@ TEST(WavWriterTest, WriteNonIntegerNumberOfSamplesFails) {

// Bit depth = 16, and writing 3 bytes = 24 bits = 1.5 samples fails.
std::vector<uint8_t> samples(3, 0);
EXPECT_FALSE(wav_writer->WriteSamples(samples));
EXPECT_FALSE(wav_writer->WriteSamples(samples).ok());
}

TEST(WavWriterTest, WriteIntegerSamplesSucceedsWithoutHeader) {
Expand All @@ -106,7 +110,7 @@ TEST(WavWriterTest, WriteIntegerSamplesSucceedsWithoutHeader) {

// Bit depth = 16, and writing 6 bytes = 48 bits = 3 samples succeeds.
std::vector<uint8_t> samples(6, 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

TEST(WavWriterTest, Write24BitSamplesSucceeds) {
Expand All @@ -116,7 +120,7 @@ TEST(WavWriterTest, Write24BitSamplesSucceeds) {

// Bit depth = 24, and writing 6 bytes = 48 bits = 2 samples succeeds.
std::vector<uint8_t> samples(6, 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

TEST(WavWriterTest, Write32BitSamplesSucceeds) {
Expand All @@ -126,7 +130,7 @@ TEST(WavWriterTest, Write32BitSamplesSucceeds) {

// Bit depth = 32, and writing 8 bytes = 64 bits = 2 samples succeeds.
std::vector<uint8_t> samples = {1, 0, 0, 0, 2, 0, 0, 0};
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

TEST(WavWriterTest, FileExistsAndHasNonZeroSizeWithHeader) {
Expand Down Expand Up @@ -176,7 +180,7 @@ TEST(WavWriterTest, OutputFileHasCorrectSizeWithoutHeader) {
kSampleRateHz, kBitDepth16, /*write_header=*/false);
ASSERT_NE(wav_writer, nullptr);
std::vector<uint8_t> samples(kInputBytes, 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

std::error_code error_code;
Expand All @@ -200,7 +204,7 @@ TEST(WavWriterTest, Output16BitWavFileHasCorrectData) {
ASSERT_NE(wav_writer, nullptr);
std::vector<uint8_t> samples(kInputBytes, 0);
std::iota(samples.begin(), samples.end(), 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

auto wav_reader =
Expand All @@ -224,7 +228,7 @@ TEST(WavWriterTest, Output24BitWavFileHasCorrectData) {
ASSERT_NE(wav_writer, nullptr);
std::vector<uint8_t> samples(kInputBytes, 0);
std::iota(samples.begin(), samples.end(), 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

auto wav_reader =
Expand All @@ -248,7 +252,7 @@ TEST(WavWriterTest, Output32BitWavFileHasCorrectData) {
ASSERT_NE(wav_writer, nullptr);
std::vector<uint8_t> samples(kInputBytes, 0);
std::iota(samples.begin(), samples.end(), 0);
EXPECT_TRUE(wav_writer->WriteSamples(samples));
EXPECT_THAT(wav_writer->WriteSamples(samples), IsOk());
}

auto wav_reader =
Expand Down
43 changes: 30 additions & 13 deletions iamf/cli/wav_writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,18 @@
#include "absl/log/check.h"
#include "absl/log/log.h"
#include "absl/memory/memory.h"
#include "absl/status/status.h"
#include "absl/strings/str_cat.h"
#include "src/dsp/write_wav_file.h"

namespace iamf_tools {

namespace {
// Some audio to tactile functions return 0 on success and 1 on failure.
constexpr int kAudioToTactileResultFailure = 0;
constexpr int kAudioToTactileResultSuccess = 1;
} // namespace

std::unique_ptr<WavWriter> WavWriter::Create(const std::string& wav_filename,
int num_channels,
int sample_rate_hz, int bit_depth,
Expand Down Expand Up @@ -64,7 +72,8 @@ std::unique_ptr<WavWriter> WavWriter::Create(const std::string& wav_filename,
// header.
if (!write_header) {
wav_header_writer = WavHeaderWriter();
} else if (wav_header_writer(file, 0, sample_rate_hz, num_channels) == 0) {
} else if (wav_header_writer(file, 0, sample_rate_hz, num_channels) ==
kAudioToTactileResultFailure) {
LOG(ERROR) << "Error writing header of file \"" << wav_filename << "\"";
return nullptr;
}
Expand All @@ -90,28 +99,30 @@ WavWriter::~WavWriter() {
}

// Write samples for all channels.
bool WavWriter::WriteSamples(const std::vector<uint8_t>& buffer) {
absl::Status WavWriter::WriteSamples(const std::vector<uint8_t>& buffer) {
if (file_ == nullptr) {
return false;
// Wav writer may have been aborted.
return absl::FailedPreconditionError(
"Wav writer is not accepting samples.");
}

const auto buffer_size = buffer.size();

if (buffer_size == 0) {
// Nothing to write.
return true;
return absl::OkStatus();
}

if (buffer_size % (bit_depth_ * num_channels_ / 8) != 0) {
LOG(ERROR) << "Must write an integer number of samples.";
return false;
return absl::InvalidArgumentError(
"Must write an integer number of samples.");
}

// Calculate how many samples there are.
const int bytes_per_sample = bit_depth_ / 8;
const size_t num_total_samples = (buffer_size) / bytes_per_sample;

int result = 0;
int write_sample_result = kAudioToTactileResultFailure;
if (bit_depth_ == 16) {
// Arrange the input samples into an int16_t to match the expected input of
// `WriteWavSamples`.
Expand All @@ -121,7 +132,8 @@ bool WavWriter::WriteSamples(const std::vector<uint8_t>& buffer) {
samples[i / bytes_per_sample] = (buffer[i + 1] << 8) | buffer[i];
}

result = WriteWavSamples(file_, samples.data(), samples.size());
write_sample_result =
WriteWavSamples(file_, samples.data(), samples.size());
} else if (bit_depth_ == 24) {
// Arrange the input samples into an int32_t to match the expected input of
// `WriteWavSamples24Bit` with the lowest byte unused.
Expand All @@ -131,7 +143,8 @@ bool WavWriter::WriteSamples(const std::vector<uint8_t>& buffer) {
samples[i / bytes_per_sample] =
(buffer[i + 2] << 24) | buffer[i + 1] << 16 | buffer[i] << 8;
}
result = WriteWavSamples24Bit(file_, samples.data(), samples.size());
write_sample_result =
WriteWavSamples24Bit(file_, samples.data(), samples.size());
} else if (bit_depth_ == 32) {
// Arrange the input samples into an int32_t to match the expected input of
// `WriteWavSamples32Bit`.
Expand All @@ -142,20 +155,24 @@ bool WavWriter::WriteSamples(const std::vector<uint8_t>& buffer) {
buffer[i + 2] << 16 | buffer[i + 1] << 8 |
buffer[i];
}
result = WriteWavSamples32Bit(file_, samples.data(), samples.size());
write_sample_result =
WriteWavSamples32Bit(file_, samples.data(), samples.size());
} else {
// This should never happen because the factory method would never create
// an object with disallowed `bit_depth_` values.
LOG(FATAL) << "WavWriter only supports 16, 24, and 32-bit samples; got "
<< bit_depth_;
}

if (result == 1) {
if (write_sample_result == kAudioToTactileResultSuccess) {
total_samples_written_ += num_total_samples;
return true;
return absl::OkStatus();
}

return false;
// It's not clear why this would happen.
return absl::UnknownError(
absl::StrCat("Error writing samples to wav file. write_sample_result= ",
write_sample_result));
}

void WavWriter::Abort() {
Expand Down
5 changes: 3 additions & 2 deletions iamf/cli/wav_writer.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <vector>

#include "absl/functional/any_invocable.h"
#include "absl/status/status.h"

namespace iamf_tools {

Expand Down Expand Up @@ -57,9 +58,9 @@ class WavWriter {
*
* \param buffer Buffer of raw input PCM with channels interlaced and no
* padding.
* \return `true` on success. `false` on failure.
* \return `absl::OkStatus()` on success. A specific status on failure.
*/
bool WriteSamples(const std::vector<uint8_t>& buffer);
absl::Status WriteSamples(const std::vector<uint8_t>& buffer);

/*!\brief Aborts the write process and deletes the wav file.*/
void Abort();
Expand Down

0 comments on commit 9a9b0a4

Please sign in to comment.