Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
532993002:
work-in-progress patch to fix context lost black video (Closed)
|
Created:
6 years, 3 months ago by boliu Modified:
6 years, 3 months ago 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. |
DescriptionHandle 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@chromium.org changed reviewers: + qinmin@chromium.org, sievers@chromium.org
Uploaded for first pass. Basically add ContextLost/ContextRestored callbacks to webmediaplyer_android, and implemented it for sync compositor. The ContextLost implementation probably isn't totally correct. Seeing low video framerate after restore too.
has crashes too :(
https://codereview.chromium.org/532993002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:199: stream_texture_factory_->AddObserver(this); don't judge...
Arggg, this has obvious deadlock issues in webview
Ok, please give a cursory look at PS4. Is the code in WMPA ok? Comments on the whole callback hook up so that it can eventually become onContextLost?
https://codereview.chromium.org/532993002/diff/60001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:342: if (hasVideo() && needs_establish_peer_ && isn't needs_establish_peer_ true since you called SetNeedsEstablishPeer(true) above? https://codereview.chromium.org/532993002/diff/60001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:347: // TODO(boliu): Inform |video_frame_provider_client_| on compositor thread. you also need to bind the new stream texture proxy to the compositor thread
https://codereview.chromium.org/532993002/diff/60001/content/browser/android/... File content/browser/android/in_process/synchronous_compositor_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/60001/content/browser/android/... content/browser/android/in_process/synchronous_compositor_factory_impl.cc:120: context_provider_->BindToCurrentThread(); You can call context_provider_->SetLostContextCallback() here and bind it to RestoreContext() instead of the RestoreContextOnMainThread() logic below. Then I'd edit GLNonOwnedContext::MakeCurrent() and track the underlying GL context (eglGetCurrentContext) and return |false| if it ever changes.
https://codereview.chromium.org/532993002/diff/80001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/80001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1247: stream_texture_proxy_initialized_ = true; This is not thread safe yet.. https://codereview.chromium.org/532993002/diff/80001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1269: SetVideoFrameProviderClient(video_frame_provider_client_); Back in m37 days, we used to call BindToThread here, but apparently that has been removed in trunk. I actually don't think that will work in webview when we could create the stream texture proxy later. Min, any thoughts on this change?
PTAL I changed webmediaplayer to block on compositor thread to bind the client. https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1252: !needs_external_surface_ && !is_remote_ && (hasVideo() || IsHLSStream()); Min, please double check this line. Unfortunately that clean up had to be reverted because it really did break perf tests. Otherwise it would be a lot easier..
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1279: base::Unretained(this), Do we need to do this or can we just invalidate and lazily bind to the thread when GetCurrentFrame() happens?
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1279: base::Unretained(this), On 2014/09/11 22:28:34, sievers wrote: > Do we need to do this or can we just invalidate and lazily bind to the thread > when GetCurrentFrame() happens? That's fine in m37. But that code doesn't exist anymore in trunk.
looks reasonable https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. At that time, This is really just based on probably what the current state of the code is, and contradicts the general comment in video_frame_provider.h ("May be called from any thread") :(
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. At that time, On 2014/09/11 22:52:33, sievers wrote: > This is really just based on probably what the current state of the code is, and > contradicts the general comment in video_frame_provider.h ("May be called from > any thread") :( Yep. It's a requirement that main thread is blocked when this is called on the compositor thread. It says so a few lines above. I'll update the header.
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. At that time, On 2014/09/11 22:56:17, boliu wrote: > On 2014/09/11 22:52:33, sievers wrote: > > This is really just based on probably what the current state of the code is, > and > > contradicts the general comment in video_frame_provider.h ("May be called from > > any thread") :( > > Yep. It's a requirement that main thread is blocked when this is called on the > compositor thread. It says so a few lines above. > > I'll update the header. Ehh, update in a follow up since that's in cc. Or maybe fix this once and for all somehow...
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1208: // If client exists, the compositor thread calls it. At that time, On 2014/09/11 22:57:35, boliu wrote: > On 2014/09/11 22:56:17, boliu wrote: > > On 2014/09/11 22:52:33, sievers wrote: > > > This is really just based on probably what the current state of the code is, > > and > > > contradicts the general comment in video_frame_provider.h ("May be called > from > > > any thread") :( > > > > Yep. It's a requirement that main thread is blocked when this is called on the > > compositor thread. It says so a few lines above. > > > > I'll update the header. > > Ehh, update in a follow up since that's in cc. Or maybe fix this once and for > all somehow... I was more worried about SetVideoFrameProviderClient() getting called on the main thread. The 'if (client' check etc. looks dubious. Well, not a problem of this patch really.
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... 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/... content/renderer/media/android/webmediaplayer_android.cc:1256: EstablishSurfaceTexturePeer(); shouldn't this be: if (needs_establish_peer_ && is_playing_) EstablishSurfaceTexturePeer(); https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); blocking the main thread? I don't feel like this is ok, better check with gpu folks on that. https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1288: SetVideoFrameProviderClient(video_frame_provider_client_); nit: no {}
PTAL https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1251: needs_establish_peer_ = On 2014/09/11 23:26:41, qinmin wrote: > need to check fullscreen status Done using player_manager_->IsInFullscreen(frame_) https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1256: EstablishSurfaceTexturePeer(); On 2014/09/11 23:26:41, qinmin wrote: > shouldn't this be: > > if (needs_establish_peer_ && is_playing_) EstablishSurfaceTexturePeer(); Yes done. If only the refactor wasn't reverted...... https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/11 23:26:41, qinmin wrote: > blocking the main thread? I don't feel like this is ok, better check with gpu > folks on that. Blink thread blocks on compositor thread all the time inside the compositor (eg when SetVideoFrameProviderClient is called), so it is definitely allowed. I'm not sure what your concern is here? https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1288: SetVideoFrameProviderClient(video_frame_provider_client_); On 2014/09/11 23:26:41, qinmin wrote: > nit: no {} Done.
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); SetVideoFrameProviderClient uses a lock inside, so it is very cheap. Here you are posting a task, and let the main thread blocked. Are you 100% sure the impl thread will never be waiting on the render thread before executing this task? And what is the impact of blocking main thread, any impacts on javascript performance? On 2014/09/12 16:38:25, boliu wrote: > On 2014/09/11 23:26:41, qinmin wrote: > > blocking the main thread? I don't feel like this is ok, better check with gpu > > folks on that. > > Blink thread blocks on compositor thread all the time inside the compositor (eg > when SetVideoFrameProviderClient is called), so it is definitely allowed. > > I'm not sure what your concern is here?
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:01:58, qinmin wrote: > SetVideoFrameProviderClient uses a lock inside, so it is very cheap. > Here you are posting a task, and let the main thread blocked. Are you 100% sure > the impl thread will never be waiting on the render thread before executing this > task? And what is the impact of blocking main thread, any impacts on javascript > performance? By render thread, you mean blink main thread, right? I am 100% sure compositor thread will never block on blink main thread. There is no deadlock issues here. Sure blocking *might* be expensive, but the goal is to keep the compositor thread lively so it's never expensive in practice. Compositor already does exactly this on every single commit: https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr... > > On 2014/09/12 16:38:25, boliu wrote: > > On 2014/09/11 23:26:41, qinmin wrote: > > > blocking the main thread? I don't feel like this is ok, better check with > gpu > > > folks on that. > > > > Blink thread blocks on compositor thread all the time inside the compositor > (eg > > when SetVideoFrameProviderClient is called), so it is definitely allowed. > > > > I'm not sure what your concern is here? >
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:08:07, boliu wrote: > On 2014/09/12 17:01:58, qinmin wrote: > > SetVideoFrameProviderClient uses a lock inside, so it is very cheap. > > Here you are posting a task, and let the main thread blocked. Are you 100% > sure > > the impl thread will never be waiting on the render thread before executing > this > > task? And what is the impact of blocking main thread, any impacts on > javascript > > performance? > > By render thread, you mean blink main thread, right? I am 100% sure compositor > thread will never block on blink main thread. There is no deadlock issues here. > > Sure blocking *might* be expensive, but the goal is to keep the compositor > thread lively so it's never expensive in practice. Compositor already does > exactly this on every single commit: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr... > > > > > On 2014/09/12 16:38:25, boliu wrote: > > > On 2014/09/11 23:26:41, qinmin wrote: > > > > blocking the main thread? I don't feel like this is ok, better check with > > gpu > > > > folks on that. > > > > > > Blink thread blocks on compositor thread all the time inside the compositor > > (eg > > > when SetVideoFrameProviderClient is called), so it is definitely allowed. > > > > > > I'm not sure what your concern is here? > > > Also I'm floating the idea that WMPA should do more blocking like this so to make threading safety saner, and keep the promises like SetVideoFrameProviderClient can be called on compositor thread at any time, not just when it's blocked.
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); But why cannot we just use a callback and don't block the render thread? On 2014/09/12 17:08:07, boliu wrote: > On 2014/09/12 17:01:58, qinmin wrote: > > SetVideoFrameProviderClient uses a lock inside, so it is very cheap. > > Here you are posting a task, and let the main thread blocked. Are you 100% > sure > > the impl thread will never be waiting on the render thread before executing > this > > task? And what is the impact of blocking main thread, any impacts on > javascript > > performance? > > By render thread, you mean blink main thread, right? I am 100% sure compositor > thread will never block on blink main thread. There is no deadlock issues here. > > Sure blocking *might* be expensive, but the goal is to keep the compositor > thread lively so it's never expensive in practice. Compositor already does > exactly this on every single commit: > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr... > > > > > On 2014/09/12 16:38:25, boliu wrote: > > > On 2014/09/11 23:26:41, qinmin wrote: > > > > blocking the main thread? I don't feel like this is ok, better check with > > gpu > > > > folks on that. > > > > > > Blink thread blocks on compositor thread all the time inside the compositor > > (eg > > > when SetVideoFrameProviderClient is called), so it is definitely allowed. > > > > > > I'm not sure what your concern is here? > > >
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:19:48, qinmin wrote: > But why cannot we just use a callback and don't block the render thread? Calling SetVideoFrameProviderClient without the main thread blocked is not thread safe right now. Eg there is no lock protecting video_frame_provider_client_, stream_texture_proxy_initialized_, etc
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:26:22, boliu wrote: > On 2014/09/12 17:19:48, qinmin wrote: > > But why cannot we just use a callback and don't block the render thread? > > Calling SetVideoFrameProviderClient without the main thread blocked is not > thread safe right now. Eg there is no lock protecting > video_frame_provider_client_, stream_texture_proxy_initialized_, etc I don't see that's a valid reason to block main thread. we can just have a variable on main thread checking that a post task is in progress, and don't access any of the method that has these thread_unsafe variables in there
https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: completion.Wait(); On 2014/09/12 17:31:09, qinmin wrote: > On 2014/09/12 17:26:22, boliu wrote: > > On 2014/09/12 17:19:48, qinmin wrote: > > > But why cannot we just use a callback and don't block the render thread? > > > > Calling SetVideoFrameProviderClient without the main thread blocked is not > > thread safe right now. Eg there is no lock protecting > > video_frame_provider_client_, stream_texture_proxy_initialized_, etc > > I don't see that's a valid reason to block main thread. > we can just have a variable on main thread checking that a post task is in > progress, and don't access any of the method that has these thread_unsafe > variables in there That is way too complex, it's a super invasive change, and easy for someone to break in the future. What do you propose you should do if those variables are needed on the main thread while the post task is pending? Also I'm not sure if it's legal to use the proxy before it's bound. Compositor already blocks on every single frame. And this only happens on context loss, which is supposedly a rare event to begin with.
On 2014/09/12 17:36:34, boliu wrote: > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:1281: > completion.Wait(); > On 2014/09/12 17:31:09, qinmin wrote: > > On 2014/09/12 17:26:22, boliu wrote: > > > On 2014/09/12 17:19:48, qinmin wrote: > > > > But why cannot we just use a callback and don't block the render thread? > > > > > > Calling SetVideoFrameProviderClient without the main thread blocked is not > > > thread safe right now. Eg there is no lock protecting > > > video_frame_provider_client_, stream_texture_proxy_initialized_, etc > > > > I don't see that's a valid reason to block main thread. > > we can just have a variable on main thread checking that a post task is in > > progress, and don't access any of the method that has these thread_unsafe > > variables in there > > That is way too complex, it's a super invasive change, and easy for someone to > break in the future. > > What do you propose you should do if those variables are needed on the main > thread while the post task is pending? > > Also I'm not sure if it's legal to use the proxy before it's bound. > > Compositor already blocks on every single frame. And this only happens on > context loss, which is supposedly a rare event to begin with. why is that "way too complex"? I am more worried about performance than we just add a variable here. shouldn't stream_texture_proxy_initialized_ help us avoiding all the calls if stream_texture_proxy if not initialized? This is not only context loss, this also happens when the first time video starts playing, which is not that rare.
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/... > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... > > content/renderer/media/android/webmediaplayer_android.cc:1281: > > completion.Wait(); > > On 2014/09/12 17:31:09, qinmin wrote: > > > On 2014/09/12 17:26:22, boliu wrote: > > > > On 2014/09/12 17:19:48, qinmin wrote: > > > > > But why cannot we just use a callback and don't block the render thread? > > > > > > > > Calling SetVideoFrameProviderClient without the main thread blocked is not > > > > thread safe right now. Eg there is no lock protecting > > > > video_frame_provider_client_, stream_texture_proxy_initialized_, etc > > > > > > I don't see that's a valid reason to block main thread. > > > we can just have a variable on main thread checking that a post task is in > > > progress, and don't access any of the method that has these thread_unsafe > > > variables in there > > > > That is way too complex, it's a super invasive change, and easy for someone to > > break in the future. > > > > What do you propose you should do if those variables are needed on the main > > thread while the post task is pending? > > > > Also I'm not sure if it's legal to use the proxy before it's bound. > > > > Compositor already blocks on every single frame. And this only happens on > > context loss, which is supposedly a rare event to begin with. > > why is that "way too complex"? I am more worried about performance than we just > add a variable here. > shouldn't stream_texture_proxy_initialized_ help us avoiding all the calls if > stream_texture_proxy if not initialized? > This is not only context loss, this also happens when the first time video > starts playing, which is not that rare. Besides complexity it might actually be worse for performance to add locks, because those might have to be acquired more often (worst case per frame), while blocking on this task only happens once during initialization (and whenever we lost the context). I *think* CrOS does similar things, for example look at RTCVideoDecoder::Create(). If you really feel strongly about avoiding the blocking PostTask here, I suggest binding lazily from GetCurrentFrame() on the impl thread, but trying to simplify the logic somehow so that we can bind more unconditionally (and don't need locks), such as: GetCurrentFrame() { ... if (!bound_proxy_on_compositor_thread_) proxy_->BindToCurrentThread();
On 2014/09/12 19:28:30, sievers wrote: > 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/... > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > https://codereview.chromium.org/532993002/diff/120001/content/renderer/media/... > > > content/renderer/media/android/webmediaplayer_android.cc:1281: > > > completion.Wait(); > > > On 2014/09/12 17:31:09, qinmin wrote: > > > > On 2014/09/12 17:26:22, boliu wrote: > > > > > On 2014/09/12 17:19:48, qinmin wrote: > > > > > > But why cannot we just use a callback and don't block the render > thread? > > > > > > > > > > Calling SetVideoFrameProviderClient without the main thread blocked is > not > > > > > thread safe right now. Eg there is no lock protecting > > > > > video_frame_provider_client_, stream_texture_proxy_initialized_, etc > > > > > > > > I don't see that's a valid reason to block main thread. > > > > we can just have a variable on main thread checking that a post task is in > > > > progress, and don't access any of the method that has these thread_unsafe > > > > variables in there > > > > > > That is way too complex, it's a super invasive change, and easy for someone > to > > > break in the future. > > > > > > What do you propose you should do if those variables are needed on the main > > > thread while the post task is pending? > > > > > > Also I'm not sure if it's legal to use the proxy before it's bound. > > > > > > Compositor already blocks on every single frame. And this only happens on > > > context loss, which is supposedly a rare event to begin with. > > > > why is that "way too complex"? I am more worried about performance than we > just > > add a variable here. > > shouldn't stream_texture_proxy_initialized_ help us avoiding all the calls if > > stream_texture_proxy if not initialized? > > This is not only context loss, this also happens when the first time video > > starts playing, which is not that rare. > > Besides complexity it might actually be worse for performance to add locks, > because those might have to be acquired more often (worst case per frame), while > blocking on this task only happens once during initialization (and whenever we > lost the context). > I *think* CrOS does similar things, for example look at > RTCVideoDecoder::Create(). > > If you really feel strongly about avoiding the blocking PostTask here, I suggest > binding lazily from GetCurrentFrame() on the impl thread, but trying to simplify > the logic somehow so that we can bind more unconditionally (and don't need > locks), such as: > > GetCurrentFrame() { > ... > if (!bound_proxy_on_compositor_thread_) > proxy_->BindToCurrentThread(); I remember we used to do that in GetCurrentFrame(), something like: if (!stream_texture_proxy_initialized_) { // Bind the streamTexture to compositor thread. } I forgot the reason it was moved out there
How about this:
WMP::SetVideoFrameProviderClient(client) {
// on impl thread
base::AutoLock lock(client_lock_);
DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread());
client_loop_ = MsgLoopProxy::current();
client_ = client;
{
base::AutoLock lock(stream_texture_proxy_creation_lock_);
if (stream_texture_proxy_)
stream_texture_proxy_->SetClient(client_, client_loop_);
}
}
WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
DCHECK(main_thread_checker_.CalledOnValidThread());
// Already created.
if (stream_texture_proxy_)
return;
// No factory to create proxy.
if (!stream_texture_factory_)
return;
if (needs_external_surface_)
return;
{
base::AutoLock lock(stream_texture_proxy_creation_lock_);
stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
}
if (stream_texture_proxy_) {
DoCreateStreamTexture();
ReallocateVideoFrame();
{
base::AutoLock lock(client_lock_);
if (video_frame_provider_client_) {
DCHECK(client_loop_.get());
stream_texture_proxy_->SetClient(video_frame_provider_client_,
client_loop_);
}
}
}
}
class StreamTextureProxy {
- BindToCurrentThread();
- SetClient();
+ SetClient(VFPClient* client, MsgLoopProxy* loop); // merge the two functions
above
}
On 2014/09/12 19:51:08, qinmin wrote:
> I remember we used to do that in GetCurrentFrame(), something like:
> if (!stream_texture_proxy_initialized_) {
> // Bind the streamTexture to compositor thread.
> }
> I forgot the reason it was moved out there
Accessing stream_texture_proxy_initialized_ is not thread safe in
GetCurrentFrame because main thread is not blocked.
On 2014/09/12 20:08:17, boliu wrote:
> On 2014/09/12 19:51:08, qinmin wrote:
> > I remember we used to do that in GetCurrentFrame(), something like:
> > if (!stream_texture_proxy_initialized_) {
> > // Bind the streamTexture to compositor thread.
> > }
> > I forgot the reason it was moved out there
>
> Accessing stream_texture_proxy_initialized_ is not thread safe in
> GetCurrentFrame because main thread is not blocked.
At that time, stream_texture_proxy_initialized_ is only used in
GetCurrentFrame() to initialize it for the first time video is played/
On 2014/09/12 20:12:38, qinmin wrote:
> On 2014/09/12 20:08:17, boliu wrote:
> > On 2014/09/12 19:51:08, qinmin wrote:
> > > I remember we used to do that in GetCurrentFrame(), something like:
> > > if (!stream_texture_proxy_initialized_) {
> > > // Bind the streamTexture to compositor thread.
> > > }
> > > I forgot the reason it was moved out there
> >
> > Accessing stream_texture_proxy_initialized_ is not thread safe in
> > GetCurrentFrame because main thread is not blocked.
>
> At that time, stream_texture_proxy_initialized_ is only used in
> GetCurrentFrame() to initialize it for the first time video is played/
The change was here:
https://codereview.chromium.org/448063003/diff/60001/content/renderer/media/a...
That wasn't the only variable, there were other things.
(That change also broke webview which calls TryCreateStreamTextureProxyIfNeeded
in play after SetVideoFrameProviderClient, which means the proxy is never bound
to a thread)
I think this sounds good to me, we don't need the long if statement inside WMPA::SetVideoFrameProviderClient(client). On Fri, Sep 12, 2014 at 12:56 PM, <sievers@chromium.org> wrote: > How about this: > > WMP::SetVideoFrameProviderClient(client) { > // on impl thread > base::AutoLock lock(client_lock_); > DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread()); > client_loop_ = MsgLoopProxy::current(); > client_ = client; > { > base::AutoLock lock(stream_texture_proxy_creation_lock_); > if (stream_texture_proxy_) > stream_texture_proxy_->SetClient(client_, client_loop_); > } > } > > WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() { > DCHECK(main_thread_checker_.CalledOnValidThread()); > // Already created. > if (stream_texture_proxy_) > return; > > // No factory to create proxy. > if (!stream_texture_factory_) > return; > > if (needs_external_surface_) > return; > > { > base::AutoLock lock(stream_texture_proxy_creation_lock_); > stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy()); > } > > if (stream_texture_proxy_) { > DoCreateStreamTexture(); > ReallocateVideoFrame(); > > { > base::AutoLock lock(client_lock_); > if (video_frame_provider_client_) { > DCHECK(client_loop_.get()); > stream_texture_proxy_->SetClient(video_frame_provider_client_, > client_loop_); > } > } > } > } > > class StreamTextureProxy { > - BindToCurrentThread(); > - SetClient(); > + SetClient(VFPClient* client, MsgLoopProxy* loop); // merge the two > functions > above > } > > > > > > > https://codereview.chromium.org/532993002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/532993002/diff/140001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/140001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1276: base::WaitableEvent completion(true, false); what if I add a "if (video_frame_provider_client_)" here so it this never happens in chrome?
On 2014/09/12 20:16:30, qinmin wrote:
> I think this sounds good to me, we don't need the long if statement inside
> WMPA::SetVideoFrameProviderClient(client).
You mean this if block?
"""
if (client && !stream_texture_proxy_initialized_ && stream_id_ &&
!needs_external_surface_ && !is_remote_) {
"""
I don't how that would work in this propsal that merges SetClient and Bind.
If the condition is really "always bind when set client", and that those
conditions are not needed, then I'll implement this propsal.
>
> On Fri, Sep 12, 2014 at 12:56 PM, <mailto:sievers@chromium.org> wrote:
>
> > How about this:
> >
> > WMP::SetVideoFrameProviderClient(client) {
> > // on impl thread
> > base::AutoLock lock(client_lock_);
> > DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread());
> > client_loop_ = MsgLoopProxy::current();
> > client_ = client;
> > {
> > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > if (stream_texture_proxy_)
> > stream_texture_proxy_->SetClient(client_, client_loop_);
> > }
> > }
> >
> > WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
> > DCHECK(main_thread_checker_.CalledOnValidThread());
> > // Already created.
> > if (stream_texture_proxy_)
> > return;
> >
> > // No factory to create proxy.
> > if (!stream_texture_factory_)
> > return;
> >
> > if (needs_external_surface_)
> > return;
> >
> > {
> > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
> > }
> >
> > if (stream_texture_proxy_) {
> > DoCreateStreamTexture();
> > ReallocateVideoFrame();
> >
> > {
> > base::AutoLock lock(client_lock_);
> > if (video_frame_provider_client_) {
> > DCHECK(client_loop_.get());
> > stream_texture_proxy_->SetClient(video_frame_provider_client_,
> > client_loop_);
> > }
> > }
> > }
> > }
> >
> > class StreamTextureProxy {
> > - BindToCurrentThread();
> > - SetClient();
> > + SetClient(VFPClient* client, MsgLoopProxy* loop); // merge the two
Assumption is SetClient can be called on any thread?
> > functions
> > above
> > }
> >
> >
> >
> >
> >
> >
> > https://codereview.chromium.org/532993002/
> >
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
Yes, I don't think we need to access !needs_external_surface_ and !is_remote_ on
impl thread, we just need the checks on main thread to not call EstablishPeer().
As for the stream_id_, it should always be non-zero unless it is the VIDEO_HOLE
case. But in that case, stream_texture_ should be null, so we won't hit the if
statement.
On 2014/09/12 20:27:59, boliu wrote:
> On 2014/09/12 20:16:30, qinmin wrote:
> > I think this sounds good to me, we don't need the long if statement inside
> > WMPA::SetVideoFrameProviderClient(client).
>
> You mean this if block?
>
> """
> if (client && !stream_texture_proxy_initialized_ && stream_id_ &&
> !needs_external_surface_ && !is_remote_) {
> """
>
> I don't how that would work in this propsal that merges SetClient and Bind.
>
> If the condition is really "always bind when set client", and that those
> conditions are not needed, then I'll implement this propsal.
>
> >
> > On Fri, Sep 12, 2014 at 12:56 PM, <mailto:sievers@chromium.org> wrote:
> >
> > > How about this:
> > >
> > > WMP::SetVideoFrameProviderClient(client) {
> > > // on impl thread
> > > base::AutoLock lock(client_lock_);
> > > DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread());
> > > client_loop_ = MsgLoopProxy::current();
> > > client_ = client;
> > > {
> > > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > > if (stream_texture_proxy_)
> > > stream_texture_proxy_->SetClient(client_, client_loop_);
> > > }
> > > }
> > >
> > > WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
> > > DCHECK(main_thread_checker_.CalledOnValidThread());
> > > // Already created.
> > > if (stream_texture_proxy_)
> > > return;
> > >
> > > // No factory to create proxy.
> > > if (!stream_texture_factory_)
> > > return;
> > >
> > > if (needs_external_surface_)
> > > return;
> > >
> > > {
> > > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > > stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
> > > }
> > >
> > > if (stream_texture_proxy_) {
> > > DoCreateStreamTexture();
> > > ReallocateVideoFrame();
> > >
> > > {
> > > base::AutoLock lock(client_lock_);
> > > if (video_frame_provider_client_) {
> > > DCHECK(client_loop_.get());
> > > stream_texture_proxy_->SetClient(video_frame_provider_client_,
> > > client_loop_);
> > > }
> > > }
> > > }
> > > }
> > >
> > > class StreamTextureProxy {
> > > - BindToCurrentThread();
> > > - SetClient();
> > > + SetClient(VFPClient* client, MsgLoopProxy* loop); // merge the two
>
> Assumption is SetClient can be called on any thread?
>
> > > functions
> > > above
> > > }
> > >
> > >
> > >
> > >
> > >
> > >
> > > https://codereview.chromium.org/532993002/
> > >
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/09/12 20:43:30, qinmin wrote:
> Yes, I don't think we need to access !needs_external_surface_ and !is_remote_
on
> impl thread, we just need the checks on main thread to not call
EstablishPeer().
> As for the stream_id_, it should always be non-zero unless it is the
VIDEO_HOLE
> case. But in that case, stream_texture_ should be null, so we won't hit the if
> statement.
>
>
> On 2014/09/12 20:27:59, boliu wrote:
> > On 2014/09/12 20:16:30, qinmin wrote:
> > > I think this sounds good to me, we don't need the long if statement inside
> > > WMPA::SetVideoFrameProviderClient(client).
> >
> > You mean this if block?
> >
> > """
> > if (client && !stream_texture_proxy_initialized_ && stream_id_ &&
> > !needs_external_surface_ && !is_remote_) {
> > """
> >
> > I don't how that would work in this propsal that merges SetClient and Bind.
> >
> > If the condition is really "always bind when set client", and that those
> > conditions are not needed, then I'll implement this propsal.
> >
> > >
> > > On Fri, Sep 12, 2014 at 12:56 PM, <mailto:sievers@chromium.org> wrote:
> > >
> > > > How about this:
> > > >
> > > > WMP::SetVideoFrameProviderClient(client) {
> > > > // on impl thread
> > > > base::AutoLock lock(client_lock_);
> > > > DCHECK(!client_loop_ || client_loop_.BelongsToCurrentThread());
> > > > client_loop_ = MsgLoopProxy::current();
> > > > client_ = client;
> > > > {
> > > > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > > > if (stream_texture_proxy_)
> > > > stream_texture_proxy_->SetClient(client_, client_loop_);
> > > > }
> > > > }
> > > >
> > > > WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
> > > > DCHECK(main_thread_checker_.CalledOnValidThread());
> > > > // Already created.
> > > > if (stream_texture_proxy_)
> > > > return;
> > > >
> > > > // No factory to create proxy.
> > > > if (!stream_texture_factory_)
> > > > return;
> > > >
> > > > if (needs_external_surface_)
> > > > return;
> > > >
> > > > {
> > > > base::AutoLock lock(stream_texture_proxy_creation_lock_);
> > > > stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
> > > > }
> > > >
> > > > if (stream_texture_proxy_) {
> > > > DoCreateStreamTexture();
> > > > ReallocateVideoFrame();
> > > >
> > > > {
> > > > base::AutoLock lock(client_lock_);
> > > > if (video_frame_provider_client_) {
> > > > DCHECK(client_loop_.get());
> > > > stream_texture_proxy_->SetClient(video_frame_provider_client_,
> > > > client_loop_);
> > > > }
> > > > }
> > > > }
> > > > }
> > > >
> > > > class StreamTextureProxy {
> > > > - BindToCurrentThread();
> > > > - SetClient();
> > > > + SetClient(VFPClient* client, MsgLoopProxy* loop); // merge the two
> >
> > Assumption is SetClient can be called on any thread?
> >
> > > > functions
> > > > above
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/532993002/
> > > >
> > >
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
That'd be good if we can simplify a bit. I think in general:
- GL texture for stream and proxy should have the same lifetime; maybe some of
that code can be merged too;
maybe the proxy can take the stream_id into the constructor/factory
- the client set on the proxy should always be up-to-date (WMP::client_ always
equal to proxy::client_)
- i don't think we really ever want to destroy the proxy, because the compositor
needs to be able to get a frame, i.e. when going into fullscreen we should
definitely keep the ST around for coming back (same as when a tab becomes
backgrounded; disconnecting the ST from media player takes care of freeing
unneeded buffers). even if we have an external surface it doesn't matter if we
create a stream texture + proxy it doesn't matter, because no buffer will ever
get allocated as long as we don't connect the surface to the media player.
similarly setting a client/listener on a proxy that's not connected will never
trigger any frame updates.
Patchset #10 (id:180001) has been deleted
Ok. You can look at the diff between PS9 and PS10 to see what's needed to avoid blocking. I think it's pretty clear that's a way worse change. Requires sprinkling locks everywhere, also holding the lock while calling into other functions, all to achieve this fuzzy feeling "hey, we are not blocking the main thread, because waiting on a lock is not blocking!"
https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/... 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 is called when the main thread is blocked. So I am wondering whether we really need the client_lock_ here since the WMPA dtor won't be called at the same time when this function is called on the impl thread. Or will webview call this function at non-commit time?
https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/200001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1230: base::AutoLock lock(client_lock_); On 2014/09/13 23:49:44, qinmin wrote: > In video_frame_provider_client_impl.cc, it mentioned that this call is called > when the main thread is blocked. > So I am wondering whether we really need the client_lock_ here since the WMPA > dtor won't be called at the same time when this function is called on the impl > thread. > Or will webview call this function at non-commit time? Daniel's pseudo code threw me off. Yeah I don't think it's needed.
Removed that lock. Didn't test it, but looks a lot saner.
https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:74: delete this; Hmm, this crashes in lock because lock can't be deleted while it's held. Technically lock only protects client_ and loop_. The fact that BindToLoop can't be called from another thread is not guaranteed in this class. So could add a AutoUnlock here. Barrrr
https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:55: if (!loop_.get() || loop_.get() != base::MessageLoopProxy::current() || so if loop_.get() != base::MessageLoopProxy::current(), we will call "delete this" instead of loop_->DeleteSoon(FROM_HERE, this)? https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:69: if (loop.get() != base::MessageLoopProxy::current()) { It doesn't seem to me that we will ever enter this if statement/ Put a NOT_REACHED() here and we can remove that BindOnThread call. https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy()); Please do the following before this to fix the VIDEO_HOLE codepath: if (!needs_establish_peer_) return; https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.h (left): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:417: bool stream_texture_proxy_initialized_; Is this variable still needed?
Ok, so StreamTextureProxyImpl threading correctness depends on the it must be used with ScopedStreamTextureProxy, which means can depend on Release being the destructor, ie no other things can reference it after Release is called. This is the existing behavior already, so I guess it ain't that bad.. https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:55: if (!loop_.get() || loop_.get() != base::MessageLoopProxy::current() || On 2014/09/15 17:16:26, qinmin wrote: > so if loop_.get() != base::MessageLoopProxy::current(), we will call "delete > this" instead of loop_->DeleteSoon(FROM_HERE, this)? Done. https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:69: if (loop.get() != base::MessageLoopProxy::current()) { On 2014/09/15 17:16:26, qinmin wrote: > It doesn't seem to me that we will ever enter this if statement/ > Put a NOT_REACHED() here and we can remove that BindOnThread call. Umm....webview will get here, but webview doesn't use this instance of proxy. I think we should keep it.. https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1281: stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy()); On 2014/09/15 17:16:26, qinmin wrote: > Please do the following before this to fix the VIDEO_HOLE codepath: > > if (!needs_establish_peer_) > return; > Done. https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.h (left): https://codereview.chromium.org/532993002/diff/240001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:417: bool stream_texture_proxy_initialized_; On 2014/09/15 17:16:26, qinmin wrote: > Is this variable still needed? Removed
https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:90: scoped_refptr<base::MessageLoopProxy> loop) { DCHECK(!client || (client && loop)); DCHECK(!loop_ || (loop == loop_)); https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:92: SetClient(client); move this to BindToLoop() https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1222: if (stream_texture_proxy_ && client) { need to handle client == NULL also https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1224: stream_id_, client, base::MessageLoopProxy::current()); Call BindToLoop() before StopUsingProvider()
PTAL https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_impl.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:90: scoped_refptr<base::MessageLoopProxy> loop) { On 2014/09/15 19:00:39, sievers wrote: > DCHECK(!client || (client && loop)); Actually easier to just DCHECK(loop) in this case imo. Caller have to always supply a loop. > DCHECK(!loop_ || (loop == loop_)); Done https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_impl.cc:92: SetClient(client); On 2014/09/15 19:00:40, sievers wrote: > move this to BindToLoop() Done. https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1222: if (stream_texture_proxy_ && client) { On 2014/09/15 19:00:40, sievers wrote: > need to handle client == NULL also Done. https://codereview.chromium.org/532993002/diff/280001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1224: stream_id_, client, base::MessageLoopProxy::current()); On 2014/09/15 19:00:40, sievers wrote: > Call BindToLoop() before StopUsingProvider() Done. Realized can't call MLP::current() here https://codereview.chromium.org/532993002/diff/320001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/532993002/diff/320001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1287: DoCreateStreamTexture(); I accidentally dropped these two lines, although amazingly that didn't appear to break anything. Anyway, added them back and wanted to call it out.
lgtm
lgtm https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... content/renderer/media/android/stream_texture_factory.h:49: ~StreamTextureFactoryContextObserver() {} virtual https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:175: context_provider_->AddObserver(observer_); Beware: ObserverList has a NOTREACHED() << "Observers can only be added once!";
https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... content/renderer/media/android/stream_texture_factory.h:49: ~StreamTextureFactoryContextObserver() {} On 2014/09/17 01:29:52, sievers wrote: > virtual Woo, good catch. Done! https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory_synchronous_impl.cc (right): https://codereview.chromium.org/532993002/diff/330001/content/renderer/media/... content/renderer/media/android/stream_texture_factory_synchronous_impl.cc:175: context_provider_->AddObserver(observer_); On 2014/09/17 01:29:52, sievers wrote: > Beware: ObserverList has a NOTREACHED() << "Observers can only be added once!"; Should be fine, context_provider_ is created only once.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/532993002/350001
The CQ bit was unchecked by commit-bot@chromium.org
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...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/532993002/370001
Message was sent while issue was closed.
Committed patchset #19 (id:370001) as 8674c1c863fe3bbd79c59811f2395e61f5070dd5
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/7a2b1cb83546418638417fc4d3c9aa6172fe02e9 Cr-Commit-Position: refs/heads/master@{#295219}
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 |
