Skip to content

Commit

Permalink
Cleanups related to LookupInMap and other template functions.
Browse files Browse the repository at this point in the history
  - `obu_util.h`: Tweak generic "validation"-themed template functions
     - Standardize use of `absl::string_view` for `context` (vs. `std::string`).
     - Add `likely/unlikely` [attributes](https://en.cppreference.com/w/cpp/language/attributes/likely). Mostly these are used for the "happy case", and lead to downstream unrecoverable failures when they fail.
  - Map lookups:
    - Unify naming to prefer `CopyFrom*` when lookups write to an output argument.
      - Rename existing template to `CopyFromMap` because it uses this style.
    - Unify naming to prefer `LookupFrom*` for lookups that return `StatusOr<T>`.
      - Add a new `LookupFromMap` template which uses this style. Use it throughout codebase.
    - Pass key by const reference, in case it is a complex type.
    - Update both templates to have a `context` string.
      - With the context string, and the template logging the key which caused lookup to fail, we can cleanup a lot of boiler-plate for lookup failures and rely on the template to handle the logging.
       - Standardize "context" language when lookup fails a bit.
  - Switch to `absl::NotFoundError` when lookup fails, which seems more canonical.
    - Drive-by: This triggered a stray test failure. Update it to prefer not checking explicit error codes.
  - Switch some underlying map types to use `absl::string_view` instead of `std::string`.

PiperOrigin-RevId: 689810011
  • Loading branch information
jwcullen committed Oct 28, 2024
1 parent cde39e8 commit 01235b2
Show file tree
Hide file tree
Showing 23 changed files with 205 additions and 210 deletions.
1 change: 0 additions & 1 deletion iamf/cli/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,6 @@ cc_library(
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/log",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
],
)

Expand Down
4 changes: 3 additions & 1 deletion iamf/cli/adm_to_user_metadata/iamf/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ cc_library(
"//iamf/cli/proto:audio_element_cc_proto",
"//iamf/cli/proto:ia_sequence_header_cc_proto",
"//iamf/cli/proto:user_metadata_cc_proto",
"//iamf/common:obu_util",
"@com_google_absl//absl/base:no_destructor",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/status",
Expand All @@ -29,6 +30,7 @@ cc_library(
":iamf_input_layout",
"//iamf/cli/proto:audio_frame_cc_proto",
"//iamf/cli/proto:user_metadata_cc_proto",
"//iamf/common:obu_util",
"@com_google_absl//absl/base:no_destructor",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/status",
Expand Down Expand Up @@ -88,9 +90,9 @@ cc_library(
srcs = ["iamf_input_layout.cc"],
hdrs = ["iamf_input_layout.h"],
deps = [
"//iamf/common:obu_util",
"@com_google_absl//absl/base:no_destructor",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
],
Expand Down
33 changes: 9 additions & 24 deletions iamf/cli/adm_to_user_metadata/iamf/audio_element_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "iamf/cli/adm_to_user_metadata/iamf/iamf_input_layout.h"
#include "iamf/cli/proto/audio_element.pb.h"
#include "iamf/cli/proto/user_metadata.pb.h"
#include "iamf/common/obu_util.h"

namespace iamf_tools {
namespace adm_to_user_metadata {
Expand Down Expand Up @@ -49,12 +50,8 @@ absl::StatusOr<int32_t> LookupNumSubstreamsFromInputLayout(
{kAmbisonicsOrder3, 16},
});

auto it = kInputLayoutToNumSubstreams->find(input_layout);
if (it == kInputLayoutToNumSubstreams->end()) {
return absl::NotFoundError(absl::StrCat(
"Num substreams not found for input_layout= ", input_layout));
}
return it->second;
return LookupInMap(*kInputLayoutToNumSubstreams, input_layout,
"Number of channels for `IamfInputLayout`");
}

absl::StatusOr<int32_t> LookupCoupledSubstreamCountFromInputLayout(
Expand All @@ -74,12 +71,8 @@ absl::StatusOr<int32_t> LookupCoupledSubstreamCountFromInputLayout(
{kBinaural, 1},
});

auto it = kInputLayoutToCoupledSubstreamCount->find(input_layout);
if (it == kInputLayoutToCoupledSubstreamCount->end()) {
return absl::NotFoundError(absl::StrCat(
"Coupled substream count not found for input_layout= ", input_layout));
}
return it->second;
return LookupInMap(*kInputLayoutToCoupledSubstreamCount, input_layout,
"Coupled substream count for `IamfInputLayout`");
}

absl::StatusOr<iamf_tools_cli_proto::LoudspeakerLayout>
Expand All @@ -102,12 +95,8 @@ LookupLoudspeakerLayoutFromInputLayout(IamfInputLayout input_layout) {
{kBinaural, LOUDSPEAKER_LAYOUT_BINAURAL},
});

auto it = KInputLayoutToLoudspeakerLayout->find(input_layout);
if (it == KInputLayoutToLoudspeakerLayout->end()) {
return absl::NotFoundError(absl::StrCat(
"Loudspeaker layout not found for input_layout= ", input_layout));
}
return it->second;
return LookupInMap(*KInputLayoutToLoudspeakerLayout, input_layout,
"Proto `LoudspeakerLayout` for `IamfInputLayout`");
}

