Android: Fix random crash in HW encode accelerator

media/gpu/android_video_encode_accelerator.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/gpu/android_video_encode_accelerator.h"
 
 #include <memory>
 #include <set>
 #include <tuple>
 
@@ skipping 29 matching lines @@
   COLOR_FORMAT_YUV420_PLANAR = 19,
   COLOR_FORMAT_YUV420_SEMIPLANAR = 21,
 };
 
 // Helper macros for dealing with failure.  If |result| evaluates false, emit
 // |log| to DLOG(ERROR), register |error| with the client, and return.
 #define RETURN_ON_FAILURE(result, log, error)                  \
   do {                                                         \
     if (!(result)) {                                           \
       DLOG(ERROR) << log;                                      \
       if (client_ptr_factory_->GetWeakPtr()) {                 \

[watk, 2016/12/28 01:01:51]

It's not possible for this to be null AFAICT. Should probably change it to if
(!error_occurred).

[braveyao, 2016/12/28 01:28:18]

Done.

         client_ptr_factory_->GetWeakPtr()->NotifyError(error); \
-        client_ptr_factory_.reset();                           \
       }                                                        \
+      error_occurred_ = true;                                  \
       return;                                                  \
     }                                                          \
   } while (0)
 
 // Because MediaCodec is thread-hostile (must be poked on a single thread) and
 // has no callback mechanism (b/11990118), we must drive it by polling for
 // complete frames (and available input buffers, when the codec is fully
 // saturated).  This function defines the polling delay.  The value used is an
 // arbitrary choice that trades off CPU utilization (spinning) against latency.
 // Mirrors android_video_decode_accelerator.cc::DecodePollDelay().
@@ skipping 23 matching lines @@
     *pixel_format = COLOR_FORMAT_YUV420_SEMIPLANAR;
   else if (formats.count(COLOR_FORMAT_YUV420_PLANAR) > 0)
     *pixel_format = COLOR_FORMAT_YUV420_PLANAR;
   else
     return false;
 
   return true;
 }
 
 AndroidVideoEncodeAccelerator::AndroidVideoEncodeAccelerator()
-    : num_buffers_at_codec_(0), last_set_bitrate_(0) {}
+    : num_buffers_at_codec_(0), last_set_bitrate_(0), error_occurred_(false) {}
 
 AndroidVideoEncodeAccelerator::~AndroidVideoEncodeAccelerator() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 VideoEncodeAccelerator::SupportedProfiles
 AndroidVideoEncodeAccelerator::GetSupportedProfiles() {
   SupportedProfiles profiles;
 
   const struct {
@@ skipping 36 matching lines @@
     VideoCodecProfile output_profile,
     uint32_t initial_bitrate,
     Client* client) {
   DVLOG(3) << __func__ << " format: " << VideoPixelFormatToString(format)
            << ", input_visible_size: " << input_visible_size.ToString()
            << ", output_profile: " << GetProfileName(output_profile)
            << ", initial_bitrate: " << initial_bitrate;
   DCHECK(!media_codec_);
   DCHECK(thread_checker_.CalledOnValidThread());
 
   client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client));

[watk, 2016/12/28 01:01:51]

I'd suggest adding a DCHECK(client) or if (!client) return false; to make it
clear that client can't be null.

[braveyao, 2016/12/28 01:28:18]

Done.

+  error_occurred_ = false;

[watk, 2016/12/28 01:01:51]

Initialize() cannot be called more than once so this is unnecessary.

[braveyao, 2016/12/28 01:28:18]

Done.

 
   if (!(MediaCodecUtil::SupportsSetParameters() &&
         format == PIXEL_FORMAT_I420)) {
     DLOG(ERROR) << "Unexpected combo: " << format << ", " << output_profile;
     return false;
   }
 
   std::string mime_type;
   VideoCodec codec;
   // The client should be prepared to feed at least this many frames into the
@@ skipping 113 matching lines @@
   // Note: Android's MediaCodec doesn't allow mid-stream adjustments to
   // framerate, so we ignore that here.  This is OK because Android only uses
   // the framerate value from MediaFormat during configure() as a proxy for
   // bitrate, and we set that explicitly.
 }
 
 void AndroidVideoEncodeAccelerator::Destroy() {
   DVLOG(3) << __PRETTY_FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   client_ptr_factory_.reset();
+  error_occurred_ = false;

[watk, 2016/12/28 01:01:51]

No need to reset this, we're deleting below. (The .reset() above is also 
unnecessary, but I can see it adding clarity).

[braveyao, 2016/12/28 01:28:18]

Done.

   if (media_codec_) {
     if (io_timer_.IsRunning())
       io_timer_.Stop();
     media_codec_->Stop();
   }
   delete this;
 }
 
 void AndroidVideoEncodeAccelerator::DoIOTask() {
   QueueInput();
   DequeueOutput();
   MaybeStartIOTimer();
   MaybeStopIOTimer();
 }
 
 void AndroidVideoEncodeAccelerator::QueueInput() {
-  if (!client_ptr_factory_->GetWeakPtr() || pending_frames_.empty())
+  if (error_occurred_ || pending_frames_.empty())
     return;
 
   int input_buf_index = 0;
   MediaCodecStatus status =
       media_codec_->DequeueInputBuffer(NoWaitTimeOut(), &input_buf_index);
   if (status != MEDIA_CODEC_OK) {
     DCHECK(status == MEDIA_CODEC_DEQUEUE_INPUT_AGAIN_LATER ||
            status == MEDIA_CODEC_ERROR);
     RETURN_ON_FAILURE(status != MEDIA_CODEC_ERROR, "MediaCodec error",
                       kPlatformFailureError);
@@ skipping 46 matching lines @@
   UMA_HISTOGRAM_TIMES("Media.AVDA.InputQueueTime",
                       base::Time::Now() - std::get<2>(input));
   RETURN_ON_FAILURE(status == MEDIA_CODEC_OK,
                     "Failed to QueueInputBuffer: " << status,
                     kPlatformFailureError);
   ++num_buffers_at_codec_;
   pending_frames_.pop();
 }
 
 void AndroidVideoEncodeAccelerator::DequeueOutput() {
-  if (!client_ptr_factory_->GetWeakPtr() ||
-      available_bitstream_buffers_.empty() || num_buffers_at_codec_ == 0) {
+  if (error_occurred_ || available_bitstream_buffers_.empty() ||
+      num_buffers_at_codec_ == 0) {
     return;
   }
 
   int32_t buf_index = 0;
   size_t offset = 0;
   size_t size = 0;
   bool key_frame = false;
 
   MediaCodecStatus status =
       media_codec_->DequeueOutputBuffer(NoWaitTimeOut(), &buf_index, &offset,
@@ skipping 39 matching lines @@
   --num_buffers_at_codec_;
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(&VideoEncodeAccelerator::Client::BitstreamBufferReady,
                  client_ptr_factory_->GetWeakPtr(), bitstream_buffer.id(), size,
                  key_frame, base::Time::Now() - base::Time()));
 }
 
 }  // namespace media