Title: [128082] trunk/Source
- Revision
- 128082
- Author
- [email protected]
- Date
- 2012-09-10 11:22:51 -0700 (Mon, 10 Sep 2012)
Log Message
[chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
https://bugs.webkit.org/show_bug.cgi?id=96010
Reviewed by James Robinson.
Source/Platform:
Add some additional clarifying comments.
* chromium/public/WebVideoFrameProvider.h:
(Client):
(WebVideoFrameProvider):
Source/WebKit/chromium:
The key fix here is that the destructor no longer has a mutex.
The m_compositingMutex was supposedly protecting races between
~WebMediaPlayerClientImpl and setVideoFrameProviderClient. The
former is only called from the main thread and the latter is called
from the compositor thread only when the main thread is blocked (and
it already asserts that this is the case).
In addition, the m_providerMutex in CCVideoLayerImpl prevents the
destruction of WebMediaPlayerClientImpl, thus keeping the frame
acquired via getCurrentFrame alive until putCurrentFrame is called.
These functions are only called by the client, and comments are added
to the interface to better document this.
To prevent a race between load() and getCurrentFrame/putCurrentFrame
(which are called from different threads) a new m_webMediaPlayerMutex
to replace part of what the old m_compositingMutex was doing.
* src/WebMediaPlayerClientImpl.cpp:
(WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl):
(WebKit::WebMediaPlayerClientImpl::load):
(WebKit::WebMediaPlayerClientImpl::loadInternal):
(WebKit::WebMediaPlayerClientImpl::setVideoFrameProviderClient):
(WebKit::WebMediaPlayerClientImpl::getCurrentFrame):
(WebKit::WebMediaPlayerClientImpl::putCurrentFrame):
* src/WebMediaPlayerClientImpl.h:
(WebMediaPlayerClientImpl):
Modified Paths
Diff
Modified: trunk/Source/Platform/ChangeLog (128081 => 128082)
--- trunk/Source/Platform/ChangeLog 2012-09-10 18:11:04 UTC (rev 128081)
+++ trunk/Source/Platform/ChangeLog 2012-09-10 18:22:51 UTC (rev 128082)
@@ -1,3 +1,16 @@
+2012-09-10 Adrienne Walker <[email protected]>
+
+ [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
+ https://bugs.webkit.org/show_bug.cgi?id=96010
+
+ Reviewed by James Robinson.
+
+ Add some additional clarifying comments.
+
+ * chromium/public/WebVideoFrameProvider.h:
+ (Client):
+ (WebVideoFrameProvider):
+
2012-09-10 Tommy Widenflycht <[email protected]>
[chromium] MediaStream API: Remove the Descriptor postfix
Modified: trunk/Source/Platform/chromium/public/WebVideoFrameProvider.h (128081 => 128082)
--- trunk/Source/Platform/chromium/public/WebVideoFrameProvider.h 2012-09-10 18:11:04 UTC (rev 128081)
+++ trunk/Source/Platform/chromium/public/WebVideoFrameProvider.h 2012-09-10 18:22:51 UTC (rev 128082)
@@ -46,7 +46,8 @@
class Client {
public:
// Provider will call this method to tell the client to stop using it.
- // stopUsingProvider() may be called from any thread.
+ // stopUsingProvider() may be called from any thread. The client should
+ // block until it has putCurrentFrame any outstanding frames.
virtual void stopUsingProvider() = 0;
// Notifies the provider's client that a call to getCurrentFrame() will return new data.
@@ -57,17 +58,20 @@
virtual void didUpdateMatrix(const float*) = 0;
};
- // May be called from any thread.
+ // May be called from any thread, but there must be some external guarantee
+ // that the provider is not destroyed before this call returns.
virtual void setVideoFrameProviderClient(Client*) = 0;
// This function places a lock on the current frame and returns a pointer to it.
// Calls to this method should always be followed with a call to putCurrentFrame().
// The ownership of the object is not transferred to the caller and
- // the caller should not free the returned object.
+ // the caller should not free the returned object. Only the current provider
+ // client should call this function.
virtual WebVideoFrame* getCurrentFrame() = 0;
// This function releases the lock on the video frame in chromium. It should
// always be called after getCurrentFrame(). Frames passed into this method
- // should no longer be referenced after the call is made.
+ // should no longer be referenced after the call is made. Only the current
+ // provider client should call this function.
virtual void putCurrentFrame(WebVideoFrame*) = 0;
};
Modified: trunk/Source/WebKit/chromium/ChangeLog (128081 => 128082)
--- trunk/Source/WebKit/chromium/ChangeLog 2012-09-10 18:11:04 UTC (rev 128081)
+++ trunk/Source/WebKit/chromium/ChangeLog 2012-09-10 18:22:51 UTC (rev 128082)
@@ -1,3 +1,37 @@
+2012-09-10 Adrienne Walker <[email protected]>
+
+ [chromium] Fix deadlock between WebMediaPlayerClientImpl dtor and PutCurrentFrame
+ https://bugs.webkit.org/show_bug.cgi?id=96010
+
+ Reviewed by James Robinson.
+
+ The key fix here is that the destructor no longer has a mutex.
+ The m_compositingMutex was supposedly protecting races between
+ ~WebMediaPlayerClientImpl and setVideoFrameProviderClient. The
+ former is only called from the main thread and the latter is called
+ from the compositor thread only when the main thread is blocked (and
+ it already asserts that this is the case).
+
+ In addition, the m_providerMutex in CCVideoLayerImpl prevents the
+ destruction of WebMediaPlayerClientImpl, thus keeping the frame
+ acquired via getCurrentFrame alive until putCurrentFrame is called.
+ These functions are only called by the client, and comments are added
+ to the interface to better document this.
+
+ To prevent a race between load() and getCurrentFrame/putCurrentFrame
+ (which are called from different threads) a new m_webMediaPlayerMutex
+ to replace part of what the old m_compositingMutex was doing.
+
+ * src/WebMediaPlayerClientImpl.cpp:
+ (WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl):
+ (WebKit::WebMediaPlayerClientImpl::load):
+ (WebKit::WebMediaPlayerClientImpl::loadInternal):
+ (WebKit::WebMediaPlayerClientImpl::setVideoFrameProviderClient):
+ (WebKit::WebMediaPlayerClientImpl::getCurrentFrame):
+ (WebKit::WebMediaPlayerClientImpl::putCurrentFrame):
+ * src/WebMediaPlayerClientImpl.h:
+ (WebMediaPlayerClientImpl):
+
2012-09-10 Rick Byers <[email protected]>
[chromium] Don't use WebGestureEvent.boundingBox for touch adjustment
Modified: trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (128081 => 128082)
--- trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp 2012-09-10 18:11:04 UTC (rev 128081)
+++ trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp 2012-09-10 18:22:51 UTC (rev 128082)
@@ -92,9 +92,11 @@
WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
{
#if USE(ACCELERATED_COMPOSITING)
- MutexLocker locker(m_compositingMutex);
if (m_videoFrameProviderClient)
m_videoFrameProviderClient->stopUsingProvider();
+ // No need for a lock here, as getCurrentFrame/putCurrentFrame can't be
+ // called now that the client is no longer using this provider. Also, load()
+ // and this destructor are called from the same thread.
if (m_webMediaPlayer)
m_webMediaPlayer->setStreamTextureClient(0);
#endif
@@ -312,8 +314,8 @@
{
m_url = url;
+ MutexLocker locker(m_webMediaPlayerMutex);
if (m_preload == MediaPlayer::None) {
- MutexLocker locker(m_compositingMutex);
#if ENABLE(WEB_AUDIO)
m_audioSourceProvider.wrap(0); // Clear weak reference to m_webMediaPlayer's WebAudioSourceProvider.
#endif
@@ -325,7 +327,6 @@
void WebMediaPlayerClientImpl::loadInternal()
{
- MutexLocker locker(m_compositingMutex);
#if ENABLE(WEB_AUDIO)
m_audioSourceProvider.wrap(0); // Clear weak reference to m_webMediaPlayer's WebAudioSourceProvider.
#endif
@@ -766,7 +767,7 @@
void WebMediaPlayerClientImpl::setVideoFrameProviderClient(WebVideoFrameProvider::Client* client)
{
- MutexLocker locker(m_compositingMutex);
+ MutexLocker locker(m_webMediaPlayerMutex);
if (m_videoFrameProviderClient)
m_videoFrameProviderClient->stopUsingProvider();
m_videoFrameProviderClient = client;
@@ -776,8 +777,10 @@
WebVideoFrame* WebMediaPlayerClientImpl::getCurrentFrame()
{
- MutexLocker locker(m_compositingMutex);
+ // This function is called only by the client.
+ MutexLocker locker(m_webMediaPlayerMutex);
ASSERT(!m_currentVideoFrame);
+ ASSERT(m_videoFrameProviderClient);
if (m_webMediaPlayer)
m_currentVideoFrame = m_webMediaPlayer->getCurrentFrame();
return m_currentVideoFrame;
@@ -785,8 +788,10 @@
void WebMediaPlayerClientImpl::putCurrentFrame(WebVideoFrame* videoFrame)
{
- MutexLocker locker(m_compositingMutex);
+ // This function is called only by the client.
+ MutexLocker locker(m_webMediaPlayerMutex);
ASSERT(videoFrame == m_currentVideoFrame);
+ ASSERT(m_videoFrameProviderClient);
if (!videoFrame)
return;
if (m_webMediaPlayer)
Modified: trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (128081 => 128082)
--- trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h 2012-09-10 18:11:04 UTC (rev 128081)
+++ trunk/Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h 2012-09-10 18:22:51 UTC (rev 128082)
@@ -197,7 +197,7 @@
bool acceleratedRenderingInUse();
#endif
- Mutex m_compositingMutex; // Guards m_currentVideoFrame and m_videoFrameProviderClient.
+ Mutex m_webMediaPlayerMutex; // Guards the m_webMediaPlayer
WebCore::MediaPlayer* m_mediaPlayer;
OwnPtr<WebMediaPlayer> m_webMediaPlayer;
WebVideoFrame* m_currentVideoFrame;
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