absl::StatusOr<iamf_tools_cli_proto::AudioElementType>
Expand All @@ -133,12 +122,8 @@ LookupAudioElementTypeFromInputLayout(IamfInputLayout input_layout) {
{kAmbisonicsOrder3, AUDIO_ELEMENT_SCENE_BASED},
});

auto it = KInputLayoutToAudioElementType->find(input_layout);
if (it == KInputLayoutToAudioElementType->end()) {
return absl::NotFoundError(absl::StrCat(
"Loudspeaker layout not found for input_layout= ", input_layout));
}
return it->second;
return LookupInMap(*KInputLayoutToAudioElementType, input_layout,
"Proto `AudioElementType` for `IamfInputLayout`");
}

absl::Status PopulateChannelBasedAudioElementMetadata(
Expand Down
9 changes: 3 additions & 6 deletions iamf/cli/adm_to_user_metadata/iamf/audio_frame_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "iamf/cli/adm_to_user_metadata/iamf/iamf_input_layout.h"
#include "iamf/cli/proto/audio_frame.pb.h"
#include "iamf/cli/proto/user_metadata.pb.h"
#include "iamf/common/obu_util.h"

namespace iamf_tools {
namespace adm_to_user_metadata {
Expand Down Expand Up @@ -59,12 +60,8 @@ absl::StatusOr<std::vector<std::string>> LookupLabelsFromInputLayout(
"A11", "A12", "A13", "A14", "A15"}},
});

auto it = kInputLayoutToLabels->find(input_layout);
if (it == kInputLayoutToLabels->end()) {
return absl::NotFoundError(
absl::StrCat("Labels not found for input_layout= ", input_layout));
}
return it->second;
return LookupInMap(*kInputLayoutToLabels, input_layout,
"String-based channels labels for `IamfInputLayout`");
}

} // namespace
Expand Down
16 changes: 4 additions & 12 deletions iamf/cli/adm_to_user_metadata/iamf/iamf_input_layout.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,11 @@

#include "iamf/cli/adm_to_user_metadata/iamf/iamf_input_layout.h"

#include <string>

#include "absl/base/no_destructor.h"
#include "absl/container/flat_hash_map.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "iamf/common/obu_util.h"

namespace iamf_tools {
namespace adm_to_user_metadata {
Expand All @@ -30,7 +27,7 @@ absl::StatusOr<IamfInputLayout> LookupInputLayoutFromAudioPackFormatId(
// loudspeaker layout in IAMF.
using enum IamfInputLayout;
static const absl::NoDestructor<
absl::flat_hash_map<std::string, IamfInputLayout>>
absl::flat_hash_map<absl::string_view, IamfInputLayout>>
kAudioPackFormatIdToInputLayout({
{"AP_00010001", IamfInputLayout::kMono},
{"AP_00010002", IamfInputLayout::kStereo},
Expand All @@ -45,13 +42,8 @@ absl::StatusOr<IamfInputLayout> LookupInputLayoutFromAudioPackFormatId(
{"AP_00040003", IamfInputLayout::kAmbisonicsOrder3},
});

auto it = kAudioPackFormatIdToInputLayout->find(audio_pack_format_id);
if (it == kAudioPackFormatIdToInputLayout->end()) {
return absl::NotFoundError(
absl::StrCat("Input layout not found for audio_pack_format_id= ",
audio_pack_format_id));
}
return it->second;
return LookupInMap(*kAudioPackFormatIdToInputLayout, audio_pack_format_id,
"`IamfInputLayout` for `audio_pack_format_id`");
}

} // namespace adm_to_user_metadata
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,8 @@ LookupSoundSystemFromInputLayout(IamfInputLayout layout) {
{k7_1_4, SOUND_SYSTEM_J_4_7_0},
});

auto it = kInputLayoutToSoundSystem->find(layout);
if (it == kInputLayoutToSoundSystem->end()) {
return absl::NotFoundError("Sound system not found for input_layout");
}
return it->second;
return LookupInMap(*kInputLayoutToSoundSystem, layout,
"`SoundSystem` for `IamfInputLayout`");
}

