media: Improve assertions of audio encoders and WebmMuxer and allow manual flushing

- Both AudioEncoders and WebmMuxer were using ThreadCheckers
  to assert operations being called on a valid thread. However,
  there's no need to enforce all operations happening on the
  exact same thread. What we care about is the sequence of
  operations, therefore a SequenceChecker is good enough.
- Add mechanisms to flush any buffered frames in the encoders
  or muxer to avoid dropping frames, and also enable manual
  flushing rather than having to destroy the object to have it
  flush its buffers.

BUG=1126586
TEST=Modified a test in media_unittests --gtest_filter=*AudioEncodersTest*

Change-Id: Ife44c440c48408553dacdd92fd8800cdc5c7a233
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490884Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820432}

media/audio/audio_encoders_unittest.cc

@@ -82,7 +82,7 @@ class AudioEncodersTest : public ::testing::TestWithParam<TestAudioParams> {
   ~AudioEncodersTest() override = default;
 
   const AudioParameters& input_params() const { return input_params_; }
-  const AudioEncoder* encoder() const { return encoder_.get(); }
+  AudioEncoder* encoder() const { return encoder_.get(); }
   int encode_callback_count() const { return encode_callback_count_; }
 
   void SetEncoder(std::unique_ptr<AudioEncoder> encoder) {
@@ -200,6 +200,14 @@ TEST_P(AudioEncodersTest, OpusEncoder) {
 
   EXPECT_EQ(1, encode_callback_count());
 
+  // If there are remaining frames in the opus encoder FIFO, we need to flush
+  // them before we destroy the encoder. Flushing should trigger the encode
+  // callback and we should be able to decode the resulting encoded frames.
+  if (total_frames > frames_in_60_ms) {
+    encoder()->Flush();
+    EXPECT_EQ(2, encode_callback_count());
+  }
+
   opus_decoder_destroy(opus_decoder);
   opus_decoder = nullptr;
 }

media/audio/audio_opus_encoder.cc

@@ -207,7 +207,10 @@ AudioOpusEncoder::AudioOpusEncoder(const AudioParameters& input_params,
       converted_params_.frames_per_buffer()));
 }
 
-AudioOpusEncoder::~AudioOpusEncoder() = default;
+AudioOpusEncoder::~AudioOpusEncoder() {
+  DCHECK_EQ(fifo_.queued_frames(), 0)
+      << "Must flush the encoder before destroying to avoid dropping frames.";
+}
 
 void AudioOpusEncoder::EncodeAudioImpl(const AudioBus& audio_bus,
                                        base::TimeTicks capture_time) {
@@ -220,6 +223,20 @@ void AudioOpusEncoder::EncodeAudioImpl(const AudioBus& audio_bus,
   fifo_.Push(audio_bus);
 }
 
+void AudioOpusEncoder::FlushImpl() {
+  // Initializing the opus encoder may have failed.
+  if (!opus_encoder_)
+    return;
+
+  // This is needed to correctly compute the timestamp, since the number of
+  // frames of |output_bus| provided to OnFifoOutput() will always be equal to
+  // the full frames_per_buffer(), as the fifo's Flush() will pad the remaining
+  // empty frames with zeros.
+  number_of_flushed_frames_ = fifo_.queued_frames();
+  fifo_.Flush();
+  number_of_flushed_frames_ = base::nullopt;
+}
+
 void AudioOpusEncoder::OnFifoOutput(const AudioBus& output_bus,
                                     int frame_delay) {
   // Provides input to the converter from |output_bus| within this scope only.
@@ -236,7 +253,9 @@ void AudioOpusEncoder::OnFifoOutput(const AudioBus& output_bus,
     DCHECK_GT(encoded_data_size, 1u);
     encode_callback().Run(EncodedAudioBuffer(
         converted_params_, std::move(encoded_data), encoded_data_size,
-        ComputeTimestamp(output_bus.frames(), last_capture_time())));
+        ComputeTimestamp(
+            number_of_flushed_frames_.value_or(output_bus.frames()),
+            last_capture_time())));
   }
 }
 

media/audio/audio_opus_encoder.h

@@ -8,6 +8,7 @@
 #include <memory>
 #include <vector>
 
