Cherry pick PR #1385: [android] Enable filtered partial audio tests (#…

…1472) Refer to the original PR: #1385 The cl also fixed a multi thread issue of AudioFrameDiscarder and enhanced timestamp checks of audio decoder outputs. b/274021285 b/289281412 b/292409536 b/298072842 Co-authored-by: Jason <[email protected]>
youtube · Sep 2, 2023 · aa1f8c8 · aa1f8c8
1 parent 77532e3
commit aa1f8c8
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 29 deletions.
diff --git a/starboard/android/shared/test_filters.py b/starboard/android/shared/test_filters.py
@@ -86,15 +86,9 @@
         # TODO: Filter this test on a per-device basis.
         'SbMediaCanPlayMimeAndKeySystem.MinimumSupport',
 
-        # TODO: b/289281412 Make this test work on lab devices consistently.
-        'SbPlayerWriteSampleTests/SbPlayerWriteSampleTest.PartialAudio/*',
-
         # TODO: b/292319097 Make this test work on lab devices consistently.
         'SbPlayerTest.MaxVideoCapabilities',
 
-        # TODO: b/292409536 Make this test fork on lab devices consistently.
-        'SbPlayerWriteSampleTests/SbPlayerWriteSampleTest.PartialAudioDiscardAll/*',
-
         # TODO: b/280432564 Make this test work on lab devices consistently.
         'SbAudioSinkTest.ContinuousAppend',
     ],

