Start the VDA device poll thread in Initialize()

content/common/gpu/media/v4l2_video_decode_accelerator.cc

 // Copyright 2014 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 <dlfcn.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <libdrm/drm_fourcc.h>
 #include <linux/videodev2.h>
 #include <poll.h>
@@ skipping 264 matching lines @@
   memset(&sub, 0, sizeof(sub));
   sub.type = V4L2_EVENT_RESOLUTION_CHANGE;
   IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_SUBSCRIBE_EVENT, &sub);
 
   // Initialize format-specific bits.
   if (video_profile_ >= media::H264PROFILE_MIN &&
       video_profile_ <= media::H264PROFILE_MAX) {
     decoder_h264_parser_.reset(new media::H264Parser());
   }
 
+  if (!StartDevicePoll()) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return false;
+  }
+

[shivdasp, 2014/03/12 12:12:27]

It seems there is a race condition here. The DevicePollTask() can get scheduled
before this thread creates the decoder_thread_ and access to decoder_thread_ in
DevicePollTask() will seg fault.
StartDevicePoll() if done after decoder_thread_ starts.

[sheu, 2014/03/12 20:51:21]

You're completely correct here.  Thanks.  Fixed.

   if (!decoder_thread_.Start()) {
     DLOG(ERROR) << "Initialize(): decoder thread failed to start";
     NOTIFY_ERROR(PLATFORM_FAILURE);
     return false;
   }
 
   SetDecoderState(kInitialized);
 
   child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
       &Client::NotifyInitializeDone, client_));
@@ skipping 407 matching lines @@
         base::Unretained(this)));
   }
 }
 
 bool V4L2VideoDecodeAccelerator::DecodeBufferInitial(
     const void* data, size_t size, size_t* endpos) {
   DVLOG(3) << "DecodeBufferInitial(): data=" << data << ", size=" << size;
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   DCHECK_NE(decoder_state_, kUninitialized);
   DCHECK_NE(decoder_state_, kDecoding);
-  DCHECK(!device_poll_thread_.IsRunning());
   // Initial decode.  We haven't been able to get output stream format info yet.
   // Get it, and start decoding.
 
   // Copy in and send to HW.
   if (!AppendToInputFrame(data, size))
     return false;
 
   // If we only have a partial frame, don't flush and process yet.
   if (decoder_partial_frame_pending_)
     return true;
@@ skipping 25 matching lines @@
 
     // We expect to process the initial buffer once during stream init to
     // configure stream parameters, but will not consume the steam data on that
     // iteration.  Subsequent iterations (including after reset) do not require
     // the stream init step.
     *endpos = 0;
   } else {
     *endpos = size;
   }
 
