Skip to content

Commit

Permalink
Fix a bug where ParameterBlockPartitioner could create too many sub…
Browse files Browse the repository at this point in the history 
…blocks.

  - IAMF REQUIRES demixing and recon gain parameter blocks only have one subblock.
    - Previous code had an index checking error and would create outputs with two subblocks in some circumstances.
    - This bug did not affect the end-user because the "two-subblock" parameter blocks would be detected as erroneous later down the line.
  - Drive-by change: Fill in some error messages.

PiperOrigin-RevId: 643038689
  • Loading branch information
@jwcullen
jwcullen committed Jun 13, 2024
1 parent 4ad1079 commit 6be3a02
Show file tree
Hide file tree
Showing 3 changed files with 201 additions and 11 deletions.
23 changes: 12 additions & 11 deletions iamf/cli/parameter_block_partitioner.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,21 +240,21 @@ absl::Status GetPartitionedSubblocks(
subblock_end_time, partitioned_subblock_start,
partitioned_subblock_end, partitioned_subblock));
} else if (subblock_i.has_demixing_info_parameter_data()) {
if (i > 1) {
LOG(ERROR) << "There should only be one subblock for demixing info.";
return absl::InvalidArgumentError("");
if (!partitioned_subblocks.empty()) {
return absl::InvalidArgumentError(
"There should only be one subblock for demixing info.");
}
*partitioned_subblock.mutable_demixing_info_parameter_data() =
subblock_i.demixing_info_parameter_data();
} else if (subblock_i.has_recon_gain_info_parameter_data()) {
if (i > 1) {
LOG(ERROR) << "There should only be one subblock for recon gain info.";
return absl::InvalidArgumentError("");
if (!partitioned_subblocks.empty()) {
return absl::InvalidArgumentError(
"There should only be one subblock for recon gain info.");
}
*partitioned_subblock.mutable_recon_gain_info_parameter_data() =
subblock_i.recon_gain_info_parameter_data();
} else {
return absl::InvalidArgumentError("");
return absl::InvalidArgumentError("Unknown subblock type.");
}
partitioned_subblocks.push_back(partitioned_subblock);

Expand Down Expand Up @@ -343,10 +343,11 @@ absl::Status ParameterBlockPartitioner::PartitionParameterBlock(
int32_t partitioned_start_time, int32_t partitioned_end_time,
ParameterBlockObuMetadata& partitioned) {
if (partitioned_start_time >= partitioned_end_time) {
LOG(ERROR) << "Cannot partition a parameter block starting at "
<< partitioned_start_time << " and ending at "
<< partitioned_end_time;
return absl::InvalidArgumentError("");
return absl::InvalidArgumentError(
absl::StrCat("Cannot partition a parameter block with < 1 duration. "
"(partitioned_start_time= ",
partitioned_start_time,
" partitioned_end_time=", partitioned_end_time));
}

// Find the subblocks that overlap this partition.
Expand Down
1 change: 1 addition & 0 deletions iamf/cli/tests/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ cc_test(
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:status_matchers",
"@com_google_googletest//:gtest_main",
"@com_google_protobuf//:protobuf",
],
)

Expand Down
188 changes: 188 additions & 0 deletions iamf/cli/tests/parameter_block_partitioner_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
*/
#include "iamf/cli/parameter_block_partitioner.h"

#include <cstddef>
#include <cstdint>
#include <vector>

Expand All @@ -20,6 +21,7 @@
#include "gtest/gtest.h"
#include "iamf/cli/proto/parameter_block.pb.h"
#include "iamf/cli/proto/parameter_data.pb.h"
#include "src/google/protobuf/text_format.h"

namespace iamf_tools {
namespace {
Expand Down Expand Up @@ -312,6 +314,192 @@ INSTANTIATE_TEST_SUITE_P(
true},
}));

TEST(PartitionParameterBlock, InvalidForUnknownOrMissingParameterData) {
ParameterBlockObuMetadata full_parameter_block;
google::protobuf::TextFormat::ParseFromString(
R"pb(
parameter_id: 100
start_timestamp: 0
duration: 4000
num_subblocks: 1
constant_subblock_duration: 4000
subblocks {
# Parameter data is missing.
}
)pb",
&full_parameter_block);

ParameterBlockObuMetadata unused_parameter_block;
EXPECT_FALSE(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/0,
/*partitioned_end_time=*/4000, unused_parameter_block)
.ok());
}

void ExpectHasOneSubblockWithDMixPMode(
const ParameterBlockObuMetadata& parameter_block_metadata,
const iamf_tools_cli_proto::DMixPMode& expected_dmixp_mode) {
EXPECT_EQ(parameter_block_metadata.subblocks().size(), 1);
ASSERT_TRUE(
parameter_block_metadata.subblocks(0).has_demixing_info_parameter_data());
EXPECT_EQ(parameter_block_metadata.subblocks(0)
.demixing_info_parameter_data()
.dmixp_mode(),
expected_dmixp_mode);
}

TEST(PartitionParameterBlock,
IsEquivalentWhenSubblockBoundaryIsNotCrossedForDemixing) {
ParameterBlockObuMetadata full_parameter_block;
google::protobuf::TextFormat::ParseFromString(
R"pb(
parameter_id: 100
start_timestamp: 0
duration: 12000
num_subblocks: 3
constant_subblock_duration: 4000
# t = [0, 4000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_1 } }
# t = [4000, 8000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_3 } }
# t = [8000, 12000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_2 } }
)pb",
&full_parameter_block);