diff --git a/starboard/shared/starboard/player/filter/audio_frame_discarder.cc b/starboard/shared/starboard/player/filter/audio_frame_discarder.cc
@@ -23,13 +23,7 @@ namespace player {
 namespace filter {
 
 void AudioFrameDiscarder::OnInputBuffers(const InputBuffers& input_buffers) {
-  if (input_buffer_infos_.size() >= kMaxNumberOfPendingInputBufferInfos) {
-    // This shouldn't happen as it's DCHECKed at the end of this function. Add
-    // an extra check here to ensure that |input_buffer_infos_| won't grow
-    // without bound, which can lead to OOM in production.
-    return;
-  }
-
+  ScopedLock lock(mutex_);
   for (auto&& input_buffer : input_buffers) {
     SB_DCHECK(input_buffer);
     SB_DCHECK(input_buffer->sample_type() == kSbMediaTypeAudio);
@@ -41,6 +35,8 @@ void AudioFrameDiscarder::OnInputBuffers(const InputBuffers& input_buffers) {
     });
   }
 
+  // Add a DCheck here to ensure that |input_buffer_infos_| won't grow
+  // without bound, which can lead to OOM.
   SB_DCHECK(input_buffer_infos_.size() < kMaxNumberOfPendingInputBufferInfos);
 }
 
@@ -49,32 +45,46 @@ void AudioFrameDiscarder::AdjustForDiscardedDurations(
     scoped_refptr<DecodedAudio>* decoded_audio) {
   SB_DCHECK(decoded_audio);
   SB_DCHECK(*decoded_audio);
-  // TODO: Comment out the SB_DCHECK due to b/274021285. We can re-enable it
-  // after b/274021285 is resolved.
-  // SB_DCHECK(!input_buffer_infos_.empty());
 
-  if (input_buffer_infos_.empty()) {
-    SB_LOG(WARNING) << "Inconsistent number of audio decoder outputs. Received "
-                       "outputs when input buffer list is empty.";
-    return;
+  InputBufferInfo input_info;
+  {
+    ScopedLock lock(mutex_);
+    SB_DCHECK(!input_buffer_infos_.empty());
+
+    if (input_buffer_infos_.empty()) {
+      SB_LOG(WARNING)
+          << "Inconsistent number of audio decoder outputs. Received "
+             "outputs when input buffer list is empty.";
+      return;
+    }
+
+    input_info = input_buffer_infos_.front();
+    input_buffer_infos_.pop();
   }
 
-  auto info = input_buffer_infos_.front();
-  SB_LOG_IF(WARNING, info.timestamp != (*decoded_audio)->timestamp())
-      << "Inconsistent timestamps between InputBuffer (@" << info.timestamp
-      << ") and DecodedAudio (@" << (*decoded_audio)->timestamp() << ").";
-  input_buffer_infos_.pop();
+  // We accept a small offset due to the precision of computation. If the
+  // outputs have different timestamps than inputs, discarded durations will be
+  // ignored.
+  const SbTimeMonotonic kTimestampOffset = 10;
+  if (std::abs(input_info.timestamp - (*decoded_audio)->timestamp()) >
+      kTimestampOffset) {
+    SB_LOG(WARNING) << "Inconsistent timestamps between InputBuffer (@"
+                    << input_info.timestamp << ") and DecodedAudio (@"
+                    << (*decoded_audio)->timestamp() << ").";
+    return;
+  }
 
   (*decoded_audio)
       ->AdjustForDiscardedDurations(sample_rate,
-                                    info.discarded_duration_from_front,
-                                    info.discarded_duration_from_back);
+                                    input_info.discarded_duration_from_front,
+                                    input_info.discarded_duration_from_back);
   // `(*decoded_audio)->frames()` might be 0 here.  We don't set it to nullptr
   // in this case so the DecodedAudio instance is always valid (but might be
   // empty).
 }
 
 void AudioFrameDiscarder::OnDecodedAudioEndOfStream() {
+  ScopedLock lock(mutex_);
   // |input_buffer_infos_| can have extra elements when the decoder skip outputs
   // due to errors (like invalid inputs).
   SB_LOG_IF(INFO, !input_buffer_infos_.empty())
@@ -83,6 +93,7 @@ void AudioFrameDiscarder::OnDecodedAudioEndOfStream() {
 }
 
 void AudioFrameDiscarder::Reset() {
+  ScopedLock lock(mutex_);
   input_buffer_infos_ = std::queue<InputBufferInfo>();
 }
 

diff --git a/starboard/shared/starboard/player/filter/audio_frame_discarder.h b/starboard/shared/starboard/player/filter/audio_frame_discarder.h
@@ -17,6 +17,7 @@
 
 #include <queue>
 
+#include "starboard/common/mutex.h"
 #include "starboard/common/ref_counted.h"
 #include "starboard/shared/internal_only.h"
 #include "starboard/shared/starboard/player/decoded_audio_internal.h"
@@ -35,8 +36,6 @@ namespace filter {
 // corresponding InputBuffer object isn't available at the time.
 // This class assumes that there is exact one DecodedAudio object produced for
 // one InputBuffer object, which may not always be the case.
-// TODO(b/274021285): Ensure that the class works when there isn't a 1:1
-//                    relationship between DecodedAudio and InputBuffer.
 class AudioFrameDiscarder {
  public:
   void OnInputBuffers(const InputBuffers& input_buffers);
@@ -55,6 +54,7 @@ class AudioFrameDiscarder {
 
   static constexpr size_t kMaxNumberOfPendingInputBufferInfos = 128;
 
+  Mutex mutex_;
   std::queue<InputBufferInfo> input_buffer_infos_;
 };
 

diff --git a/starboard/shared/starboard/player/filter/testing/audio_decoder_test.cc b/starboard/shared/starboard/player/filter/testing/audio_decoder_test.cc
@@ -185,6 +185,7 @@ class AudioDecoderTest
 
     last_input_buffer_ = GetAudioInputBuffer(index);
     audio_decoder_->Decode({last_input_buffer_}, consumed_cb());
+    written_inputs_.push_back(last_input_buffer_);
   }
 
   void WriteSingleInput(size_t index,
@@ -200,6 +201,7 @@ class AudioDecoderTest
     last_input_buffer_ = GetAudioInputBuffer(
         index, discarded_duration_from_front, discarded_duration_from_back);
     audio_decoder_->Decode({last_input_buffer_}, consumed_cb());
+    written_inputs_.push_back(last_input_buffer_);
   }
 
   // This has to be called when OnOutput() is called.
@@ -228,6 +230,11 @@ class AudioDecoderTest
       ASSERT_LT(decoded_audios_.back()->timestamp(),
                 local_decoded_audio->timestamp());
     }
+    if (!using_stub_decoder_ && invalid_inputs_.empty()) {
+      ASSERT_NEAR(local_decoded_audio->timestamp(),
+                  written_inputs_.front()->timestamp(), 5);
+      written_inputs_.pop_front();
+    }
     decoded_audios_.push_back(local_decoded_audio);
     *decoded_audio = local_decoded_audio;
   }
@@ -428,6 +435,7 @@ class AudioDecoderTest
 
   bool can_accept_more_input_ = true;
   scoped_refptr<InputBuffer> last_input_buffer_;
+  std::deque<scoped_refptr<InputBuffer>> written_inputs_;
   std::vector<scoped_refptr<DecodedAudio>> decoded_audios_;
 
   bool eos_written_ = false;