Skip to content

Commit

Permalink
Fix a bug where CreateFromBuffer would result in all codecs being t…
Browse files Browse the repository at this point in the history 
…reated as lossy.

  - No downstream effect because nothing was using this field on OBUs created via `CreateFromBuffer`.
  - Also, update CreateFromBuffer tests to more check requirements.

PiperOrigin-RevId: 654012954
  • Loading branch information
@jwcullen
jwcullen committed Jul 19, 2024
1 parent 52eb5ee commit df2ff29
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 45 deletions.
2 changes: 1 addition & 1 deletion iamf/cli/proto_to_obu/audio_element_generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ absl::Status ValidateReconGainDefined(
// First layer: there is no demixed channel, so recon gain is not
// required.
expected_recon_gain_is_present_flag = 0;
} else if (codec_config_obu.is_lossless_) {
} else if (codec_config_obu.IsLossless()) {
// Lossless codec does not require recon gain.
expected_recon_gain_is_present_flag = 0;
} else {
Expand Down
12 changes: 6 additions & 6 deletions iamf/obu/codec_config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ absl::Status ValidateNumSamplesPerFrame(uint32_t num_samples_per_frame) {
return absl::OkStatus();
}

bool IsLossless(CodecConfig::CodecId codec_id) {
return codec_id == CodecConfig::kCodecIdFlac ||
codec_id == CodecConfig::kCodecIdLpcm;
}

absl::Status SetSampleRatesAndBitDepths(
uint32_t codec_id, const DecoderConfig& decoder_config,
uint32_t& output_sample_rate, uint32_t& input_sample_rate,
Expand Down Expand Up @@ -101,7 +96,6 @@ CodecConfigObu::CodecConfigObu(const ObuHeader& header,
const DecodedUleb128 codec_config_id,
const CodecConfig& codec_config)
: ObuBase(header, kObuIaCodecConfig),
is_lossless_(IsLossless(codec_config.codec_id)),
codec_config_id_(codec_config_id),
codec_config_(std::move(codec_config)) {}

Expand Down Expand Up @@ -249,4 +243,10 @@ absl::Status CodecConfigObu::Initialize() {
return init_status_;
}

bool CodecConfigObu::IsLossless() const {
using enum CodecConfig::CodecId;
return codec_config_.codec_id == kCodecIdFlac ||
codec_config_.codec_id == kCodecIdLpcm;
}

} // namespace iamf_tools
4 changes: 1 addition & 3 deletions iamf/obu/codec_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,12 @@ class CodecConfigObu : public ObuBase {
*/
const CodecConfig& GetCodecConfig() const { return codec_config_; }

// Metadata fields.
const bool is_lossless_;
bool IsLossless() const;

private:
// Used only by the factory create function.
explicit CodecConfigObu(const ObuHeader& header)
: ObuBase(header, kObuIaCodecConfig),
is_lossless_(false),
codec_config_id_(DecodedUleb128()),
codec_config_(CodecConfig()) {}

Expand Down
114 changes: 79 additions & 35 deletions iamf/obu/tests/codec_config_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <cstdint>
#include <limits>
#include <memory>
#include <variant>
#include <vector>

#include "absl/status/status.h"
Expand All @@ -35,13 +36,15 @@ namespace {

using ::absl_testing::IsOk;

constexpr DecodedUleb128 kCodecConfigId = 123;

class CodecConfigTestBase : public ObuTestBase {
public:
CodecConfigTestBase(CodecConfig::CodecId codec_id,
DecoderConfig decoder_config)
: ObuTestBase(
/*expected_header=*/{0, 14}, /*expected_payload=*/{}),
codec_config_id_(0),
codec_config_id_(kCodecConfigId),
codec_config_({.codec_id = codec_id,
.num_samples_per_frame = 64,
.audio_roll_distance = 0,
Expand Down Expand Up @@ -155,7 +158,7 @@ class CodecConfigLpcmTest : public CodecConfigTestBase, public testing::Test {
.sample_size_ = 16,
.sample_rate_ = 48000}) {
expected_payload_ = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
Expand All @@ -171,6 +174,12 @@ class CodecConfigLpcmTest : public CodecConfigTestBase, public testing::Test {
}
};

TEST_F(CodecConfigLpcmTest, IsAlwaysLossless) {
InitExpectOk();

EXPECT_TRUE(obu_->IsLossless());
}

TEST_F(CodecConfigLpcmTest, ConstructorSetsObuTyoe) {
InitExpectOk();

Expand Down Expand Up @@ -275,7 +284,7 @@ TEST_F(CodecConfigLpcmTest, NumSamplesPerFrame) {
codec_config_.num_samples_per_frame = 128;
expected_header_ = {0, 15};
expected_payload_ = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
Expand All @@ -296,7 +305,7 @@ TEST_F(CodecConfigLpcmTest, SampleFormatFlags) {
std::get<LpcmDecoderConfig>(codec_config_.decoder_config)
.sample_format_flags_bitmask_ = LpcmDecoderConfig::kLpcmLittleEndian;
expected_payload_ = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
Expand All @@ -316,7 +325,7 @@ TEST_F(CodecConfigLpcmTest, SampleFormatFlags) {
TEST_F(CodecConfigLpcmTest, WriteSampleSize) {
std::get<LpcmDecoderConfig>(codec_config_.decoder_config).sample_size_ = 24;
expected_payload_ = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
Expand Down Expand Up @@ -344,7 +353,7 @@ TEST_F(CodecConfigLpcmTest, WriteSampleRate) {
std::get<LpcmDecoderConfig>(codec_config_.decoder_config).sample_rate_ =
16000;
expected_payload_ = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
Expand Down Expand Up @@ -395,12 +404,19 @@ class CodecConfigOpusTest : public CodecConfigTestBase, public testing::Test {
codec_config_.num_samples_per_frame = 960;
codec_config_.audio_roll_distance = -4;
expected_header_ = {0, 20};
expected_payload_ = {0, 'O', 'p', 'u', 's', 0xc0, 0x07, 0xff, 0xfc,
expected_payload_ = {kCodecConfigId, 'O', 'p', 'u', 's', 0xc0, 0x07, 0xff,
0xfc,
// Start `DecoderConfig`.
1, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0};
}
};

TEST_F(CodecConfigOpusTest, IsNeverLossless) {
InitExpectOk();

EXPECT_FALSE(obu_->IsLossless());
}

TEST_F(CodecConfigOpusTest, ManyLargeValues) {
leb_generator_ =
LebGenerator::Create(LebGenerator::GenerationMode::kFixedSize, 8);
Expand Down Expand Up @@ -459,12 +475,12 @@ TEST_F(CodecConfigOpusTest,
TEST_F(CodecConfigOpusTest, Default) { InitAndTestWrite(); }

TEST_F(CodecConfigOpusTest, VarySeveralFields) {
codec_config_id_ = 123;
codec_config_id_ = 99;
std::get<OpusDecoderConfig>(codec_config_.decoder_config).version_ = 15;
std::get<OpusDecoderConfig>(codec_config_.decoder_config).pre_skip_ = 3;
std::get<OpusDecoderConfig>(codec_config_.decoder_config).input_sample_rate_ =
4;
expected_payload_ = {123, 'O', 'p', 'u', 's', 0xc0, 0x07, 0xff, 0xfc,
expected_payload_ = {99, 'O', 'p', 'u', 's', 0xc0, 0x07, 0xff, 0xfc,
// Start `DecoderConfig`.
// `version`.
15,
Expand All @@ -489,14 +505,19 @@ TEST_F(CodecConfigOpusTest, RedundantCopy) {
}

TEST(CreateFromBuffer, OpusDecoderConfig) {
std::vector<uint8_t> source_data = {123, 'O', 'p', 'u', 's',
constexpr DecodedUleb128 kExpectedNumSamplesPerFrame = 960;
constexpr int16_t kExpectedAudioRollDistance = -4;
constexpr uint8_t kVersion = 15;
constexpr int16_t kExpectedPreSkip = 3;
constexpr int16_t kExpectedInputSampleRate = 4;
std::vector<uint8_t> source_data = {kCodecConfigId, 'O', 'p', 'u', 's',
// num_samples_per_frame
0xc0, 0x07,
// audio_roll_distance
0xff, 0xfc,
// Start `DecoderConfig`.
// `version`.
15,
kVersion,
// `output_channel_count`.
OpusDecoderConfig::kOutputChannelCount,
// `pre_skip`
Expand All @@ -510,53 +531,76 @@ TEST(CreateFromBuffer, OpusDecoderConfig) {
OpusDecoderConfig::kMappingFamily};
ReadBitBuffer buffer(1024, &source_data);
ObuHeader header;

absl::StatusOr<CodecConfigObu> obu =
CodecConfigObu::CreateFromBuffer(header, buffer);
EXPECT_THAT(obu, IsOk());

// Set up expected data
DecodedUleb128 expected_codec_config_id = 123;
CodecConfig expected_codec_config = {
.codec_id = static_cast<CodecConfig::CodecId>(CodecConfig::kCodecIdOpus),
.num_samples_per_frame = 960,
.audio_roll_distance = -4,
.decoder_config =
OpusDecoderConfig{
.version_ = 15,
.output_channel_count_ = OpusDecoderConfig::kOutputChannelCount,
.pre_skip_ = 3,
.input_sample_rate_ = 4,
.output_gain_ = 0,
.mapping_family_ = OpusDecoderConfig::kMappingFamily},
};

// Validate fields
EXPECT_EQ(obu.value().GetCodecConfigId(), expected_codec_config_id);
EXPECT_EQ(obu.value().GetCodecConfig(), expected_codec_config);
}
EXPECT_EQ(obu->GetCodecConfigId(), kCodecConfigId);
EXPECT_EQ(obu->GetCodecConfig().codec_id, CodecConfig::kCodecIdOpus);
EXPECT_EQ(obu->GetNumSamplesPerFrame(), kExpectedNumSamplesPerFrame);
EXPECT_EQ(obu->GetCodecConfig().audio_roll_distance,
kExpectedAudioRollDistance);
ASSERT_TRUE(std::holds_alternative<OpusDecoderConfig>(
obu->GetCodecConfig().decoder_config));
const auto& opus_decoder_config =
std::get<OpusDecoderConfig>(obu->GetCodecConfig().decoder_config);
EXPECT_EQ(opus_decoder_config.version_, kVersion);
EXPECT_EQ(opus_decoder_config.output_channel_count_,
OpusDecoderConfig::kOutputChannelCount);
EXPECT_EQ(opus_decoder_config.pre_skip_, kExpectedPreSkip);
EXPECT_EQ(opus_decoder_config.input_sample_rate_, kExpectedInputSampleRate);
EXPECT_EQ(opus_decoder_config.output_gain_, OpusDecoderConfig::kOutputGain);
EXPECT_EQ(opus_decoder_config.mapping_family_,
OpusDecoderConfig::kMappingFamily);
EXPECT_FALSE(obu->IsLossless());
};

// TODO(b/331833384, b/331831926): Add test cases for other
// decoder configs.
TEST(CreateFromBuffer, ValidLpcmDecoderConfig) {
constexpr DecodedUleb128 kNumSamplesPerFrame = 64;
constexpr int16_t kExpectedAudioRollDistance = 0;
constexpr uint8_t kSampleFormatFlagsAsUint8 = 0x00;
constexpr uint8_t kSampleSize = 16;
constexpr uint32_t kExpectedSampleRate = 48000;

std::vector<uint8_t> source_data = {// `codec_config_id`.
0,
kCodecConfigId,
// `codec_id`.
'i', 'p', 'c', 'm',
// `num_samples_per_frame`.
64,
kNumSamplesPerFrame,
// `audio_roll_distance`.
0, 0,
// `sample_format_flags`.
0,
kSampleFormatFlagsAsUint8,
// `sample_size`.
16,
kSampleSize,
// `sample_rate`.
0, 0, 0xbb, 0x80};
ReadBitBuffer buffer(1024, &source_data);
ObuHeader header;

absl::StatusOr<CodecConfigObu> obu =
CodecConfigObu::CreateFromBuffer(header, buffer);

EXPECT_THAT(obu, IsOk());
EXPECT_EQ(obu->GetCodecConfigId(), kCodecConfigId);
EXPECT_EQ(obu->GetCodecConfig().codec_id, CodecConfig::kCodecIdLpcm);
EXPECT_EQ(obu->GetNumSamplesPerFrame(), kNumSamplesPerFrame);
EXPECT_EQ(obu->GetCodecConfig().audio_roll_distance,
kExpectedAudioRollDistance);
ASSERT_TRUE(std::holds_alternative<LpcmDecoderConfig>(
obu->GetCodecConfig().decoder_config));
const auto& lpcm_decoder_config =
std::get<LpcmDecoderConfig>(obu->GetCodecConfig().decoder_config);
EXPECT_EQ(
static_cast<uint8_t>(lpcm_decoder_config.sample_format_flags_bitmask_),
kSampleFormatFlagsAsUint8);
EXPECT_EQ(lpcm_decoder_config.sample_size_, kSampleSize);
EXPECT_EQ(lpcm_decoder_config.sample_rate_, kExpectedSampleRate);
EXPECT_TRUE(obu->IsLossless());
}

} // namespace
Expand Down

0 comments on commit df2ff29

Please sign in to comment.