// OK if it spans the whole (semi-open) range.
ParameterBlockObuMetadata partition_from_first_subblock;
EXPECT_THAT(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/0,
/*partitioned_end_time=*/4000, partition_from_first_subblock),
IsOk());
ExpectHasOneSubblockWithDMixPMode(partition_from_first_subblock,
iamf_tools_cli_proto::DMIXP_MODE_1);
// OK if the new duration is shorter than the original subblock duration.
ParameterBlockObuMetadata partition_from_third_subblock;
EXPECT_THAT(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/9000,
/*partitioned_end_time=*/9001, partition_from_third_subblock),
IsOk());
ExpectHasOneSubblockWithDMixPMode(partition_from_third_subblock,
iamf_tools_cli_proto::DMIXP_MODE_2);
}

TEST(PartitionParameterBlock, InvalidWhenSubblockBoundaryIsCrossedForDemixing) {
ParameterBlockObuMetadata full_parameter_block;
google::protobuf::TextFormat::ParseFromString(
R"pb(
parameter_id: 100
start_timestamp: 0
duration: 12000
num_subblocks: 3
constant_subblock_duration: 4000
# t = [0, 4000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_1 } }
# t = [4000, 8000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_3 } }
# t = [8000, 12000).
subblocks { demixing_info_parameter_data { dmixp_mode: DMIXP_MODE_2 } }
)pb",
&full_parameter_block);

ParameterBlockObuMetadata unused_partitioned_parameter_block;
EXPECT_FALSE(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/3950,
/*partitioned_end_time=*/4500,
unused_partitioned_parameter_block)
.ok());
EXPECT_FALSE(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/3999,
/*partitioned_end_time=*/4001,
unused_partitioned_parameter_block)
.ok());
}

TEST(PartitionParameterBlock,
IsEquivalentWhenSubblockBoundaryIsNotCrossedForReconGain) {
const int32_t kStartDuration = 0;
const int32_t kEndDuration = 4000;
const int32_t kExpectedDuration = kEndDuration - kStartDuration;
ParameterBlockObuMetadata full_parameter_block;
google::protobuf::TextFormat::ParseFromString(
R"pb(
parameter_id: 100
start_timestamp: 0
duration: 8000
num_subblocks: 1
constant_subblock_duration: 8000
subblocks {
recon_gain_info_parameter_data {
recon_gains_for_layer {}
recon_gains_for_layer { recon_gain { key: 2 value: 200 } }
}
}
)pb",
&full_parameter_block);
const size_t kNumLayers = 2;
const size_t kNumReconGainsForSecondLayer = 1;
const size_t kSecondLayerReconGainValueForKey2 = 200;

ParameterBlockObuMetadata partitioned_parameter_block;
EXPECT_THAT(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, kStartDuration, kEndDuration,
partitioned_parameter_block),
IsOk());

EXPECT_EQ(partitioned_parameter_block.duration(), kExpectedDuration);
EXPECT_EQ(partitioned_parameter_block.subblocks().size(), 1);
ASSERT_TRUE(partitioned_parameter_block.subblocks(0)
.has_recon_gain_info_parameter_data());
const auto& recon_gain_info_parameter_data =
partitioned_parameter_block.subblocks(0).recon_gain_info_parameter_data();
EXPECT_EQ(recon_gain_info_parameter_data.recon_gains_for_layer().size(),
kNumLayers);
EXPECT_TRUE(recon_gain_info_parameter_data.recon_gains_for_layer(0)
.recon_gain()
.empty());
const auto& second_layer_recon_gains =
recon_gain_info_parameter_data.recon_gains_for_layer(1);
EXPECT_EQ(second_layer_recon_gains.recon_gain().size(),
kNumReconGainsForSecondLayer);
EXPECT_EQ(second_layer_recon_gains.recon_gain().at(2),
kSecondLayerReconGainValueForKey2);
}

TEST(PartitionParameterBlock,
InvalidWhenSubblockBoundaryIsCrossedForReconGain) {
ParameterBlockObuMetadata full_parameter_block;
google::protobuf::TextFormat::ParseFromString(
R"pb(
parameter_id: 100
start_timestamp: 0
duration: 8000
num_subblocks: 2
constant_subblock_duration: 4000
# t = [0, 4000).
subblocks {
recon_gain_info_parameter_data {
recon_gains_for_layer {}
recon_gains_for_layer { recon_gain { key: 2 value: 200 } }
}
}
# t = [4000, 8000).
subblocks {
recon_gain_info_parameter_data {
recon_gains_for_layer {}
recon_gains_for_layer { recon_gain { key: 2 value: 100 } }
}
}
)pb",
&full_parameter_block);

ParameterBlockObuMetadata partitioned_parameter_block;
EXPECT_FALSE(ParameterBlockPartitioner::PartitionParameterBlock(
full_parameter_block, /*partitioned_start_time=*/3999,
/*partitioned_end_time=*/4001, partitioned_parameter_block)
.ok());
}

struct FindConstantSubblockDurationTestCase {
std::vector<uint32_t> input_subblock_durations;
uint32_t expected_constant_subblock_duration;
Expand Down

0 comments on commit 6be3a02

Please sign in to comment.