absl::Status CopyLoudness(const LoudnessMetadata& loudness_metadata,
Expand Down
28 changes: 7 additions & 21 deletions iamf/cli/channel_label.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

#include <optional>
#include <string>
#include <utility>
#include <vector>

#include "absl/base/no_destructor.h"
Expand Down Expand Up @@ -60,12 +59,8 @@ LookupEarChannelOrderFromNonExpandedLoudspeakerLayout(
{kLayoutBinaural, {kL2, kR2}},
});

auto it = kSoundSystemToLoudspeakerLayout->find(loudspeaker_layout);
if (it == kSoundSystemToLoudspeakerLayout->end()) {
return absl::InvalidArgumentError(absl::StrCat(
"Channel order not found for layout= ", loudspeaker_layout));
}
return it->second;
return LookupInMap(*kSoundSystemToLoudspeakerLayout, loudspeaker_layout,
"`ChannelLabel::Label` for `LoudspeakerLayout`");
}

void SetLabelsToOmittedExceptFor(
Expand Down Expand Up @@ -180,7 +175,7 @@ absl::StatusOr<ChannelLabel::Label> ChannelLabel::StringToLabel(
absl::string_view label) {
using enum ChannelLabel::Label;
static const absl::NoDestructor<
absl::flat_hash_map<std::string, ChannelLabel::Label>>
absl::flat_hash_map<absl::string_view, ChannelLabel::Label>>
kStringToChannelLabel({
{"Omitted", kOmitted},
{"M", kMono},
Expand Down Expand Up @@ -265,12 +260,8 @@ absl::StatusOr<ChannelLabel::Label> ChannelLabel::StringToLabel(
{"A24", kA24},
});

auto it = kStringToChannelLabel->find(label);
if (it == kStringToChannelLabel->end()) {
return absl::InvalidArgumentError(
absl::StrCat("Unknown string-based label= ", label));
}
return it->second;
return LookupInMap(*kStringToChannelLabel, label,
"`Channel::Label` for string-based label");
}

absl::StatusOr<ChannelLabel::Label> ChannelLabel::ProtoToLabel(
Expand Down Expand Up @@ -607,13 +598,8 @@ absl::StatusOr<ChannelLabel::Label> ChannelLabel::GetDemixedLabel(
{kR7, kDemixedR7},
{kLrs7, kDemixedLrs7},
{kRrs7, kDemixedRrs7}});

auto it = kChannelLabelToDemixedLabel->find(label);
if (it == kChannelLabelToDemixedLabel->end()) {
return absl::InvalidArgumentError(absl::StrCat(
"Demixed label is not known or allowed for label= ", label));
}
return it->second;
return LookupInMap(*kChannelLabelToDemixedLabel, label,
"Demixed label for `ChannelLabel::Label`");
}

absl::StatusOr<std::vector<ChannelLabel::Label>>
Expand Down
1 change: 0 additions & 1 deletion iamf/cli/obu_to_proto/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,5 @@ cc_library(
"@com_google_absl//absl/log:check",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
],
)
10 changes: 2 additions & 8 deletions iamf/cli/obu_to_proto/parameter_block_metadata_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "absl/log/log.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "absl/strings/str_cat.h"
#include "iamf/cli/proto/parameter_block.pb.h"
#include "iamf/cli/proto/parameter_data.pb.h"
#include "iamf/common/macros.h"
Expand Down Expand Up @@ -113,14 +112,9 @@ absl::Status CopyDMixPMode(DemixingInfoParameterData::DMixPMode obu_dmixp_mode,
{kDMixPMode3_n, DMIXP_MODE_3_N},
{kDMixPModeReserved2, DMIXP_MODE_RESERVED_B},
});
if (!LookupInMap(*kObuDmixPModeToMetadataDMixPMode, obu_dmixp_mode,
dmixp_mode)
.ok()) {
return absl::InvalidArgumentError(
absl::StrCat("Unknown dmixp_mode = ", obu_dmixp_mode));
}

return absl::OkStatus();
return CopyFromMap(*kObuDmixPModeToMetadataDMixPMode, obu_dmixp_mode,
"Proto version of internal `DMixPMode`", dmixp_mode);
}

// Gets the proto representation of the input `mix_gain_parameter_data`.
Expand Down
10 changes: 3 additions & 7 deletions iamf/cli/proto_to_obu/arbitrary_obu_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,9 @@ absl::Status CopyArbitraryObuType(
{OBU_IA_SEQUENCE_HEADER, kObuIaSequenceHeader},
});

if (!LookupInMap(*kArbitraryObuTypeToObuType, arbitrary_obu_type,
output_obu_type)
.ok()) {
return absl::InvalidArgumentError(
absl::StrCat("Unknown arbitrary_obu_type= ", arbitrary_obu_type));
}
return absl::OkStatus();
return CopyFromMap(*kArbitraryObuTypeToObuType, arbitrary_obu_type,
"Internal version of proto `ArbitraryObuType`",
output_obu_type);
}
} // namespace

