Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(121)

Issue 532993002: work-in-progress patch to fix context lost black video (Closed)

Created:
4 years, 9 months ago by boliu
Modified:
4 years, 9 months ago
Reviewers:
qinmin, no sievers
CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Handle context loss in WebMediaPlayerPlayer in-process Add plumbing for a ResetStreamTextureProxy which should be called on the event of a context loss. Only hooked up the call for the in-process implementation in this CL when the contex is restored. ResetStreamTextureProxy currently handles "onContextLoss" as well as "onContextRestored" concepts. It deletes the old stream texture id, and recreates and binds a new StreamTextureProxy. BUG=412578 Committed: https://crrev.com/7a2b1cb83546418638417fc4d3c9aa6172fe02e9 Cr-Commit-Position: refs/heads/master@{#295219}

Patch Set 1 #

Total comments: 1

Patch Set 2 : RemoveObserver in destructor #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 3

Patch Set 5 : post to impl #

Total comments: 2

Patch Set 6 : rebase #

Patch Set 7 : block on compositor #

Total comments: 22

Patch Set 8 : review #

Total comments: 1

Patch Set 9 : rebase #

Patch Set 10 : no block #

Total comments: 2

Patch Set 11 : remove client_lock_ #

Patch Set 12 : check client #

Total comments: 9

Patch Set 13 : fixes #

Patch Set 14 : more threading fix #

Total comments: 8

Patch Set 15 : sievers review #

Patch Set 16 : add back deleted lines #

Total comments: 1

Patch Set 17 : fix software mode #

Total comments: 4

Patch Set 18 : virtual destructor #

Patch Set 19 : rebase #

Total comments: 1

Messages