+#include "base/optional.h"
 #include "media/base/audio_bus.h"
 #include "media/base/audio_converter.h"
 #include "media/base/audio_encoder.h"
@@ -36,6 +37,7 @@ class MEDIA_EXPORT AudioOpusEncoder : public AudioEncoder {
   // AudioEncoder:
   void EncodeAudioImpl(const AudioBus& audio_bus,
                        base::TimeTicks capture_time) override;
+  void FlushImpl() override;
 
  private:
   // Called synchronously by |fifo_| once enough audio frames have been
@@ -70,6 +72,11 @@ class MEDIA_EXPORT AudioOpusEncoder : public AudioEncoder {
   // The actual libopus encoder instance. This is nullptr if creating the
   // encoder fails.
   OwnedOpusEncoder opus_encoder_;
+
+  // If FlushImpl() was called while |fifo_| has some frames but not full yet,
+  // this will be the number of flushed frames, which is used to compute the
+  // timestamp provided in the output |EncodedAudioBuffer|.
+  base::Optional<int> number_of_flushed_frames_;
 };
 
 }  // namespace media

media/audio/audio_pcm_encoder.cc

@@ -27,4 +27,8 @@ void AudioPcmEncoder::EncodeAudioImpl(const AudioBus& audio_bus,
                          ComputeTimestamp(audio_bus.frames(), capture_time)));
 }
 
+void AudioPcmEncoder::FlushImpl() {
+  // No buffering is done here, so do nothing.
+}
+
 }  // namespace media

media/audio/audio_pcm_encoder.h

@@ -24,6 +24,7 @@ class MEDIA_EXPORT AudioPcmEncoder : public AudioEncoder {
   // AudioEncoder:
   void EncodeAudioImpl(const AudioBus& audio_bus,
                        base::TimeTicks capture_time) override;
+  void FlushImpl() override;
 };
 
 }  // namespace media

media/base/audio_encoder.cc

