Fix VideoCaptureModule and unit test valgrind errors on linux.

BUG= 302

Review URL: https://webrtc-codereview.appspot.com/717008

git-svn-id: http://webrtc.googlecode.com/svn/trunk@2574 4adac7df-926f-26a2-2b94-8c16560cd09d

src/modules/video_capture/main/source/Linux/video_capture_linux.cc

@@ -282,39 +282,31 @@ WebRtc_Word32 VideoCaptureModuleV4L2::StartCapture(
 
 WebRtc_Word32 VideoCaptureModuleV4L2::StopCapture()
 {
-    if (_captureThread)
+    if (_captureThread) {
-        _captureThread->SetNotAlive();// Make sure the capture thread stop stop using the critsect.
+        // Make sure the capture thread stop stop using the critsect.
-
+        _captureThread->SetNotAlive();
-
+        if (_captureThread->Stop()) {
-    CriticalSectionScoped cs(_captureCritSect);
+            delete _captureThread;
-
+            _captureThread = NULL;
-    WEBRTC_TRACE(webrtc::kTraceInfo, webrtc::kTraceVideoCapture, -1, "StopCapture(), was running: %d",
+        } else
-               _captureStarted);
-
-    if (!_captureStarted)
-    {
-        // we were not capturing!
-        return 0;
-    }
-
-    _captureStarted = false;
-
-    // stop the capture thread
-    // Delete capture update thread and event
-    if (_captureThread)
-    {
-        ThreadWrapper* temp = _captureThread;
-        _captureThread = NULL;
-        temp->SetNotAlive();
-        if (temp->Stop())
         {
-            delete temp;
+            // Couldn't stop the thread, leak instead of crash.
+            WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceVideoCapture, -1,
+                         "%s: could not stop capture thread", __FUNCTION__);
+            assert(!"could not stop capture thread");
         }
     }
 
-    DeAllocateVideoBuffers();
+    CriticalSectionScoped cs(_captureCritSect);
-    close(_deviceFd);
+    if (_captureStarted)
-    _deviceFd = -1;
+    {
+        _captureStarted = false;
+        _captureThread = NULL;
+
+        DeAllocateVideoBuffers();
+        close(_deviceFd);
+        _deviceFd = -1;
+    }
 
     return 0;
 }
@@ -414,12 +406,6 @@ bool VideoCaptureModuleV4L2::CaptureProcess()
     struct timeval timeout;
 
     _captureCritSect->Enter();
-    if (!_captureThread)
-    {
-        // terminating
-        _captureCritSect->Leave();
-        return false;
-    }
 
     FD_ZERO(&rSet);
     FD_SET(_deviceFd, &rSet);

src/modules/video_capture/main/source/video_capture_impl.cc

@@ -48,6 +48,7 @@ WebRtc_Word32 VideoCaptureImpl::ChangeUniqueId(const WebRtc_Word32 id)
 // returns the number of milliseconds until the module want a worker thread to call Process
 WebRtc_Word32 VideoCaptureImpl::TimeUntilNextProcess()
 {
+    CriticalSectionScoped cs(&_callBackCs);
     TickTime timeNow = TickTime::Now();
 
     WebRtc_Word32 timeToNormalProcess = kProcessInterval

src/modules/video_capture/main/test/video_capture_unittest.cc

@@ -10,15 +10,21 @@
 
 #include <stdio.h>
 
-#include "gtest/gtest.h"
-#include "process_thread.h"
-#include "scoped_ptr.h"
-#include "scoped_refptr.h"
-#include "system_wrappers/interface/sleep.h"
-#include "tick_util.h"
-#include "video_capture.h"
-#include "video_capture_factory.h"
 
+
+#include "system_wrappers/interface/critical_section_wrapper.h"
+#include "system_wrappers/interface/scoped_ptr.h"
+#include "system_wrappers/interface/scoped_refptr.h"
+#include "system_wrappers/interface/sleep.h"
+#include "system_wrappers/interface/tick_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "modules/utility/interface/process_thread.h"
+#include "modules/video_capture/main/interface/video_capture.h"
+#include "modules/video_capture/main/interface/video_capture_factory.h"
+
+using webrtc::CriticalSectionWrapper;
+using webrtc::CriticalSectionScoped;
+using webrtc::scoped_ptr;
 using webrtc::SleepMs;
 using webrtc::TickTime;
 using webrtc::VideoCaptureAlarm;
@@ -28,6 +34,7 @@ using webrtc::VideoCaptureFactory;
 using webrtc::VideoCaptureFeedBack;
 using webrtc::VideoCaptureModule;
 
+
 #define WAIT_(ex, timeout, res) \
   do { \
     res = (ex); \
@@ -113,74 +120,134 @@ static bool CompareFrames(const webrtc::VideoFrameI420& frame1,
   return true;
 }
 
+
 class TestVideoCaptureCallback : public VideoCaptureDataCallback {
  public:
   TestVideoCaptureCallback()
-    : capture_delay(0),
+    : capture_cs_(CriticalSectionWrapper::CreateCriticalSection()),
-      last_render_time_ms(0),
+      capture_delay_(0),
-      incoming_frames(0),
+      last_render_time_ms_(0),
-      timing_warnings(0) {
+      incoming_frames_(0),
+      timing_warnings_(0) {
   }
 
   ~TestVideoCaptureCallback() {
-    if (timing_warnings > 0)
+    if (timing_warnings_ > 0)
-      printf("No of timing warnings %d\n", timing_warnings);
+      printf("No of timing warnings %d\n", timing_warnings_);
   }
 
   virtual void OnIncomingCapturedFrame(const WebRtc_Word32 id,
                                        webrtc::VideoFrame& videoFrame,
                                        webrtc::VideoCodecType codecType) {
+    CriticalSectionScoped cs(capture_cs_.get());
+
     int height = static_cast<int>(videoFrame.Height());
     int width = static_cast<int>(videoFrame.Width());
-    EXPECT_EQ(height, capability.height);
+    EXPECT_EQ(height, capability_.height);
-    EXPECT_EQ(width, capability.width);
+    EXPECT_EQ(width, capability_.width);
     // RenderTimstamp should be the time now.
     EXPECT_TRUE(
         videoFrame.RenderTimeMs() >= TickTime::MillisecondTimestamp()-30 &&
         videoFrame.RenderTimeMs() <= TickTime::MillisecondTimestamp());
 
     if ((videoFrame.RenderTimeMs() >
-            last_render_time_ms + (1000 * 1.1) / capability.maxFPS &&
+            last_render_time_ms_ + (1000 * 1.1) / capability_.maxFPS &&
-            last_render_time_ms > 0) ||
+            last_render_time_ms_ > 0) ||
         (videoFrame.RenderTimeMs() <
-            last_render_time_ms + (1000 * 0.9) / capability.maxFPS &&
+            last_render_time_ms_ + (1000 * 0.9) / capability_.maxFPS &&
-            last_render_time_ms > 0)) {
+            last_render_time_ms_ > 0)) {
-      timing_warnings++;
+      timing_warnings_++;
     }
 
-    incoming_frames++;
+    incoming_frames_++;
-    last_render_time_ms = videoFrame.RenderTimeMs();
+    last_render_time_ms_ = videoFrame.RenderTimeMs();
-    last_frame.CopyFrame(videoFrame);
+    last_frame_.CopyFrame(videoFrame);
   }
 
   virtual void OnCaptureDelayChanged(const WebRtc_Word32 id,
                                      const WebRtc_Word32 delay) {
-    capture_delay = delay;
+    CriticalSectionScoped cs(capture_cs_.get());
+    capture_delay_ = delay;
   }
 
-  VideoCaptureCapability capability;
+  void SetExpectedCapability(VideoCaptureCapability capability) {
-  int capture_delay;
+    CriticalSectionScoped cs(capture_cs_.get());
-  WebRtc_Word64 last_render_time_ms;
+    capability_= capability;
-  int incoming_frames;
+    incoming_frames_ = 0;
-  int timing_warnings;
+    last_render_time_ms_ = 0;
-  webrtc::VideoFrame last_frame;
+    capture_delay_ = 0;
+  }
+  int incoming_frames() {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return incoming_frames_;
+  }
+
+  int capture_delay() {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return capture_delay_;
+  }
+  int timing_warnings() {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return timing_warnings_;
+  }
+  VideoCaptureCapability capability() {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return capability_;
+  }
+
+  bool CompareLastFrame(const webrtc::VideoFrame& frame) {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return CompareFrames(last_frame_, frame);
+  }
+
+  bool CompareLastFrame(const webrtc::VideoFrameI420& frame) {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return CompareFrames(frame, last_frame_);
+  }
+
+ private:
+  scoped_ptr<CriticalSectionWrapper> capture_cs_;
+  VideoCaptureCapability capability_;
+  int capture_delay_;
+  WebRtc_Word64 last_render_time_ms_;
+  int incoming_frames_;
+  int timing_warnings_;
+  webrtc::VideoFrame last_frame_;
 };
 
 class TestVideoCaptureFeedBack : public VideoCaptureFeedBack {
  public:
-  TestVideoCaptureFeedBack() : frame_rate(0), alarm(webrtc::Cleared) {}
+  TestVideoCaptureFeedBack() :
+    capture_cs_(CriticalSectionWrapper::CreateCriticalSection()),
+    frame_rate_(0),
+    alarm_(webrtc::Cleared) {
+  }
 
   virtual void OnCaptureFrameRate(const WebRtc_Word32 id,
                                   const WebRtc_UWord32 frameRate) {
-    frame_rate = frameRate;
+    CriticalSectionScoped cs(capture_cs_.get());
+    frame_rate_ = frameRate;
   }
 
   virtual void OnNoPictureAlarm(const WebRtc_Word32 id,
                                 const VideoCaptureAlarm reported_alarm) {
-    alarm = reported_alarm;
+    CriticalSectionScoped cs(capture_cs_.get());
+    alarm_ = reported_alarm;
   }
-  unsigned int frame_rate;
+  int frame_rate() {
-  VideoCaptureAlarm alarm;
+    CriticalSectionScoped cs(capture_cs_.get());
+    return frame_rate_;
+
+  }
+  VideoCaptureAlarm alarm() {
+    CriticalSectionScoped cs(capture_cs_.get());
+    return alarm_;
+  }
+
+ private:
+  scoped_ptr<CriticalSectionWrapper> capture_cs_;
+  unsigned int frame_rate_;
+  VideoCaptureAlarm alarm_;
 };
 
 class VideoCaptureTest : public testing::Test {
@@ -220,10 +287,11 @@ class VideoCaptureTest : public testing::Test {
 
     VideoCaptureCapability resulting_capability;
     EXPECT_EQ(0, capture_module->CaptureSettings(resulting_capability));
-    EXPECT_EQ(capability, resulting_capability);
+    EXPECT_EQ(capability.width, resulting_capability.width);
+    EXPECT_EQ(capability.height, resulting_capability.height);
   }
 
-  webrtc::scoped_ptr<VideoCaptureModule::DeviceInfo> device_info_;
+  scoped_ptr<VideoCaptureModule::DeviceInfo> device_info_;
   unsigned int number_of_devices_;
 };
 
@@ -235,25 +303,25 @@ TEST_F(VideoCaptureTest, CreateDelete) {
         0, &capture_observer));
     ASSERT_TRUE(module.get() != NULL);
 
+    VideoCaptureCapability capability;
 #ifndef WEBRTC_MAC
-    device_info_->GetCapability(module->CurrentDeviceName(), 0,
+    device_info_->GetCapability(module->CurrentDeviceName(), 0, capability);
-                                capture_observer.capability);
 #else
-    capture_observer.capability.width = kTestWidth;
+    capability.width = kTestWidth;
-    capture_observer.capability.height = kTestHeight;
+    capability.height = kTestHeight;
-    capture_observer.capability.maxFPS = kTestFramerate;
+    capability.maxFPS = kTestFramerate;
-    capture_observer.capability.rawType = webrtc::kVideoUnknown;
+    capability.rawType = webrtc::kVideoUnknown;
 #endif
-
+    capture_observer.SetExpectedCapability(capability);
-    StartCapture(module.get(), capture_observer.capability);
+    StartCapture(module.get(), capability);
 
     // Less than 4s to start the camera.
     EXPECT_LE(TickTime::MillisecondTimestamp() - start_time, 4000);
 
     // Make sure 5 frames are captured.
-    EXPECT_TRUE_WAIT(capture_observer.incoming_frames >= 5, kTimeOut);
+    EXPECT_TRUE_WAIT(capture_observer.incoming_frames() >= 5, kTimeOut);
 
-    EXPECT_GT(capture_observer.capture_delay, 0);
+    EXPECT_GT(capture_observer.capture_delay(), 0);
 
     WebRtc_Word64 stop_time = TickTime::MillisecondTimestamp();
     EXPECT_EQ(0, module->StopCapture());
@@ -280,12 +348,14 @@ TEST_F(VideoCaptureTest, Capabilities) {
       module->CurrentDeviceName());
   EXPECT_GT(number_of_capabilities, 0);
   for (int i = 0; i < number_of_capabilities; ++i) {
-    device_info_->GetCapability(module->CurrentDeviceName(), i,
+    VideoCaptureCapability capability;
-                                capture_observer.capability);
+    EXPECT_EQ(0, device_info_->GetCapability(module->CurrentDeviceName(), i,
-    StartCapture(module.get(), capture_observer.capability);
+                                             capability));
+    capture_observer.SetExpectedCapability(capability);
+    StartCapture(module.get(), capability);
     // Make sure 5 frames are captured.
-    EXPECT_TRUE_WAIT(capture_observer.incoming_frames >= 5, kTimeOut);
+    EXPECT_TRUE_WAIT(capture_observer.incoming_frames() >= 5, kTimeOut);
-    capture_observer.incoming_frames = 0;
+
     EXPECT_EQ(0, module->StopCapture());
   }
 }
