Skip to content

Commit

Permalink
MixPresentationGenerator: Infer num_tags instead of requiring use…
Browse files Browse the repository at this point in the history
…r input to match.

  - b/397462369 This signal is pretty useless:
    - When it matches the number of `tags`, then encoding would succeed.
    - When it did not match the number of `tags`, then encoding would fail.
    - Therefore, it is redundant with the number of `tags`, we can simply infer the correct value.
    - N.b.: "zero" vs. "absent" tags case are distinguished via `include_mix_presentation_tags` (`CopiesMixPresentationTagsWithZeroTags` vs `IgnoresTagsWhenSetIncludeMixPresentationTagsIsFalse`).
  - Motivation (b/388577499):
    - In the future, the encoder may start inserting custom tags automatically.
    - Avoid complicating behavior (what would `num_tags` represent at the proto layer, if the encoder adds implicit ones).
    - Dropping the field means we can simply ignore and not care about the answer to this question - all we care is that the generated OBU has the final correct value.
  - Tests:
    - Add coverage that a poorly set `num_tags` is now ignored.
    - Backfill test coverage when there are more than 255 tags (spec max).

PiperOrigin-RevId: 728647443
  • Loading branch information
jwcullen committed Feb 19, 2025
1 parent 2ea5d8d commit d6e92ac
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 3 deletions.
4 changes: 3 additions & 1 deletion iamf/cli/proto/mix_presentation.proto
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,9 @@ message MixPresentationTag {
}

message MixPresentationTags {
optional uint32 num_tags = 1;
// `num_tags` is ignored. The value in the bitstream is inferred based on the
// number of tags.
optional uint32 num_tags = 1 [deprecated = true];
repeated MixPresentationTag tags = 2;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
*/
#include "iamf/cli/proto_conversion/proto_to_obu/mix_presentation_generator.h"

#include <cstddef>
#include <cstdint>
#include <list>
#include <optional>
Expand Down Expand Up @@ -304,10 +305,14 @@ absl::Status FillLayouts(
absl::Status FillMixPresentationTags(
const iamf_tools_cli_proto::MixPresentationTags& mix_presentation_tags,
std::optional<MixPresentationTags>& obu_mix_presentation_tags) {
if (mix_presentation_tags.has_num_tags()) {
LOG(WARNING) << "Ignoring deprecated `num_tags` field. Please remove it.";
}
obu_mix_presentation_tags = MixPresentationTags{};
RETURN_IF_NOT_OK(StaticCastIfInRange<uint32_t, uint8_t>(
"MixPresentationTags.num_tags", mix_presentation_tags.num_tags(),
RETURN_IF_NOT_OK(StaticCastIfInRange<size_t, uint8_t>(
"Number of MixPresentationTags.tags", mix_presentation_tags.tags().size(),
obu_mix_presentation_tags->num_tags));
obu_mix_presentation_tags->tags.reserve(mix_presentation_tags.tags().size());
for (const auto& input_tag : mix_presentation_tags.tags()) {
obu_mix_presentation_tags->tags.push_back(MixPresentationTags::Tag{
.tag_name = input_tag.tag_name(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ namespace {

using ::absl_testing::IsOk;
using ::testing::ElementsAreArray;
using ::testing::Not;
typedef ::google::protobuf::RepeatedPtrField<
iamf_tools_cli_proto::MixPresentationObuMetadata>
MixPresentationObuMetadatas;
Expand Down Expand Up @@ -428,6 +429,45 @@ TEST(Generate, CopiesMixPresentationTagsWithZeroTags) {
EXPECT_TRUE(first_obu.mix_presentation_tags_->tags.empty());
}

TEST(Generate, IgnoresDeprecatedNumTags) {
MixPresentationObuMetadatas mix_presentation_metadata;
FillMixPresentationMetadata(mix_presentation_metadata.Add());
auto& mix_presentation = mix_presentation_metadata.at(0);
mix_presentation.set_include_mix_presentation_tags(true);
constexpr uint32_t kIncorrectIgnoredNumTags = 1;
mix_presentation.mutable_mix_presentation_tags()->set_num_tags(
kIncorrectIgnoredNumTags);

MixPresentationGenerator generator(mix_presentation_metadata);

std::list<MixPresentationObu> generated_obus;
EXPECT_THAT(generator.Generate(generated_obus), IsOk());

// Ok safely ignore the deprecated `num_tags` field.
const auto& first_obu = generated_obus.front();
ASSERT_TRUE(first_obu.mix_presentation_tags_.has_value());
EXPECT_EQ(first_obu.mix_presentation_tags_->num_tags, 0);
EXPECT_TRUE(first_obu.mix_presentation_tags_->tags.empty());
}

TEST(Generate, ReturnsErrorWhenTooManyTagsArePresent) {
MixPresentationObuMetadatas mix_presentation_metadata;
FillMixPresentationMetadata(mix_presentation_metadata.Add());
auto& mix_presentation = mix_presentation_metadata.at(0);
mix_presentation.set_include_mix_presentation_tags(true);
constexpr int kMaxNumTags = 255;
for (int i = 0; i < kMaxNumTags + 1; ++i) {
auto* tag = mix_presentation.mutable_mix_presentation_tags()->add_tags();
tag->set_tag_name("tag_name");
tag->set_tag_value("tag_value");
}

MixPresentationGenerator generator(mix_presentation_metadata);

std::list<MixPresentationObu> ignored_generated_obus;
EXPECT_THAT(generator.Generate(ignored_generated_obus), Not(IsOk()));
}

TEST(Generate, CopiesDuplicateContentLanguageTags) {
MixPresentationObuMetadatas mix_presentation_metadata;
FillMixPresentationMetadata(mix_presentation_metadata.Add());
Expand Down

0 comments on commit d6e92ac

Please sign in to comment.