@@ -38,14 +38,16 @@ AudioEncoder::AudioEncoder(const AudioParameters& input_params,
   DCHECK(audio_input_params_.IsValid());
   DCHECK(!encode_callback_.is_null());
   DCHECK(!status_callback_.is_null());
-  DETACH_FROM_THREAD(thread_checker_);
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
-AudioEncoder::~AudioEncoder() = default;
+AudioEncoder::~AudioEncoder() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 void AudioEncoder::EncodeAudio(const AudioBus& audio_bus,
                                base::TimeTicks capture_time) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_EQ(audio_bus.channels(), audio_input_params_.channels());
   DCHECK(!capture_time.is_null());
 
@@ -61,6 +63,12 @@ void AudioEncoder::EncodeAudio(const AudioBus& audio_bus,
   EncodeAudioImpl(audio_bus, capture_time);
 }
 
+void AudioEncoder::Flush() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  FlushImpl();
+}
+
 base::TimeTicks AudioEncoder::ComputeTimestamp(
     int num_frames,
     base::TimeTicks capture_time) const {

media/base/audio_encoder.h

@@ -8,7 +8,7 @@
 #include <memory>
 
 #include "base/callback.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 #include "base/time/time.h"
 #include "media/base/audio_bus.h"
 #include "media/base/audio_parameters.h"
@@ -41,7 +41,8 @@ struct MEDIA_EXPORT EncodedAudioBuffer {
   // number of encoded bytes may not be known in advance.
   const size_t encoded_data_size;
 
-  // The capture time of the first sample of the current AudioBus.
+  // The capture time of the first sample of the current AudioBus, or a previous
+  // AudioBus If this output was generated because of a call to Flush().
   const base::TimeTicks timestamp;
 };
 
@@ -50,9 +51,9 @@ struct MEDIA_EXPORT EncodedAudioBuffer {
 class MEDIA_EXPORT AudioEncoder {
  public:
   // Signature of the callback invoked to provide the encoded audio data. It is
-  // invoked on the same thread on which EncodeAudio() is called. The utility
+  // invoked on the same sequence on which EncodeAudio() is called. The utility
   // media::BindToCurrentLoop() can be used to create a callback that will be
-  // invoked on the same thread it is constructed on.
+  // invoked on the same sequence it is constructed on.
   using EncodeCB = base::RepeatingCallback<void(EncodedAudioBuffer output)>;
 
   // Signature of the callback to report errors.
@@ -62,8 +63,8 @@ class MEDIA_EXPORT AudioEncoder {
   // encoder, and a callback to trigger to provide the encoded audio data.
   // |input_params| must be valid, and |encode_callback| and |status_callback|
   // must not be null callbacks. All calls to EncodeAudio() must happen on the
-  // same thread (usually an encoder thread), but the encoder itself can be
-  // constructed on any thread.
+  // same sequence (usually an encoder blocking pool sequence), but the encoder
+  // itself can be constructed on any sequence.
   AudioEncoder(const AudioParameters& input_params,
                EncodeCB encode_callback,
                StatusCB status_callback);
@@ -79,6 +80,11 @@ class MEDIA_EXPORT AudioEncoder {
   // actual encoding.
   void EncodeAudio(const AudioBus& audio_bus, base::TimeTicks capture_time);
 
+  // Some encoders may choose to buffer audio frames before they encode them.
+  // This function provides a mechanism to drain and encode any buffered frames
+  // (if any). Must be called on the encoder sequence.
+  void Flush();
+
  protected:
   const EncodeCB& encode_callback() const { return encode_callback_; }
   const StatusCB& status_callback() const { return status_callback_; }
@@ -87,6 +93,8 @@ class MEDIA_EXPORT AudioEncoder {
   virtual void EncodeAudioImpl(const AudioBus& audio_bus,
                                base::TimeTicks capture_time) = 0;
 
+  virtual void FlushImpl() = 0;
+
   // Computes the timestamp of an AudioBus which has |num_frames| and was
   // captured at |capture_time|. This timestamp is the capture time of the first
   // sample in that AudioBus.
@@ -104,7 +112,7 @@ class MEDIA_EXPORT AudioEncoder {
   // EncodeAudio().
   base::TimeTicks last_capture_time_;
 
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 }  // namespace media

media/base/audio_push_fifo.h

@@ -42,6 +42,9 @@ class MEDIA_EXPORT AudioPushFifo final {
   // OutputCallback.
   int frames_per_buffer() const { return frames_per_buffer_; }
 
+  // The number of frames currently queued in this FIFO.
+  int queued_frames() const { return queued_frames_; }
+
   // Must be called at least once before the first call to Push().  May be
   // called later (e.g., to support an audio format change).
   void Reset(int frames_per_buffer);

media/muxers/webm_muxer.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/sequence_checker.h"
 #include "media/base/audio_parameters.h"
 #include "media/base/limits.h"
 #include "media/base/video_frame.h"
@@ -182,16 +183,13 @@ WebmMuxer::WebmMuxer(AudioCodec audio_codec,
   info->set_writing_app("Chrome");
   info->set_muxing_app("Chrome");
 
-  // Creation is done on a different thread than main activities.
-  thread_checker_.DetachFromThread();
+  // Creation can be done on a different sequence than main activities.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 WebmMuxer::~WebmMuxer() {
-  // No need to segment_.Finalize() since is not Seekable(), i.e. a live
-  // stream, but is a good practice.
-  DCHECK(thread_checker_.CalledOnValidThread());
-  FlushQueues();
-  segment_.Finalize();
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  Flush();
 }
 
 bool WebmMuxer::OnEncodedVideo(const VideoParameters& params,
@@ -200,7 +198,7 @@ bool WebmMuxer::OnEncodedVideo(const VideoParameters& params,
                                base::TimeTicks timestamp,
                                bool is_key_frame) {
   DVLOG(1) << __func__ << " - " << encoded_data.size() << "B";
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(params.codec == kCodecVP8 || params.codec == kCodecVP9 ||
          params.codec == kCodecH264)
       << " Unsupported video codec: " << GetCodecName(params.codec);
@@ -244,7 +242,7 @@ bool WebmMuxer::OnEncodedAudio(const media::AudioParameters& params,
                                std::string encoded_data,
                                base::TimeTicks timestamp) {
   DVLOG(2) << __func__ << " - " << encoded_data.size() << "B";
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (!audio_track_index_) {
     AddAudioTrack(params);
@@ -266,25 +264,33 @@ bool WebmMuxer::OnEncodedAudio(const media::AudioParameters& params,
 
 void WebmMuxer::Pause() {
   DVLOG(1) << __func__;
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!elapsed_time_in_pause_)
     elapsed_time_in_pause_.reset(new base::ElapsedTimer());
 }
 
 void WebmMuxer::Resume() {
   DVLOG(1) << __func__;
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (elapsed_time_in_pause_) {
     total_time_in_pause_ += elapsed_time_in_pause_->Elapsed();
     elapsed_time_in_pause_.reset();
   }
 }
 
+bool WebmMuxer::Flush() {
+  // No need to segment_.Finalize() since is not Seekable(), i.e. a live
+  // stream, but is a good practice.
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  FlushQueues();
+  return segment_.Finalize();
+}
+
 void WebmMuxer::AddVideoTrack(
     const gfx::Size& frame_size,
     double frame_rate,
     const base::Optional<gfx::ColorSpace>& color_space) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_EQ(0u, video_track_index_)
       << "WebmMuxer can only be initialized once.";
 
@@ -329,7 +335,7 @@ void WebmMuxer::AddVideoTrack(
 
 void WebmMuxer::AddAudioTrack(const media::AudioParameters& params) {
   DVLOG(1) << __func__ << " " << params.AsHumanReadableString();
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK_EQ(0u, audio_track_index_)
       << "WebmMuxer audio can only be initialised once.";
 
@@ -370,7 +376,7 @@ void WebmMuxer::AddAudioTrack(const media::AudioParameters& params) {
 }
 
 mkvmuxer::int32 WebmMuxer::Write(const void* buf, mkvmuxer::uint32 len) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(buf);
   write_data_callback_.Run(
       base::StringPiece(reinterpret_cast<const char*>(buf), len));
@@ -399,14 +405,14 @@ void WebmMuxer::ElementStartNotify(mkvmuxer::uint64 element_id,
 }
 
 void WebmMuxer::FlushQueues() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   while ((!video_frames_.empty() || !audio_frames_.empty()) &&
          FlushNextFrame()) {
   }
 }
 
 bool WebmMuxer::PartiallyFlushQueues() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   bool result = true;
   while (!(has_video_ && video_frames_.empty()) &&
          !(has_audio_ && audio_frames_.empty()) && result) {
@@ -416,7 +422,7 @@ bool WebmMuxer::PartiallyFlushQueues() {
 }
 
 bool WebmMuxer::FlushNextFrame() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   base::TimeDelta min_timestamp = base::TimeDelta::Max();
   base::circular_deque<EncodedFrame>* queue = &video_frames_;
   uint8_t track_index = video_track_index_;

media/muxers/webm_muxer.h

@@ -13,8 +13,8 @@
 #include "base/containers/circular_deque.h"
 #include "base/macros.h"
 #include "base/numerics/safe_math.h"
+#include "base/sequence_checker.h"
 #include "base/strings/string_piece.h"
-#include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "base/timer/elapsed_timer.h"
 #include "media/base/audio_codecs.h"
@@ -89,6 +89,10 @@ class MEDIA_EXPORT WebmMuxer : public mkvmuxer::IMkvWriter {
   void Pause();
   void Resume();
 
+  // Drains and writes out all buffered frames and finalizes the segment.
+  // Returns true on success, false otherwise.
+  bool Flush();
+
   void ForceOneLibWebmErrorForTesting() { force_one_libwebm_error_ = true; }
 
  private:
@@ -137,9 +141,6 @@ class MEDIA_EXPORT WebmMuxer : public mkvmuxer::IMkvWriter {
       base::TimeTicks timestamp,
       base::TimeTicks* last_timestamp);
 
-  // Used to DCHECK that we are called on the correct thread.
-  base::ThreadChecker thread_checker_;
-
   // Audio codec configured on construction. Video codec is taken from first
   // received frame.
   const AudioCodec audio_codec_;
@@ -192,6 +193,8 @@ class MEDIA_EXPORT WebmMuxer : public mkvmuxer::IMkvWriter {
   // frame appears.
   base::circular_deque<EncodedFrame> video_frames_;
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(WebmMuxer);
 };