@@ -300,16 +370,16 @@ TEST_F(VideoCaptureTest, TestTwoCameras) {
   webrtc::scoped_refptr<VideoCaptureModule> module1(OpenVideoCaptureDevice(
           0, &capture_observer1));
   ASSERT_TRUE(module1.get() != NULL);
-
+  VideoCaptureCapability capability1;
 #ifndef WEBRTC_MAC
-  device_info_->GetCapability(module1->CurrentDeviceName(), 0,
+  device_info_->GetCapability(module1->CurrentDeviceName(), 0, capability1);
-                              capture_observer1.capability);
 #else
-  capture_observer1.capability.width = kTestWidth;
+  capability1.width = kTestWidth;
-  capture_observer1.capability.height = kTestHeight;
+  capability1.height = kTestHeight;
-  capture_observer1.capability.maxFPS = kTestFramerate;
+  capability1.maxFPS = kTestFramerate;
-  capture_observer1.capability.rawType = webrtc::kVideoUnknown;
+  capability1.rawType = webrtc::kVideoUnknown;
 #endif
+  capture_observer1.SetExpectedCapability(capability1);
 
   TestVideoCaptureCallback capture_observer2;
   webrtc::scoped_refptr<VideoCaptureModule> module2(OpenVideoCaptureDevice(
@@ -317,20 +387,21 @@ TEST_F(VideoCaptureTest, TestTwoCameras) {
   ASSERT_TRUE(module1.get() != NULL);
 
 
+  VideoCaptureCapability capability2;
 #ifndef WEBRTC_MAC
-  device_info_->GetCapability(module2->CurrentDeviceName(), 0,
+  device_info_->GetCapability(module2->CurrentDeviceName(), 0, capability2);
-                              capture_observer2.capability);
 #else
-  capture_observer2.capability.width = kTestWidth;
+  capability2.width = kTestWidth;
-  capture_observer2.capability.height = kTestHeight;
+  capability2.height = kTestHeight;
-  capture_observer2.capability.maxFPS = kTestFramerate;
+  capability2.maxFPS = kTestFramerate;
-  capture_observer2.capability.rawType = webrtc::kVideoUnknown;
+  capability2.rawType = webrtc::kVideoUnknown;
 #endif
+  capture_observer2.SetExpectedCapability(capability2);
 
-  StartCapture(module1.get(), capture_observer1.capability);
+  StartCapture(module1.get(), capability1);
-  StartCapture(module2.get(), capture_observer2.capability);
+  StartCapture(module2.get(), capability2);
-  EXPECT_TRUE_WAIT(capture_observer1.incoming_frames >= 5, kTimeOut);
+  EXPECT_TRUE_WAIT(capture_observer1.incoming_frames() >= 5, kTimeOut);
-  EXPECT_TRUE_WAIT(capture_observer2.incoming_frames >= 5, kTimeOut);
+  EXPECT_TRUE_WAIT(capture_observer2.incoming_frames() >= 5, kTimeOut);
 }
 
 // Test class for testing external capture and capture feedback information
@@ -343,10 +414,12 @@ class VideoCaptureExternalTest : public testing::Test {
     process_module_->Start();
     process_module_->RegisterModule(capture_module_);
 
-    capture_callback_.capability.width = kTestWidth;
+    VideoCaptureCapability capability;
-    capture_callback_.capability.height = kTestHeight;
+    capability.width = kTestWidth;
-    capture_callback_.capability.rawType = webrtc::kVideoYV12;
+    capability.height = kTestHeight;
-    capture_callback_.capability.maxFPS = kTestFramerate;
+    capability.rawType = webrtc::kVideoYV12;
+    capability.maxFPS = kTestFramerate;
+    capture_callback_.SetExpectedCapability(capability);
 
     test_frame_.VerifyAndAllocate(kTestWidth * kTestHeight * 3 / 2);
     test_frame_.SetLength(kTestWidth * kTestHeight * 3 / 2);
@@ -378,9 +451,9 @@ class VideoCaptureExternalTest : public testing::Test {
 // Test input of external video frames.
 TEST_F(VideoCaptureExternalTest , TestExternalCapture) {
   EXPECT_EQ(0, capture_input_interface_->IncomingFrame(
-      test_frame_.Buffer(), test_frame_.Length(), capture_callback_.capability,
+      test_frame_.Buffer(), test_frame_.Length(),
-      0));
+      capture_callback_.capability(), 0));
-  EXPECT_TRUE(CompareFrames(test_frame_, capture_callback_.last_frame));
+  EXPECT_TRUE(capture_callback_.CompareLastFrame(test_frame_));
 }
 
 // Test input of planar I420 frames.
@@ -395,7 +468,7 @@ TEST_F(VideoCaptureExternalTest , TestExternalCaptureI420) {
   frame_i420.u_pitch = kTestWidth / 2;
   frame_i420.v_pitch = kTestWidth / 2;
   EXPECT_EQ(0, capture_input_interface_->IncomingFrameI420(frame_i420, 0));
-  EXPECT_TRUE(CompareFrames(frame_i420, capture_callback_.last_frame));
+  EXPECT_TRUE(capture_callback_.CompareLastFrame(frame_i420));
 
   // Test with a frame with pitch not equal to width
   memset(test_frame_.Buffer(), 0xAA, test_frame_.Length());
@@ -448,54 +521,51 @@ TEST_F(VideoCaptureExternalTest , TestExternalCaptureI420) {
   frame_i420.v_pitch = v_pitch;
 
   EXPECT_EQ(0, capture_input_interface_->IncomingFrameI420(frame_i420, 0));
-  EXPECT_TRUE(CompareFrames(test_frame_, capture_callback_.last_frame));
+  EXPECT_TRUE(capture_callback_.CompareLastFrame(test_frame_));
-
 }
 
 // Test frame rate and no picture alarm.
 TEST_F(VideoCaptureExternalTest , FrameRate) {
   WebRtc_Word64 testTime = 3;
   TickTime startTime = TickTime::Now();
-  capture_callback_.capability.maxFPS = 10;
+
   while ((TickTime::Now() - startTime).Milliseconds() < testTime * 1000) {
     EXPECT_EQ(0, capture_input_interface_->IncomingFrame(
         test_frame_.Buffer(), test_frame_.Length(),
-        capture_callback_.capability, 0));
+        capture_callback_.capability(), 0));
-    SleepMs(1000 / capture_callback_.capability.maxFPS);
+    SleepMs(100);
   }
-  EXPECT_TRUE(capture_feedback_.frame_rate >= 8 &&
+  EXPECT_TRUE(capture_feedback_.frame_rate() >= 8 &&
-              capture_feedback_.frame_rate <= 10);
+              capture_feedback_.frame_rate() <= 10);
   SleepMs(500);
-  EXPECT_EQ(webrtc::Raised, capture_feedback_.alarm);
+  EXPECT_EQ(webrtc::Raised, capture_feedback_.alarm());
 
   startTime = TickTime::Now();
-  capture_callback_.capability.maxFPS = 30;
   while ((TickTime::Now() - startTime).Milliseconds() < testTime * 1000) {
     EXPECT_EQ(0, capture_input_interface_->IncomingFrame(
         test_frame_.Buffer(), test_frame_.Length(),
-        capture_callback_.capability, 0));
+        capture_callback_.capability(), 0));
-    SleepMs(1000 / capture_callback_.capability.maxFPS);
+    SleepMs(1000 / 30);
   }
-  EXPECT_EQ(webrtc::Cleared, capture_feedback_.alarm);
+  EXPECT_EQ(webrtc::Cleared, capture_feedback_.alarm());
   // Frame rate might be less than 33 since we have paused providing
   // frames for a while.
-  EXPECT_TRUE(capture_feedback_.frame_rate >= 25 &&
+  EXPECT_TRUE(capture_feedback_.frame_rate() >= 25 &&
-              capture_feedback_.frame_rate <= 33);
+              capture_feedback_.frame_rate() <= 33);
 }
 
 // Test start image
 TEST_F(VideoCaptureExternalTest , StartImage) {
-  capture_callback_.capability.maxFPS = 10;
   EXPECT_EQ(0, capture_module_->StartSendImage(
-      test_frame_, capture_callback_.capability.maxFPS));
+      test_frame_, 10));
 
-  EXPECT_TRUE_WAIT(capture_callback_.incoming_frames == 5, kTimeOut);
+  EXPECT_TRUE_WAIT(capture_callback_.incoming_frames() == 5, kTimeOut);
   EXPECT_EQ(0, capture_module_->StopSendImage());
 
   SleepMs(200);
   // Test that no more start images have arrived.
-  EXPECT_TRUE(capture_callback_.incoming_frames >= 4 &&
+  EXPECT_TRUE(capture_callback_.incoming_frames() >= 4 &&
-              capture_callback_.incoming_frames <= 5);
+              capture_callback_.incoming_frames() <= 5);
-  EXPECT_TRUE(CompareFrames(test_frame_, capture_callback_.last_frame));
+  EXPECT_TRUE(capture_callback_.CompareLastFrame(test_frame_));
 }