Commit 7def98aa authored by xhwang's avatar xhwang Committed by Commit bot

Avoid deadlock between Pipeline and RendererImpl.

Both Pipeline and RendererImpl use it's own lock. We could end up with a dead
lock in the following scenario:
- On main thread, Pipeline holds it's lock (lock A) and calls into RendererImpl,
  which also requires the RendererImpl's lock (lock B).
- On media thread, RendererImpl holds it's lock B and calls callback provided by
  the Pipeline, which requires lock A.

This CL makes sure Pipeline never calls into RendererImpl while holding lock A
and RendererImpl nevers calls (back) into Pipeline while holding lock B. So
deadlock should not happen.

This CL also reverts 3 CLs that disabled various tests due to the bug:
- Revert "Exclude tests that deadlock under DrMemory (and Tsan)"
   This reverts commit 0e014b9a.
- Revert "Disable deadlock-y tests in TSan instead of just suppressing the error."
   This reverts commit f712d106.
- Revert "Suppress a deadlock report through media::Pipeline::GetMediaTime."
   This reverts commit 5c72e0f6.

BUG=407452
TEST=Ran PipelineIntegrationTest.ChunkDemuxerAbortRead_VideoOnly in a TSAN build for 10 mins and didn't see any issue.

Review URL: https://codereview.chromium.org/512973002