Expand Down
27 changes: 8 additions & 19 deletions iamf/cli/proto_to_obu/audio_element_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -721,16 +721,10 @@ absl::Status CopyLoudspeakerLayout(
{LOUDSPEAKER_LAYOUT_EXPANDED, kLayoutExpanded},
});

if (!LookupInMap(*kInputLoudspeakerLayoutToOutputLoudspeakerLayout,
input_channel_audio_layer_config.loudspeaker_layout(),
output_loudspeaker_layout)
.ok()) {
return InvalidArgumentError(
StrCat("Unknown loudspeaker_layout= ",
input_channel_audio_layer_config.loudspeaker_layout()));
}

return absl::OkStatus();
return CopyFromMap(*kInputLoudspeakerLayoutToOutputLoudspeakerLayout,
input_channel_audio_layer_config.loudspeaker_layout(),
"Internal version of proto `LoudspeakerLayout`= ",
output_loudspeaker_layout);
}

// Copies the `ExpandedLoudspeakerLayout` based on the input data.
Expand Down Expand Up @@ -760,15 +754,10 @@ absl::Status CopyExpandedLoudspeakerLayout(
{EXPANDED_LOUDSPEAKER_LAYOUT_TOP_6_CH, kExpandedLayoutTop6Ch},
});

if (!LookupInMap(*kInputLoudspeakerLayoutToOutputLoudspeakerLayout,
input_expanded_loudspeaker_layout,
output_expanded_loudspeaker_layout)
.ok()) {
return InvalidArgumentError(StrCat("Unknown expanded_loudspeaker_layout= ",
input_expanded_loudspeaker_layout));
}

return absl::OkStatus();
return CopyFromMap(*kInputLoudspeakerLayoutToOutputLoudspeakerLayout,
input_expanded_loudspeaker_layout,
"Internal version of proto `ExpandedLoudspeakerLayout`= ",
output_expanded_loudspeaker_layout);
}

// Copies the `LoudspeakerLayout` and `ExpandedLoudspeakerLayout` based on the
Expand Down
32 changes: 10 additions & 22 deletions iamf/cli/proto_to_obu/codec_config_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,9 @@ absl::Status CopyCodecId(
{CODEC_ID_LPCM, kCodecIdLpcm},
});

if (!LookupInMap(*kInputCodecIdToOutputCodecId, input_codec_config.codec_id(),
output_codec_id)
.ok()) {
return absl::InvalidArgumentError(absl::StrCat(
"Unknown codec with codec_id= ", input_codec_config.codec_id()));
}
return absl::OkStatus();
return CopyFromMap(*kInputCodecIdToOutputCodecId,
input_codec_config.codec_id(),
"Internal version of proto `CodecId`= ", output_codec_id);
}

absl::Status CopyFlacBlockType(
Expand All @@ -91,13 +87,9 @@ absl::Status CopyFlacBlockType(
{FLAC_BLOCK_TYPE_CUESHEET, kFlacCuesheet},
{FLAC_BLOCK_TYPE_PICTURE, kFlacPicture}});

if (!LookupInMap(*kInputFlacBlockTypeToOutputFlacBlockType,
input_flac_block_type, output_flac_block_type)
.ok()) {
return absl::InvalidArgumentError(
absl::StrCat("Unknown input_flac_block_type= ", input_flac_block_type));
}
return absl::OkStatus();
return CopyFromMap(
*kInputFlacBlockTypeToOutputFlacBlockType, input_flac_block_type,
"Internal version of proto `FlacBlockType`", output_flac_block_type);
}

absl::Status CopySampleFrequencyIndex(
Expand Down Expand Up @@ -131,14 +123,10 @@ absl::Status CopySampleFrequencyIndex(
kSampleFrequencyIndexEscapeValue},
});

if (!LookupInMap(*kInputSampleFrequencyIndexToOutputSampleFrequencyIndex,
input_sample_frequency_index, output_sample_frequency_index)
.ok()) {
return absl::InvalidArgumentError(
absl::StrCat("Unknown input_sample_frequency_index= ",
input_sample_frequency_index));
}
return absl::OkStatus();
return CopyFromMap(*kInputSampleFrequencyIndexToOutputSampleFrequencyIndex,
input_sample_frequency_index,
"Internal version of proto `SampleFrequencyIndex`",
output_sample_frequency_index);
}

absl::Status GenerateLpcmDecoderConfig(
Expand Down
Loading

0 comments on commit 01235b2

Please sign in to comment.