-  // StartDevicePoll will raise the error if there is one.
-  if (!StartDevicePoll())
-    return false;
-
   decoder_state_ = kDecoding;
   ScheduleDecodeBufferTaskIfNeeded();
   return true;
 }
 
 bool V4L2VideoDecodeAccelerator::DecodeBufferContinue(
     const void* data, size_t size) {
   DVLOG(3) << "DecodeBufferContinue(): data=" << data << ", size=" << size;
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   DCHECK_EQ(decoder_state_, kDecoding);
@@ skipping 106 matching lines @@
   // Enqueue once since there's new available input for it.
   Enqueue();
 
   return (decoder_state_ != kError);
 }
 
 void V4L2VideoDecodeAccelerator::ServiceDeviceTask(bool event_pending) {
   DVLOG(3) << "ServiceDeviceTask()";
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   DCHECK_NE(decoder_state_, kUninitialized);
-  DCHECK_NE(decoder_state_, kInitialized);
-  DCHECK_NE(decoder_state_, kAfterReset);
   TRACE_EVENT0("Video Decoder", "V4L2VDA::ServiceDeviceTask");
 
   if (decoder_state_ == kResetting) {
     DVLOG(2) << "ServiceDeviceTask(): early out: kResetting state";
     return;
   } else if (decoder_state_ == kError) {
     DVLOG(2) << "ServiceDeviceTask(): early out: kError state";
     return;
   } else if (decoder_state_ == kChangingResolution) {
     DVLOG(2) << "ServiceDeviceTask(): early out: kChangingResolution state";
@@ skipping 377 matching lines @@
 
   // TODO(posciak): crbug.com/270039. Exynos requires a streamoff-streamon
   // sequence after flush to continue, even if we are not resetting. This would
   // make sense, because we don't really want to resume from a non-resume point
   // (e.g. not from an IDR) if we are flushed.
   // MSE player however triggers a Flush() on chunk end, but never Reset(). One
   // could argue either way, or even say that Flush() is not needed/harmful when
   // transitioning to next chunk.
   // For now, do the streamoff-streamon cycle to satisfy Exynos and not freeze
   // when doing MSE. This should be harmless otherwise.
-  if (!StopDevicePoll(false))
+  if (!StopDevicePoll(false)) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
-  if (!StartDevicePoll())
+  if (!StartDevicePoll()) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
   decoder_delay_bitstream_buffer_id_ = -1;
   decoder_flushing_ = false;
   DVLOG(3) << "NotifyFlushDoneIfNeeded(): returning flush";
   child_message_loop_proxy_->PostTask(
       FROM_HERE, base::Bind(&Client::NotifyFlushDone, client_));
 
   // While we were flushing, we early-outed DecodeBufferTask()s.
   ScheduleDecodeBufferTaskIfNeeded();
 }
@@ skipping 13 matching lines @@
   // because input pipe is already stopped if we are changing resolution.
   // We will come back here after we are done with the resolution change.
   DCHECK(!resolution_change_reset_pending_);
   if (resolution_change_pending_ || decoder_state_ == kChangingResolution) {
     resolution_change_reset_pending_ = true;
     return;
   }
 
   // We stop streaming and clear buffer tracking info (not preserving inputs).
   // StopDevicePoll() unconditionally does _not_ destroy buffers, however.
-  if (!StopDevicePoll(false))
+  if (!StopDevicePoll(false)) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
   decoder_current_bitstream_buffer_.reset();
   while (!decoder_input_queue_.empty())
     decoder_input_queue_.pop();
 
   decoder_current_input_buffer_ = -1;
 
   // If we were flushing, we'll never return any more BitstreamBuffers or
   // PictureBuffers; they have all been dropped and returned by now.
   NotifyFlushDoneIfNeeded();
 
   // Mark that we're resetting, then enqueue a ResetDoneTask().  All intervening
   // jobs will early-out in the kResetting state.
   decoder_state_ = kResetting;
   SendPictureReady();  // Send all pending PictureReady.
   decoder_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
       &V4L2VideoDecodeAccelerator::ResetDoneTask, base::Unretained(this)));
 }
 
 void V4L2VideoDecodeAccelerator::ResetDoneTask() {
   DVLOG(3) << "ResetDoneTask()";
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   TRACE_EVENT0("Video Decoder", "V4L2VDA::ResetDoneTask");
 
   if (decoder_state_ == kError) {
     DVLOG(2) << "ResetDoneTask(): early out: kError state";
     return;
   }
 
+  if (!StartDevicePoll()) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
+    return;
+  }
+
   // We might have received a resolution change event while we were waiting
   // for the reset to finish. The codec will not post another event if the
   // resolution after reset remains the same as the one to which were just
   // about to switch, so preserve the event across reset so we can address
   // it after resuming.
 
   // Reset format-specific bits.
   if (video_profile_ >= media::H264PROFILE_MIN &&
       video_profile_ <= media::H264PROFILE_MAX) {
     decoder_h264_parser_.reset(new media::H264Parser());
@@ skipping 34 matching lines @@
   while (!decoder_input_queue_.empty())
     decoder_input_queue_.pop();
   decoder_flushing_ = false;
 
   // Set our state to kError.  Just in case.
   decoder_state_ = kError;
 }
 
 bool V4L2VideoDecodeAccelerator::StartDevicePoll() {
   DVLOG(3) << "StartDevicePoll()";
-  DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   DCHECK(!device_poll_thread_.IsRunning());
+  if (decoder_thread_.IsRunning())
+    DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
 
   // Start up the device poll thread and schedule its first DevicePollTask().
   if (!device_poll_thread_.Start()) {
     DLOG(ERROR) << "StartDevicePoll(): Device thread failed to start";
-    NOTIFY_ERROR(PLATFORM_FAILURE);

[Pawel Osciak, 2014/03/09 02:34:50]

Could you instead just:

if (state != kUninitialized)
  NOTIFY_ERROR()

?

[sheu, 2014/03/10 19:38:42]

Not sure what you mean here.  We should error out also if thread start fails.

[Pawel Osciak, 2014/03/11 00:07:44]

Sorry, should have been more precise. The issue here is that you had to move
NOTIFY_ERROR out of {Start,Stop}DevicePoll because you didn't want to
NOTIFY_ERROR in Initialize().

What I'm suggesting is to keep NOTIFY_ERROR in those methods, but only call it
if state is not kUnintialized (i.e. it's ok to NOTIFY_ERROR).

Or maybe even better, check for it in the actual NOTIFY_ERROR macro?

[sheu, 2014/03/12 20:51:21]

I'd be more inclined to check in the macro, but even then I'm not really
inclined to conditionally do NOTIFY_ERROR.  If a call is made in a location
where state matters, I'd prefer that the caller take care of doing things that
are appropriate for the state, especially since the state is part of decoder
thread state and it would be a whole 'nother can of worms to make that state
safe to query from the child thread as well.

     return false;
   }
   device_poll_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
       &V4L2VideoDecodeAccelerator::DevicePollTask,
       base::Unretained(this),
       0));
 
   return true;
 }
 
 bool V4L2VideoDecodeAccelerator::StopDevicePoll(bool keep_input_state) {
   DVLOG(3) << "StopDevicePoll()";
   if (decoder_thread_.IsRunning())
     DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
 
   // Signal the DevicePollTask() to stop, and stop the device poll thread.
   if (!device_->SetDevicePollInterrupt()) {
     DPLOG(ERROR) << "SetDevicePollInterrupt(): failed";
-    NOTIFY_ERROR(PLATFORM_FAILURE);
     return false;
   }
   device_poll_thread_.Stop();
   // Clear the interrupt now, to be sure.
   if (!device_->ClearDevicePollInterrupt()) {
-    NOTIFY_ERROR(PLATFORM_FAILURE);
     return false;
   }
 
   // Stop streaming.
   if (!keep_input_state) {
     if (input_streamon_) {
       __u32 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
       IOCTL_OR_ERROR_RETURN_FALSE(VIDIOC_STREAMOFF, &type);
     }
     input_streamon_ = false;
@@ skipping 36 matching lines @@
     }
   }
   output_buffer_queued_count_ = 0;
 
   DVLOG(3) << "StopDevicePoll(): device poll stopped";
   return true;
 }
 
 void V4L2VideoDecodeAccelerator::StartResolutionChangeIfNeeded() {
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
-  DCHECK_EQ(decoder_state_, kDecoding);
+  DCHECK_NE(decoder_state_, kUninitialized);
+  DCHECK_NE(decoder_state_, kResetting);
 
   if (!resolution_change_pending_)
     return;
 
   DVLOG(3) << "No more work, initiate resolution change";
 
   // Keep input queue.
-  if (!StopDevicePoll(true))
+  if (!StopDevicePoll(true)) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
   decoder_state_ = kChangingResolution;
   DCHECK(resolution_change_pending_);
   resolution_change_pending_ = false;
 
   // Post a task to clean up buffers on child thread. This will also ensure
   // that we won't accept ReusePictureBuffer() anymore after that.
   child_message_loop_proxy_->PostTask(FROM_HERE, base::Bind(
       &V4L2VideoDecodeAccelerator::ResolutionChangeDestroyBuffers,
       weak_this_));
@@ skipping 25 matching lines @@
   }
 
   decoder_state_ = kDecoding;
 
   if (resolution_change_reset_pending_) {
     resolution_change_reset_pending_ = false;
     ResetTask();
     return;
   }
 
-  if (!StartDevicePoll())
+  if (!StartDevicePoll()) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
   Enqueue();
   ScheduleDecodeBufferTaskIfNeeded();
 }
 
 void V4L2VideoDecodeAccelerator::DevicePollTask(bool poll_device) {
   DVLOG(3) << "DevicePollTask()";
   DCHECK_EQ(device_poll_thread_.message_loop(), base::MessageLoop::current());
   TRACE_EVENT0("Video Decoder", "V4L2VDA::DevicePollTask");
 
@@ skipping 354 matching lines @@
 
 void V4L2VideoDecodeAccelerator::PictureCleared() {
   DVLOG(3) << "PictureCleared(). clearing count=" << picture_clearing_count_;
   DCHECK_EQ(decoder_thread_.message_loop(), base::MessageLoop::current());
   DCHECK_GT(picture_clearing_count_, 0);
   picture_clearing_count_--;
   SendPictureReady();
 }
 
 }  // namespace content