Total messages: 59 (5 generated)
boliu
Uploaded for first pass. Basically add ContextLost/ContextRestored callbacks to webmediaplyer_android, and implemented it for sync ...
4 years, 9 months ago (2014-09-03 00:56:02 UTC) #2
boliu
has crashes too :(
4 years, 9 months ago (2014-09-03 00:58:03 UTC) #3
boliu
https://codereview.chromium.org/532993002/diff/1/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/1/content/renderer/media/android/webmediaplayer_android.cc#newcode199 content/renderer/media/android/webmediaplayer_android.cc:199: stream_texture_factory_->AddObserver(this); don't judge...
4 years, 9 months ago (2014-09-03 01:00:07 UTC) #4
boliu
Arggg, this has obvious deadlock issues in webview
4 years, 9 months ago (2014-09-03 01:11:14 UTC) #5
boliu
Ok, please give a cursory look at PS4. Is the code in WMPA ok? Comments ...
4 years, 9 months ago (2014-09-03 21:04:16 UTC) #6
qinmin
https://codereview.chromium.org/532993002/diff/60001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/60001/content/renderer/media/android/webmediaplayer_android.cc#newcode342 content/renderer/media/android/webmediaplayer_android.cc:342: if (hasVideo() && needs_establish_peer_ && isn't needs_establish_peer_ true since ...
4 years, 9 months ago (2014-09-04 00:14:47 UTC) #7
no sievers
https://codereview.chromium.org/532993002/diff/60001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/60001/content/browser/android/in_process/synchronous_compositor_factory_impl.cc#newcode120 content/browser/android/in_process/synchronous_compositor_factory_impl.cc:120: context_provider_->BindToCurrentThread(); You can call context_provider_->SetLostContextCallback() here and bind it ...
4 years, 9 months ago (2014-09-04 22:06:36 UTC) #8
boliu
https://codereview.chromium.org/532993002/diff/80001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/80001/content/renderer/media/android/webmediaplayer_android.cc#newcode1247 content/renderer/media/android/webmediaplayer_android.cc:1247: stream_texture_proxy_initialized_ = true; This is not thread safe yet.. ...
4 years, 9 months ago (2014-09-08 15:31:10 UTC) #9
boliu
PTAL I changed webmediaplayer to block on compositor thread to bind the client. https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File ...
4 years, 9 months ago (2014-09-11 17:08:33 UTC) #10
no sievers
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1279 content/renderer/media/android/webmediaplayer_android.cc:1279: base::Unretained(this), Do we need to do this or can ...
4 years, 9 months ago (2014-09-11 22:28:34 UTC) #11
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1279 content/renderer/media/android/webmediaplayer_android.cc:1279: base::Unretained(this), On 2014/09/11 22:28:34, sievers wrote: > Do we ...
4 years, 9 months ago (2014-09-11 22:30:56 UTC) #12
no sievers
looks reasonable https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1208 content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread ...
4 years, 9 months ago (2014-09-11 22:52:34 UTC) #13
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1208 content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. ...
4 years, 9 months ago (2014-09-11 22:56:17 UTC) #14
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1208 content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. ...
4 years, 9 months ago (2014-09-11 22:57:35 UTC) #15
no sievers
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1208 content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. ...
4 years, 9 months ago (2014-09-11 23:06:24 UTC) #16
qinmin
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1251 content/renderer/media/android/webmediaplayer_android.cc:1251: needs_establish_peer_ = need to check fullscreen status https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1256 content/renderer/media/android/webmediaplayer_android.cc:1256: ...
4 years, 9 months ago (2014-09-11 23:26:41 UTC) #17
boliu
PTAL https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1251 content/renderer/media/android/webmediaplayer_android.cc:1251: needs_establish_peer_ = On 2014/09/11 23:26:41, qinmin wrote: > ...
4 years, 9 months ago (2014-09-12 16:38:25 UTC) #18
qinmin
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); SetVideoFrameProviderClient uses a lock inside, so it is ...
4 years, 9 months ago (2014-09-12 17:01:58 UTC) #19
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:01:58, qinmin wrote: > SetVideoFrameProviderClient uses ...
4 years, 9 months ago (2014-09-12 17:08:07 UTC) #20
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:08:07, boliu wrote: > On 2014/09/12 ...
4 years, 9 months ago (2014-09-12 17:10:14 UTC) #21
qinmin
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); But why cannot we just use a callback ...
4 years, 9 months ago (2014-09-12 17:19:48 UTC) #22
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:19:48, qinmin wrote: > But why ...
4 years, 9 months ago (2014-09-12 17:26:22 UTC) #23
qinmin
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:26:22, boliu wrote: > On 2014/09/12 ...
4 years, 9 months ago (2014-09-12 17:31:09 UTC) #24
boliu
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:31:09, qinmin wrote: > On 2014/09/12 ...
4 years, 9 months ago (2014-09-12 17:36:34 UTC) #25
qinmin
On 2014/09/12 17:36:34, boliu wrote: > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc#newcode1281 > ...
4 years, 9 months ago (2014-09-12 17:55:30 UTC) #26
no sievers
On 2014/09/12 17:55:30, qinmin wrote: > On 2014/09/12 17:36:34, boliu wrote: > > > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/android/webmediaplayer_android.cc ...
4 years, 9 months ago (2014-09-12 19:28:30 UTC) #27
qinmin
On 2014/09/12 19:28:30, sievers wrote: > On 2014/09/12 17:55:30, qinmin wrote: > > On 2014/09/12 ...
4 years, 9 months ago (2014-09-12 19:51:08 UTC) #28
no sievers
How about this: WMP::SetVideoFrameProviderClient(client) { // on impl thread base::AutoLock lock(client_lock_); DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread()); client_loop_ ...
4 years, 9 months ago (2014-09-12 19:56:22 UTC) #29
boliu
On 2014/09/12 19:51:08, qinmin wrote: > I remember we used to do that in GetCurrentFrame(), ...
4 years, 9 months ago (2014-09-12 20:08:17 UTC) #30
qinmin
On 2014/09/12 20:08:17, boliu wrote: > On 2014/09/12 19:51:08, qinmin wrote: > > I remember ...
4 years, 9 months ago (2014-09-12 20:12:38 UTC) #31
boliu
On 2014/09/12 20:12:38, qinmin wrote: > On 2014/09/12 20:08:17, boliu wrote: > > On 2014/09/12 ...
4 years, 9 months ago (2014-09-12 20:16:23 UTC) #32
qinmin
I think this sounds good to me, we don't need the long if statement inside ...
4 years, 9 months ago (2014-09-12 20:16:30 UTC) #33
boliu
https://codereview.chromium.org/532993002/diff/140001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/140001/content/renderer/media/android/webmediaplayer_android.cc#newcode1276 content/renderer/media/android/webmediaplayer_android.cc:1276: base::WaitableEvent completion(true, false); what if I add a "if ...
4 years, 9 months ago (2014-09-12 20:22:48 UTC) #34
boliu
On 2014/09/12 20:16:30, qinmin wrote: > I think this sounds good to me, we don't ...
4 years, 9 months ago (2014-09-12 20:27:59 UTC) #35
qinmin
Yes, I don't think we need to access !needs_external_surface_ and !is_remote_ on impl thread, we ...
4 years, 9 months ago (2014-09-12 20:43:30 UTC) #36
no sievers
On 2014/09/12 20:43:30, qinmin wrote: > Yes, I don't think we need to access !needs_external_surface_ ...
4 years, 9 months ago (2014-09-12 21:42:57 UTC) #37
boliu
Ok. You can look at the diff between PS9 and PS10 to see what's needed ...
4 years, 9 months ago (2014-09-13 20:46:02 UTC) #39
qinmin
https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/android/webmediaplayer_android.cc#newcode1230 content/renderer/media/android/webmediaplayer_android.cc:1230: base::AutoLock lock(client_lock_); In video_frame_provider_client_impl.cc, it mentioned that this call ...
4 years, 9 months ago (2014-09-13 23:49:45 UTC) #40
boliu
https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/android/webmediaplayer_android.cc#newcode1230 content/renderer/media/android/webmediaplayer_android.cc:1230: base::AutoLock lock(client_lock_); On 2014/09/13 23:49:44, qinmin wrote: > In ...
4 years, 9 months ago (2014-09-14 00:12:30 UTC) #41
boliu
Removed that lock. Didn't test it, but looks a lot saner.
4 years, 9 months ago (2014-09-14 00:26:35 UTC) #42
boliu
https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/android/stream_texture_factory_synchronous_impl.cc File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/android/stream_texture_factory_synchronous_impl.cc#newcode74 content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:74: delete this; Hmm, this crashes in lock because lock ...
4 years, 9 months ago (2014-09-15 17:00:29 UTC) #43
qinmin
https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/android/stream_texture_factory_impl.cc File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/android/stream_texture_factory_impl.cc#newcode55 content/renderer/media/android/stream_texture_factory_impl.cc:55: if (!loop_.get() || loop_.get() != base::MessageLoopProxy::current() || so if ...
4 years, 9 months ago (2014-09-15 17:16:27 UTC) #44
boliu
Ok, so StreamTextureProxyImpl threading correctness depends on the it must be used with ScopedStreamTextureProxy, which ...
4 years, 9 months ago (2014-09-15 17:54:07 UTC) #45
no sievers
https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/android/stream_texture_factory_impl.cc File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/android/stream_texture_factory_impl.cc#newcode90 content/renderer/media/android/stream_texture_factory_impl.cc:90: scoped_refptr<base::MessageLoopProxy> loop) { DCHECK(!client || (client && loop)); DCHECK(!loop_ ...
4 years, 9 months ago (2014-09-15 19:00:40 UTC) #46
boliu
PTAL https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/android/stream_texture_factory_impl.cc File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/android/stream_texture_factory_impl.cc#newcode90 content/renderer/media/android/stream_texture_factory_impl.cc:90: scoped_refptr<base::MessageLoopProxy> loop) { On 2014/09/15 19:00:39, sievers wrote: ...
4 years, 9 months ago (2014-09-15 20:41:53 UTC) #47
qinmin
lgtm
4 years, 9 months ago (2014-09-16 03:36:42 UTC) #48
no sievers
lgtm https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory.h File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory.h#newcode49 content/renderer/media/android/stream_texture_factory.h:49: ~StreamTextureFactoryContextObserver() {} virtual https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory_synchronous_impl.cc File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory_synchronous_impl.cc#newcode175 ...
4 years, 9 months ago (2014-09-17 01:29:52 UTC) #49
boliu
https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory.h File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/android/stream_texture_factory.h#newcode49 content/renderer/media/android/stream_texture_factory.h:49: ~StreamTextureFactoryContextObserver() {} On 2014/09/17 01:29:52, sievers wrote: > virtual ...
4 years, 9 months ago (2014-09-17 02:09:03 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/532993002/350001
4 years, 9 months ago (2014-09-17 02:09:47 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56893) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel/builds/11007) win_chromium_x64_rel_swarming ...
4 years, 9 months ago (2014-09-17 02:13:24 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/532993002/370001
4 years, 9 months ago (2014-09-17 02:19:57 UTC) #56
commit-bot: I haz the power
Committed patchset #19 (id:370001) as 8674c1c863fe3bbd79c59811f2395e61f5070dd5
4 years, 9 months ago (2014-09-17 03:16:31 UTC) #57
commit-bot: I haz the power
Patchset 19 (id:??) landed as https://crrev.com/7a2b1cb83546418638417fc4d3c9aa6172fe02e9 Cr-Commit-Position: refs/heads/master@{#295219}
4 years, 9 months ago (2014-09-17 03:17:10 UTC) #58
boliu
4 years, 9 months ago (2014-09-17 16:44:06 UTC) #59
Message was sent while issue was closed.
https://codereview.chromium.org/532993002/diff/370001/content/renderer/media/...
File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc
(right):

https://codereview.chromium.org/532993002/diff/370001/content/renderer/media/...
content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:231:
context_provider_->AddObserver(obs);
BAGGGG, did I really do this. :( :( :( Yes, yes I did, in a late refactor
yesterday morning. Never perf and code at the same time :(

Fix coming up

Powered by Google App Engine
This is Rietveld 408576698