Commit aba6a96e authored by qinmin's avatar qinmin Committed by Commit bot

Revert of Fix a crash that MediaPlayerAndroid weak_ptr is incorrectly used...

Revert of Fix a crash that MediaPlayerAndroid weak_ptr is incorrectly used across thread (patchset #1 id:1 of https://codereview.chromium.org/899473002/)

Reason for revert:
this CL doesn't fix the original issue

Original issue's description:
> Fix a crash that MediaPlayerAndroid weak_ptr is incorrectly used across thread
>
> For MediaPlayerListener, the callbacks can happen on a non-UI thread.
> Weak_ptr is not thread safe, so we shouldn't bind the weak_ptr in those callbacks.
> This change creates a threadsafe proxy class to bridge all the callbacks.
>
> BUG=453581, 447368, 447367
>
> Committed: https://crrev.com/2e9885788692f69203a98fb42347428ef553d640
> Cr-Commit-Position: refs/heads/master@{#314241}

TBR=xhwang@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=453581, 447368, 447367

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

Cr-Commit-Position: refs/heads/master@{#315352}
parent c946cf3e
...@@ -75,6 +75,16 @@ class MEDIA_EXPORT MediaPlayerAndroid { ...@@ -75,6 +75,16 @@ class MEDIA_EXPORT MediaPlayerAndroid {
// Associates the |cdm| with this player. // Associates the |cdm| with this player.
virtual void SetCdm(BrowserCdm* cdm); virtual void SetCdm(BrowserCdm* cdm);
int player_id() { return player_id_; }
GURL frame_url() { return frame_url_; }
protected:
MediaPlayerAndroid(int player_id,
MediaPlayerManager* manager,
const RequestMediaResourcesCB& request_media_resources_cb,
const GURL& frame_url);
// TODO(qinmin): Simplify the MediaPlayerListener class to only listen to // TODO(qinmin): Simplify the MediaPlayerListener class to only listen to
// media interrupt events. And have a separate child class to listen to all // media interrupt events. And have a separate child class to listen to all
// the events needed by MediaPlayerBridge. http://crbug.com/422597. // the events needed by MediaPlayerBridge. http://crbug.com/422597.
...@@ -87,16 +97,6 @@ class MEDIA_EXPORT MediaPlayerAndroid { ...@@ -87,16 +97,6 @@ class MEDIA_EXPORT MediaPlayerAndroid {
virtual void OnSeekComplete(); virtual void OnSeekComplete();
virtual void OnMediaPrepared(); virtual void OnMediaPrepared();
int player_id() { return player_id_; }
GURL frame_url() { return frame_url_; }
protected:
MediaPlayerAndroid(int player_id,
MediaPlayerManager* manager,
const RequestMediaResourcesCB& request_media_resources_cb,
const GURL& frame_url);
// Attach/Detaches |listener_| for listening to all the media events. If // Attach/Detaches |listener_| for listening to all the media events. If
// |j_media_player| is NULL, |listener_| only listens to the system media // |j_media_player| is NULL, |listener_| only listens to the system media
// events. Otherwise, it also listens to the events from |j_media_player|. // events. Otherwise, it also listens to the events from |j_media_player|.
...@@ -108,6 +108,8 @@ class MEDIA_EXPORT MediaPlayerAndroid { ...@@ -108,6 +108,8 @@ class MEDIA_EXPORT MediaPlayerAndroid {
RequestMediaResourcesCB request_media_resources_cb_; RequestMediaResourcesCB request_media_resources_cb_;
private: private:
friend class MediaPlayerListener;
// Player ID assigned to this player. // Player ID assigned to this player.
int player_id_; int player_id_;
......
...@@ -18,7 +18,7 @@ using base::android::ScopedJavaLocalRef; ...@@ -18,7 +18,7 @@ using base::android::ScopedJavaLocalRef;
namespace media { namespace media {
MediaPlayerListenerProxy::MediaPlayerListenerProxy( MediaPlayerListener::MediaPlayerListener(
const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
base::WeakPtr<MediaPlayerAndroid> media_player) base::WeakPtr<MediaPlayerAndroid> media_player)
: task_runner_(task_runner), : task_runner_(task_runner),
...@@ -27,100 +27,6 @@ MediaPlayerListenerProxy::MediaPlayerListenerProxy( ...@@ -27,100 +27,6 @@ MediaPlayerListenerProxy::MediaPlayerListenerProxy(
DCHECK(media_player_); DCHECK(media_player_);
} }
void MediaPlayerListenerProxy::OnMediaError(int error_type) {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnMediaError, this, error_type));
return;
}
if (media_player_)
media_player_->OnMediaError(error_type);
}
void MediaPlayerListenerProxy::OnVideoSizeChanged(int width, int height) {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnVideoSizeChanged,
this, width, height));
return;
}
if (media_player_)
media_player_->OnVideoSizeChanged(width, height);
}
void MediaPlayerListenerProxy::OnBufferingUpdate(int percent) {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnBufferingUpdate,
this, percent));
return;
}
if (media_player_)
media_player_->OnBufferingUpdate(percent);
}
void MediaPlayerListenerProxy::OnPlaybackComplete() {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnPlaybackComplete, this));
return;
}
if (media_player_)
media_player_->OnPlaybackComplete();
}
void MediaPlayerListenerProxy::OnSeekComplete() {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnSeekComplete, this));
return;
}
if (media_player_)
media_player_->OnSeekComplete();
}
void MediaPlayerListenerProxy::OnMediaPrepared() {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnMediaPrepared, this));
return;
}
if (media_player_)
media_player_->OnMediaPrepared();
}
void MediaPlayerListenerProxy::OnMediaInterrupted() {
if (!task_runner_->BelongsToCurrentThread()) {
task_runner_->PostTask(
FROM_HERE,
base::Bind(&MediaPlayerListenerProxy::OnMediaInterrupted, this));
return;
}
if (media_player_)
media_player_->OnMediaInterrupted();
}
MediaPlayerListenerProxy::~MediaPlayerListenerProxy() {}
MediaPlayerListener::MediaPlayerListener(
const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
base::WeakPtr<MediaPlayerAndroid> media_player)
: proxy_(new MediaPlayerListenerProxy(task_runner, media_player)) {
}
MediaPlayerListener::~MediaPlayerListener() {} MediaPlayerListener::~MediaPlayerListener() {}
void MediaPlayerListener::CreateMediaPlayerListener( void MediaPlayerListener::CreateMediaPlayerListener(
...@@ -133,6 +39,7 @@ void MediaPlayerListener::CreateMediaPlayerListener( ...@@ -133,6 +39,7 @@ void MediaPlayerListener::CreateMediaPlayerListener(
} }
} }
void MediaPlayerListener::ReleaseMediaPlayerListenerResources() { void MediaPlayerListener::ReleaseMediaPlayerListenerResources() {
JNIEnv* env = AttachCurrentThread(); JNIEnv* env = AttachCurrentThread();
CHECK(env); CHECK(env);
...@@ -145,37 +52,45 @@ void MediaPlayerListener::ReleaseMediaPlayerListenerResources() { ...@@ -145,37 +52,45 @@ void MediaPlayerListener::ReleaseMediaPlayerListenerResources() {
void MediaPlayerListener::OnMediaError( void MediaPlayerListener::OnMediaError(
JNIEnv* /* env */, jobject /* obj */, jint error_type) { JNIEnv* /* env */, jobject /* obj */, jint error_type) {
proxy_->OnMediaError(error_type); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnMediaError, media_player_, error_type));
} }
void MediaPlayerListener::OnVideoSizeChanged( void MediaPlayerListener::OnVideoSizeChanged(
JNIEnv* /* env */, jobject /* obj */, jint width, jint height) { JNIEnv* /* env */, jobject /* obj */, jint width, jint height) {
proxy_->OnVideoSizeChanged(width, height); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnVideoSizeChanged, media_player_,
width, height));
} }
void MediaPlayerListener::OnBufferingUpdate( void MediaPlayerListener::OnBufferingUpdate(
JNIEnv* /* env */, jobject /* obj */, jint percent) { JNIEnv* /* env */, jobject /* obj */, jint percent) {
proxy_->OnBufferingUpdate(percent); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnBufferingUpdate, media_player_, percent));
} }
void MediaPlayerListener::OnPlaybackComplete( void MediaPlayerListener::OnPlaybackComplete(
JNIEnv* /* env */, jobject /* obj */) { JNIEnv* /* env */, jobject /* obj */) {
proxy_->OnPlaybackComplete(); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnPlaybackComplete, media_player_));
} }
void MediaPlayerListener::OnSeekComplete( void MediaPlayerListener::OnSeekComplete(
JNIEnv* /* env */, jobject /* obj */) { JNIEnv* /* env */, jobject /* obj */) {
proxy_->OnSeekComplete(); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnSeekComplete, media_player_));
} }
void MediaPlayerListener::OnMediaPrepared( void MediaPlayerListener::OnMediaPrepared(
JNIEnv* /* env */, jobject /* obj */) { JNIEnv* /* env */, jobject /* obj */) {
proxy_->OnMediaPrepared(); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnMediaPrepared, media_player_));
} }
void MediaPlayerListener::OnMediaInterrupted( void MediaPlayerListener::OnMediaInterrupted(
JNIEnv* /* env */, jobject /* obj */) { JNIEnv* /* env */, jobject /* obj */) {
proxy_->OnMediaInterrupted(); task_runner_->PostTask(FROM_HERE, base::Bind(
&MediaPlayerAndroid::OnMediaInterrupted, media_player_));
} }
bool MediaPlayerListener::RegisterMediaPlayerListener(JNIEnv* env) { bool MediaPlayerListener::RegisterMediaPlayerListener(JNIEnv* env) {
......
...@@ -19,32 +19,6 @@ namespace media { ...@@ -19,32 +19,6 @@ namespace media {
class MediaPlayerAndroid; class MediaPlayerAndroid;
// Helper class for posting MediaPlayerAndroid tasks on the UI thread.
class MediaPlayerListenerProxy
: public base::RefCountedThreadSafe<MediaPlayerListenerProxy> {
public:
MediaPlayerListenerProxy(
const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
base::WeakPtr<MediaPlayerAndroid> media_player);
void OnMediaError(int error_type);
void OnVideoSizeChanged(int width, int height);
void OnBufferingUpdate(int percent);
void OnPlaybackComplete();
void OnSeekComplete();
void OnMediaPrepared();
void OnMediaInterrupted();
private:
friend class base::RefCountedThreadSafe<MediaPlayerListenerProxy>;
~MediaPlayerListenerProxy();
// The message loop where |media_player_| lives.
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
// The MediaPlayerAndroid object all the callbacks should be sent to.
base::WeakPtr<MediaPlayerAndroid> media_player_;
};
// Acts as a thread proxy between java MediaPlayerListener object and // Acts as a thread proxy between java MediaPlayerListener object and
// MediaPlayerAndroid so that callbacks are posted onto the UI thread. // MediaPlayerAndroid so that callbacks are posted onto the UI thread.
class MediaPlayerListener { class MediaPlayerListener {
...@@ -55,7 +29,7 @@ class MediaPlayerListener { ...@@ -55,7 +29,7 @@ class MediaPlayerListener {
MediaPlayerListener( MediaPlayerListener(
const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
base::WeakPtr<MediaPlayerAndroid> media_player); base::WeakPtr<MediaPlayerAndroid> media_player);
virtual ~MediaPlayerListener(); virtual ~MediaPlayerListener();
// Called by the Java MediaPlayerListener and mirrored to corresponding // Called by the Java MediaPlayerListener and mirrored to corresponding
// callbacks. // callbacks.
...@@ -78,8 +52,11 @@ class MediaPlayerListener { ...@@ -78,8 +52,11 @@ class MediaPlayerListener {
static bool RegisterMediaPlayerListener(JNIEnv* env); static bool RegisterMediaPlayerListener(JNIEnv* env);
private: private:
// Proxy for posting tasks to the UI thread. // The message loop where |media_player_| lives.
scoped_refptr<MediaPlayerListenerProxy> proxy_; scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
// The MediaPlayerAndroid object all the callbacks should be sent to.
base::WeakPtr<MediaPlayerAndroid> media_player_;
base::android::ScopedJavaGlobalRef<jobject> j_media_player_listener_; base::android::ScopedJavaGlobalRef<jobject> j_media_player_listener_;
......
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