Cr-Commit-Position: refs/heads/master@{#292332}
parent 6b4a8879
......@@ -18,9 +18,7 @@ const char kClearKeyKeySystem[] = "webkit-org.w3.clearkey";
// Supported media types.
const char kWebMAudioOnly[] = "audio/webm; codecs=\"vorbis\"";
#if !defined(THREAD_SANITIZER)
const char kWebMVideoOnly[] = "video/webm; codecs=\"vp8\"";
#endif // !defined(THREAD_SANITIZER)
const char kWebMAudioVideo[] = "video/webm; codecs=\"vorbis, vp8\"";
// EME-specific test results and errors.
......@@ -148,8 +146,6 @@ INSTANTIATE_TEST_CASE_P(SRC_ClearKey, EncryptedMediaTest,
INSTANTIATE_TEST_CASE_P(MSE_ClearKey, EncryptedMediaTest,
Combine(Values(kClearKeyKeySystem), Values(MSE)));
// http://crbug.com/407452
#if !defined(THREAD_SANITIZER)
IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, Playback_AudioOnly_WebM) {
TestSimplePlayback("bear-a_enc-a.webm", kWebMAudioOnly);
}
......@@ -173,7 +169,6 @@ IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, Playback_VideoClearAudio_WebM) {
IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, ConfigChangeVideo) {
TestConfigChange();
}
#endif // !defined(THREAD_SANITIZER)
IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, FrameSizeChangeVideo) {
// Times out on Windows XP. http://crbug.com/171937
......
......@@ -110,8 +110,6 @@ class MediaTest : public testing::WithParamInterface<bool>,
}
};
// http://crbug.com/407452
#if !defined(THREAD_SANITIZER)
IN_PROC_BROWSER_TEST_P(MediaTest, VideoBearTheora) {
PlayVideo("bear.ogv", GetParam());
}
......@@ -236,13 +234,10 @@ IN_PROC_BROWSER_TEST_F(MediaTest, Navigate) {
NavigateToURL(shell(), GURL(url::kAboutBlankURL));
EXPECT_FALSE(shell()->web_contents()->IsCrashed());
}
#endif // !defined(THREAD_SANITIZER)
INSTANTIATE_TEST_CASE_P(File, MediaTest, ::testing::Values(false));
INSTANTIATE_TEST_CASE_P(Http, MediaTest, ::testing::Values(true));
// http://crbug.com/407452
#if !defined(THREAD_SANITIZER)
IN_PROC_BROWSER_TEST_F(MediaTest, MAYBE(Yuv420pTheora)) {
RunColorFormatTest("yuv420p.ogv", kEnded);
}
......@@ -263,8 +258,6 @@ IN_PROC_BROWSER_TEST_F(MediaTest, MAYBE(Yuv444pVp9)) {
RunColorFormatTest("yuv444p.webm", "ENDED");
}
#endif // !defined(THREAD_SANITIZER)
#if defined(USE_PROPRIETARY_CODECS)
IN_PROC_BROWSER_TEST_F(MediaTest, MAYBE(Yuv420pH264)) {
RunColorFormatTest("yuv420p.mp4", kEnded);
......
......@@ -10,14 +10,12 @@
#endif
// Common media types.
#if !defined(THREAD_SANITIZER)
const char kWebMAudioOnly[] = "audio/webm; codecs=\"vorbis\"";
#if !defined(OS_ANDROID)
const char kWebMOpusAudioOnly[] = "audio/webm; codecs=\"opus\"";
#endif
const char kWebMVideoOnly[] = "video/webm; codecs=\"vp8\"";
const char kWebMAudioVideo[] = "video/webm; codecs=\"vorbis, vp8\"";
#endif // !defined(THREAD_SANITIZER)
namespace content {
......@@ -57,8 +55,6 @@ class MediaSourceTest : public content::MediaBrowserTest {
#endif
};
// http://crbug.com/407452
#if !defined(THREAD_SANITIZER)
IN_PROC_BROWSER_TEST_F(MediaSourceTest, Playback_VideoAudio_WebM) {
TestSimplePlayback("bear-320x240.webm", kWebMAudioVideo, kEnded);
}
......@@ -94,6 +90,5 @@ IN_PROC_BROWSER_TEST_F(MediaSourceTest, ConfigChangeVideo) {
kEnded,
true);
}
#endif // !defined(THREAD_SANITIZER)
} // namespace content
......@@ -153,9 +153,13 @@ void Pipeline::SetVolume(float volume) {
}
TimeDelta Pipeline::GetMediaTime() const {
if (!renderer_)
return TimeDelta();
TimeDelta media_time = renderer_->GetMediaTime();
base::AutoLock auto_lock(lock_);
return renderer_ ? std::min(renderer_->GetMediaTime(), duration_)
: TimeDelta();
return std::min(media_time, duration_);
}
Ranges<TimeDelta> Pipeline::GetBufferedTimeRanges() const {
......@@ -439,7 +443,7 @@ void Pipeline::OnStopCompleted(PipelineStatus status) {
DCHECK(!text_renderer_);
{
base::AutoLock l(lock_);
base::AutoLock auto_lock(lock_);
running_ = false;
}
......
......@@ -1425,8 +1425,6 @@ TEST_F(PipelineIntegrationTest, ChunkDemuxerAbortRead_AudioOnly) {
0x10CA, 19730));
}
// http://crbug.com/407452
#if !defined(THREAD_SANITIZER)
// Verify video decoder & renderer can handle aborted demuxer reads.
TEST_F(PipelineIntegrationTest, ChunkDemuxerAbortRead_VideoOnly) {
ASSERT_TRUE(TestSeekDuringRead("bear-320x240-video-only.webm", kVideoOnlyWebM,
......@@ -1435,7 +1433,6 @@ TEST_F(PipelineIntegrationTest, ChunkDemuxerAbortRead_VideoOnly) {
base::TimeDelta::FromMilliseconds(1668),
0x1C896, 65536));
}
#endif // !defined(THREAD_SANITIZER)
// Verify that Opus audio in WebM containers can be played back.
TEST_F(PipelineIntegrationTest, BasicPlayback_AudioOnly_Opus_WebM) {
......
......@@ -456,9 +456,10 @@ void RendererImpl::StartPlayback() {
interpolation_state_ = INTERPOLATION_WAITING_FOR_AUDIO_TIME_UPDATE;
time_source_->StartTicking();
} else {
base::TimeDelta duration = get_duration_cb_.Run();
base::AutoLock auto_lock(interpolator_lock_);
interpolation_state_ = INTERPOLATION_STARTED;
interpolator_->SetUpperBound(get_duration_cb_.Run());
interpolator_->SetUpperBound(duration);
interpolator_->StartInterpolating();
}
}
......@@ -505,8 +506,9 @@ void RendererImpl::OnAudioRendererEnded() {
// Start clock since there is no more audio to trigger clock updates.
{
base::TimeDelta duration = get_duration_cb_.Run();
base::AutoLock auto_lock(interpolator_lock_);
interpolator_->SetUpperBound(get_duration_cb_.Run());
interpolator_->SetUpperBound(duration);
StartClockIfWaitingForTimeUpdate_Locked();
}
......@@ -537,9 +539,9 @@ void RendererImpl::RunEndedCallbackIfNeeded() {
return;
{
base::TimeDelta duration = get_duration_cb_.Run();
base::AutoLock auto_lock(interpolator_lock_);
PauseClockAndStopTicking_Locked();
base::TimeDelta duration = get_duration_cb_.Run();
interpolator_->SetBounds(duration, duration);
}
......
......@@ -4,8 +4,3 @@ WebRtcBrowserTest.NoCrashWhenConnectChromiumSinkToRemoteTrack
# crbug.com/400490
PluginTest.PluginSingleRangeRequest
PluginTest.PluginThreadAsyncCall
# crbug.com/407452
*/MediaTest.*
MediaSourceTest.*
*/EncryptedMediaTest.*
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment