From a5c9c5758bd83b035323493a5f46d18c9335b91d Mon Sep 17 00:00:00 2001 From: "scherkus@chromium.org" Date: Tue, 21 Apr 2009 17:06:43 +0000 Subject: Fixes FFmpegDemuxerTest.ReadAndSeek test flakiness by using a WaitableEvent. Turns out it was the test code that was flaky and not a threading bug in our actual code. Under heavy load it was possible for a thread holding onto a reference to get time sliced long enough that the unit test continued executing until it asserted that the object was deleted when in fact it was not. BUG=10653 Review URL: http://codereview.chromium.org/88010 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@14113 0039d316-1c4b-4281-b951-d872f2087c98 --- media/base/mock_pipeline.h | 14 +++------- media/filters/ffmpeg_demuxer_unittest.cc | 44 ++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 23 deletions(-) (limited to 'media') diff --git a/media/base/mock_pipeline.h b/media/base/mock_pipeline.h index d320301..e317a5d 100644 --- a/media/base/mock_pipeline.h +++ b/media/base/mock_pipeline.h @@ -14,6 +14,7 @@ #include #include +#include "base/message_loop.h" #include "media/base/media_format.h" #include "media/base/pipeline.h" #include "testing/gtest/include/gtest/gtest.h" @@ -135,17 +136,11 @@ class MockPipeline : public media::Pipeline { // pipeline is request/pull-based, only enough tasks to satisfy the request // should ever be executed. void RunAllTasks() { - while (!task_queue_.empty()) { - Task* task = task_queue_.front(); - task_queue_.pop_front(); - task->Run(); - delete task; - } + message_loop_.RunAllPending(); } void PostTask(Task* task) { - EXPECT_TRUE(task); - task_queue_.push_back(task); + message_loop_.PostTask(FROM_HERE, task); } void Error(media::PipelineError error) { @@ -191,8 +186,7 @@ class MockPipeline : public media::Pipeline { int64 buffered_bytes_; int64 total_bytes_; - typedef std::deque TaskQueue; - TaskQueue task_queue_; + MessageLoop message_loop_; DISALLOW_COPY_AND_ASSIGN(MockPipeline); }; diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc index 47e8d61..d9cabfa 100644 --- a/media/filters/ffmpeg_demuxer_unittest.cc +++ b/media/filters/ffmpeg_demuxer_unittest.cc @@ -35,26 +35,40 @@ class PacketQueue : public Singleton { packet->stream_index = packets_.front().a; packet->size = packets_.front().b; packet->data = packets_.front().c; - packet->destruct = &DestructPacket; + packet->destruct = &PacketQueue::DestructPacket; packets_.pop_front(); // We now have an outstanding packet which must be freed at some point. ++outstanding_packets_; } - int outstanding_packets() { - return outstanding_packets_; + bool WaitForOutstandingPackets(int count) { + const base::TimeDelta kTimedWait = base::TimeDelta::FromMilliseconds(500); + while (outstanding_packets_ != count) { + if (!wait_for_outstanding_packets_.TimedWait(kTimedWait)) { + return false; + } + } + return true; } private: static void DestructPacket(AVPacket* packet) { - --(PacketQueue::get()->outstanding_packets_); + PacketQueue::get()->DestructPacket(); + } + + void DestructPacket() { + --outstanding_packets_; + wait_for_outstanding_packets_.Signal(); } // Only allow Singleton to create and delete PacketQueue. friend struct DefaultSingletonTraits; - PacketQueue() : outstanding_packets_(0) {} + PacketQueue() + : outstanding_packets_(0), + wait_for_outstanding_packets_(false, false) { + } ~PacketQueue() { CHECK(outstanding_packets_ == 0); @@ -70,6 +84,12 @@ class PacketQueue : public Singleton { // cleaned up. int outstanding_packets_; + // Tests can wait on this event until a specific number of outstanding packets + // have been reached. Used to ensure other threads release their references + // to objects so we don't get false positive test results when comparing the + // number of outstanding packets. + base::WaitableEvent wait_for_outstanding_packets_; + DISALLOW_COPY_AND_ASSIGN(PacketQueue); }; @@ -379,9 +399,7 @@ TEST(FFmpegDemuxerTest, InitializeStreams) { // TODO(scherkus): as we keep refactoring and improving our mocks (both FFmpeg // and pipeline/filters), try to break this test into two. Big issue right now // is that it takes ~50 lines of code just to set up FFmpegDemuxer. -// -// Refer to http://crbug.com/10653 -TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { +TEST(FFmpegDemuxerTest, ReadAndSeek) { // Prepare some test data. const int kAudio = 0; const int kVideo = 1; @@ -458,7 +476,7 @@ TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { // Manually release buffer, which should release any remaining AVPackets. reader = NULL; - EXPECT_EQ(0, PacketQueue::get()->outstanding_packets()); + EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0)); //---------------------------------------------------------------------------- // Seek tests. @@ -528,7 +546,7 @@ TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { // Manually release buffer, which should release any remaining AVPackets. reader = NULL; - EXPECT_EQ(0, PacketQueue::get()->outstanding_packets()); + EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0)); // Let's trigger another simple forward seek, but with outstanding packets. // The outstanding packets should get freed after the Seek() is issued. @@ -551,7 +569,7 @@ TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { // Manually release video buffer, remaining audio packets are outstanding. reader = NULL; - EXPECT_EQ(3, PacketQueue::get()->outstanding_packets()); + EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(3)); // Trigger the seek. g_expected_seek_timestamp = 1234; @@ -560,7 +578,7 @@ TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { EXPECT_TRUE(g_seek_event->TimedWait(base::TimeDelta::FromSeconds(1))); // All outstanding packets should have been freed. - EXPECT_EQ(0, PacketQueue::get()->outstanding_packets()); + EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0)); // Clean up. delete g_seek_event; @@ -582,5 +600,5 @@ TEST(FFmpegDemuxerTest, DISABLED_ReadAndSeek) { // Manually release buffer, which should release any remaining AVPackets. reader = NULL; - EXPECT_EQ(0, PacketQueue::get()->outstanding_packets()); + EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0)); } -- cgit v1.1