Implement GlobalTimingModule::Create, harden move semantics.

  - Use a factory function, instead of construct and `Initialize` pattern.
  - Some modules hold it by reference.
    - Delete the move constructor, and have the factory wrap it in `std::unique_ptr`.
    - b/390150766: Improvement because it is now impossible to invalidate the reference by moving to underlying object; moving the pointer is safe.
  - It felt unnecessary to further wrap this in `absl::StatusOr`.

PiperOrigin-RevId: 730984795

iamf/cli/BUILD

@@ -185,6 +185,7 @@ cc_library(
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/log",
+        "@com_google_absl//absl/memory",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
     ],

iamf/cli/global_timing_module.cc

@@ -12,11 +12,14 @@
 #include "iamf/cli/global_timing_module.h"
 
 #include <cstdint>
+#include <memory>
 #include <optional>
+#include <utility>
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/container/flat_hash_set.h"
 #include "absl/log/log.h"
+#include "absl/memory/memory.h"
 #include "absl/status/status.h"
 #include "absl/strings/str_cat.h"
 #include "iamf/cli/audio_element_with_data.h"
@@ -30,34 +33,14 @@
 
 namespace iamf_tools {
 
-absl::Status GlobalTimingModule::GetTimestampsForId(
-    const DecodedUleb128 id, const uint32_t duration,
-    absl::flat_hash_map<DecodedUleb128, TimingData>& id_to_timing_data,
-    InternalTimestamp& start_timestamp, InternalTimestamp& end_timestamp) {
-  auto timing_data_iter = id_to_timing_data.find(id);
-  if (timing_data_iter == id_to_timing_data.end()) {
-    // This allows generating timing information when
-    // `IGNORE_ERRORS_USE_ONLY_FOR_IAMF_TEST_SUITE` is defined.
-    // TODO(b/278865608): Find better solutions to generate negative test
-    //                    vectors.
-    start_timestamp = 0;
-    end_timestamp = duration;
-    return absl::InvalidArgumentError(
-        absl::StrCat("Timestamps for ID: ", id, " not found"));
-  }
-
-  auto& timing_data = timing_data_iter->second;
-  start_timestamp = timing_data.timestamp;
-  end_timestamp = start_timestamp + duration;
-  timing_data.timestamp += duration;
-  return absl::OkStatus();
-}
+namespace {
 
-absl::Status GlobalTimingModule::Initialize(
+absl::Status InitializeInternal(
     const absl::flat_hash_map<DecodedUleb128, AudioElementWithData>&
         audio_elements,
     const absl::flat_hash_map<DecodedUleb128, const ParamDefinition*>&
-        param_definitions) {
+        param_definitions,
+    auto& audio_frame_timing_data, auto& parameter_block_timing_data) {
   // TODO(b/283281856): Handle cases where `parameter_rate` and `sample_rate`
   //                    differ.
   for (const auto& [unused_id, audio_element] : audio_elements) {
@@ -70,7 +53,7 @@ absl::Status GlobalTimingModule::Initialize(
       RETURN_IF_NOT_OK(
           ValidateNotEqual(sample_rate, uint32_t{0}, "sample rate"));
 
-      const auto [unused_iter, inserted] = audio_frame_timing_data_.insert(
+      const auto [unused_iter, inserted] = audio_frame_timing_data.insert(
           {audio_substream_id, {.rate = sample_rate, .timestamp = 0}});
 
       if (!inserted) {
@@ -87,7 +70,7 @@ absl::Status GlobalTimingModule::Initialize(
     RETURN_IF_NOT_OK(
         ValidateNotEqual(parameter_rate, DecodedUleb128(0), "parameter rate"));
 
-    const auto [unused_iter, inserted] = parameter_block_timing_data_.insert(
+    const auto [unused_iter, inserted] = parameter_block_timing_data.insert(
         {parameter_id, {.rate = parameter_rate, .timestamp = 0}});
     if (!inserted) {
       return absl::InvalidArgumentError(
@@ -99,6 +82,28 @@ absl::Status GlobalTimingModule::Initialize(
   return absl::OkStatus();
 }
 
+}  // namespace
+
+std::unique_ptr<GlobalTimingModule> GlobalTimingModule::Create(
+    const absl::flat_hash_map<DecodedUleb128, AudioElementWithData>&
+        audio_elements,
+    const absl::flat_hash_map<DecodedUleb128, const ParamDefinition*>&
+        param_definitions) {
+  absl::flat_hash_map<DecodedUleb128, TimingData> audio_frame_timing_data;
+  absl::flat_hash_map<DecodedUleb128, TimingData> parameter_block_timing_data;
+  const auto status =
+      InitializeInternal(audio_elements, param_definitions,
+                         audio_frame_timing_data, parameter_block_timing_data);
+  if (!status.ok()) {
+    LOG(ERROR) << status;
+    return nullptr;
+  }
+
+  return absl::WrapUnique(
+      new GlobalTimingModule(std::move(audio_frame_timing_data),
+                             std::move(parameter_block_timing_data)));
+}
+
 absl::Status GlobalTimingModule::GetNextAudioFrameTimestamps(
     const DecodedUleb128 audio_substream_id, const uint32_t duration,
     InternalTimestamp& start_timestamp, InternalTimestamp& end_timestamp) {
@@ -141,4 +146,27 @@ absl::Status GlobalTimingModule::GetGlobalAudioFrameTimestamp(
   return absl::OkStatus();
 }
 
+absl::Status GlobalTimingModule::GetTimestampsForId(
+    const DecodedUleb128 id, const uint32_t duration,
+    absl::flat_hash_map<DecodedUleb128, TimingData>& id_to_timing_data,
+    InternalTimestamp& start_timestamp, InternalTimestamp& end_timestamp) {
+  auto timing_data_iter = id_to_timing_data.find(id);
+  if (timing_data_iter == id_to_timing_data.end()) {
+    // This allows generating timing information when
+    // `IGNORE_ERRORS_USE_ONLY_FOR_IAMF_TEST_SUITE` is defined.
+    // TODO(b/278865608): Find better solutions to generate negative test
+    //                    vectors.
+    start_timestamp = 0;
+    end_timestamp = duration;
+    return absl::InvalidArgumentError(
+        absl::StrCat("Timestamps for ID: ", id, " not found"));
+  }
+
+  auto& timing_data = timing_data_iter->second;
+  start_timestamp = timing_data.timestamp;
+  end_timestamp = start_timestamp + duration;
+  timing_data.timestamp += duration;
+  return absl::OkStatus();
+}
+
 }  // namespace iamf_tools

iamf/cli/global_timing_module.h

@@ -14,7 +14,9 @@
 #define CLI_GLOBAL_TIMING_MODULE_H_
 
 #include <cstdint>
+#include <memory>
 #include <optional>
+#include <utility>
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/status/status.h"
@@ -26,21 +28,14 @@ namespace iamf_tools {
 
 class GlobalTimingModule {
  public:
-  /*!\brief Constructor.
-   */
-  GlobalTimingModule() = default;
-
-  /*!\brief Initializes a Global Timing Module.
-   *
-   * Must be called before calling `GetNextAudioFrameTimestamps()` and
-   * `GetNextParameterBlockTimestamps()`.
+  /*!\brief Creates a Global Timing Module.
    *
    * \param audio_elements Audio Element OBUs with data to search for sample
    *        rates.
    * \param param_definitions Parameter definitions keyed by parameter IDs.
-   * \return `absl::OkStatus()` on success. A specific status on failure.
+   * \return `GlobalTimingModule` on success. A specific status on failure.
    */
-  absl::Status Initialize(
+  static std::unique_ptr<GlobalTimingModule> Create(
       const absl::flat_hash_map<DecodedUleb128, AudioElementWithData>&
           audio_elements,
       const absl::flat_hash_map<DecodedUleb128, const ParamDefinition*>&
@@ -97,6 +92,22 @@ class GlobalTimingModule {
     InternalTimestamp timestamp;
   };
 
+  /*!\brief Constructor.
+   *
+   * Used only by `Create()`.
+   *
+   * \param audio_frame_timing_data Timing data for Audio Frames keyed by
+   *        substream ID.
+   * \param parameter_block_timing_data Timing data for Parameter Blocks keyed
+   *        by parameter ID.
+   */
+  GlobalTimingModule(
+      absl::flat_hash_map<DecodedUleb128, TimingData>&& audio_frame_timing_data,
+      absl::flat_hash_map<DecodedUleb128, TimingData>&&
+          parameter_block_timing_data)
+      : audio_frame_timing_data_(std::move(audio_frame_timing_data)),
+        parameter_block_timing_data_(std::move(parameter_block_timing_data)) {}
+
   absl::Status GetTimestampsForId(
       DecodedUleb128 id, uint32_t duration,
       absl::flat_hash_map<DecodedUleb128, TimingData>& id_to_timing_data,

iamf/cli/iamf_encoder.cc

@@ -141,9 +141,12 @@ absl::StatusOr<IamfEncoder> IamfEncoder::Create(
       audio_elements, mix_presentation_obus, param_definitions));
 
   // Initialize the global timing module.
-  auto global_timing_module = std::make_unique<GlobalTimingModule>();
-  RETURN_IF_NOT_OK(
-      global_timing_module->Initialize(audio_elements, param_definitions));
+  auto global_timing_module =
+      GlobalTimingModule::Create(audio_elements, param_definitions);
+  if (global_timing_module == nullptr) {
+    return absl::InvalidArgumentError(
+        "Failed to initialize the global timing module");
+  }
 
   // Initialize the parameter block generator.
   auto parameter_id_to_metadata = std::make_unique<

iamf/cli/obu_processor.cc

@@ -291,8 +291,12 @@ absl::Status ObuProcessor::InitializeInternal(bool is_exhaustive_and_exact,
       }
     }
   }
-  RETURN_IF_NOT_OK(
-      global_timing_module_.Initialize(audio_elements_, param_definitions_));
+  global_timing_module_ =
+      GlobalTimingModule::Create(audio_elements_, param_definitions_);
+  if (global_timing_module_ == nullptr) {
+    return absl::InvalidArgumentError(
+        "Failed to initialize the global timing module");
+  }
   parameters_manager_.emplace(audio_elements_);
   RETURN_IF_NOT_OK(parameters_manager_->Initialize());
   return absl::OkStatus();
@@ -720,6 +724,11 @@ absl::Status ObuProcessor::ProcessTemporalUnitObu(
         "Parameters manager is not constructed; "
         "remember to call `Initialize()` first.");
   }
+  if (global_timing_module_ == nullptr) {
+    return absl::InvalidArgumentError(
+        "Global timing module is not constructed; "
+        "remember to call `Initialize()` first.");
+  }
   if (read_bit_buffer_ == nullptr) {
     return absl::InvalidArgumentError(
         "Read bit buffer is not constructed; "
@@ -729,7 +738,7 @@ absl::Status ObuProcessor::ProcessTemporalUnitObu(
   return ObuProcessor::ProcessTemporalUnitObu(
       audio_elements_, codec_config_obus_, substream_id_to_audio_element_,
       *parameters_manager_, parameter_id_to_metadata_, *read_bit_buffer_,
-      global_timing_module_, output_audio_frame_with_data,
+      *global_timing_module_, output_audio_frame_with_data,
       output_parameter_block_with_data, output_temporal_delimiter,
       continue_processing);
 }

iamf/cli/obu_processor.h

@@ -21,7 +21,6 @@
 
 #include "absl/container/flat_hash_map.h"
 #include "absl/status/status.h"
-#include "absl/strings/string_view.h"
 #include "absl/types/span.h"
 #include "iamf/cli/audio_element_with_data.h"
 #include "iamf/cli/audio_frame_decoder.h"
@@ -337,7 +336,7 @@ class ObuProcessor {
       parameter_id_to_metadata_;
   absl::flat_hash_map<DecodedUleb128, const AudioElementWithData*>
       substream_id_to_audio_element_;
-  GlobalTimingModule global_timing_module_;
+  std::unique_ptr<GlobalTimingModule> global_timing_module_;
   std::optional<ParametersManager> parameters_manager_;
   ReadBitBuffer* read_bit_buffer_;