Start the VDA device poll thread in Initialize()

content/common/gpu/media/v4l2_video_decode_accelerator.cc

Index: content/common/gpu/media/v4l2_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/v4l2_video_decode_accelerator.cc b/content/common/gpu/media/v4l2_video_decode_accelerator.cc
index 68baaa85c845c91dda916e544ddd9e15e27431a3..bbb51d4ea990ae2f6b6480a1494895f54ddc7125 100644
--- a/content/common/gpu/media/v4l2_video_decode_accelerator.cc
+++ b/content/common/gpu/media/v4l2_video_decode_accelerator.cc
@@ -282,6 +282,11 @@ bool V4L2VideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile,
     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);
@@ -709,7 +714,6 @@ bool V4L2VideoDecodeAccelerator::DecodeBufferInitial(
   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.
 
@@ -755,10 +759,6 @@ bool V4L2VideoDecodeAccelerator::DecodeBufferInitial(
     *endpos = size;
   }
 
-  // StartDevicePoll will raise the error if there is one.
-  if (!StartDevicePoll())
-    return false;
-
   decoder_state_ = kDecoding;
   ScheduleDecodeBufferTaskIfNeeded();
   return true;
@@ -885,8 +885,6 @@ 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) {
@@ -1284,11 +1282,15 @@ void V4L2VideoDecodeAccelerator::NotifyFlushDoneIfNeeded() {
   // 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;
@@ -1322,8 +1324,10 @@ void V4L2VideoDecodeAccelerator::ResetTask() {
 
   // 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())
@@ -1353,6 +1357,11 @@ void V4L2VideoDecodeAccelerator::ResetDoneTask() {
     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
@@ -1407,13 +1416,13 @@ void V4L2VideoDecodeAccelerator::DestroyTask() {
 
 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(
@@ -1432,13 +1441,11 @@ bool V4L2VideoDecodeAccelerator::StopDevicePoll(bool keep_input_state) {
   // 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;
   }
 
@@ -1495,7 +1502,8 @@ bool V4L2VideoDecodeAccelerator::StopDevicePoll(bool keep_input_state) {
 
 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;
@@ -1503,8 +1511,10 @@ void V4L2VideoDecodeAccelerator::StartResolutionChangeIfNeeded() {
   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_);
@@ -1550,8 +1560,10 @@ void V4L2VideoDecodeAccelerator::FinishResolutionChange() {
     return;
   }
 
-  if (!StartDevicePoll())
+  if (!StartDevicePoll()) {
+    NOTIFY_ERROR(PLATFORM_FAILURE);
     return;
+  }
 
   Enqueue();
   ScheduleDecodeBufferTaskIfNeeded();