Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27420004:
Remove threading from RendererGpuVideoAcceleratorFactories (Closed)
|
Created:
7 years, 2 months ago by sheu Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, darin-cc_chromium.org, wuchengli, scherkus (not reviewing), brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@dthread Visibility:
Public. |
DescriptionThis change removes all the threading considerations from
GpuVideoAcceleratorFactories (and its implementation,
RendererGpuVideoAcceleratorFactories). Most notably, it removes Abort() and
associated functions and state. And with the removal of Abort() and friends,
we can also remove its Clone() interface.
All of the previously abortable operations on the RGVAF (with the exception of
ReadPixels()) can be made non-abortable, with no functional difference, due to
the way the users of RGVAF function. These three users are
WebMediaPlayerImpl/GpuVideoDecoder, RTCVideoDecoder, and RTCVideoEncoder, and
they can be made non-abortable because:
WebMediaPlayerImpl/GpuVideoDecoder:
* Abort() is called from WebMediaPlayerImpl::Destroy(). It has no effect, as:
* All the RGVAF entry points are called from the the RGVAF message loop
from GpuVideoDecoder (except for ReadPixels()), so the Abort() has no
effect on them.
RTCVideoDecoder:
* Abort() is called from RTCVideoDecoder::WillDestroyCurrentMessageLoop() for
the RGVAF message loop. It has no effect, as:
* Amost all the RGVAF entry points are called from the RGVAF message loop
(except for ReadPixels()), so Abort() has no effect on them.
* The other exception is CreateVideoDecodeAccelerator(), which is called from
RTC's main thread. But as the Abort() is called from
WillDestroyCurrentMessageLoop() for the RGVAF message loop itself, it is
guaranteed to occur after any tasks posted to the RGVAF message loop by
CreateVideoDecodeAccelerator() has completed, and so the Abort() has no
effect.
RTCVideoEncoder:
* Abort() is called from RTCVideoDecoder::Release(). It has no effect, as:
* All the RGVAF entry points are called from the RGVAF message loop.
The only functional difference remaining is that making ReadPixels()
non-abortable. This is preferable, as as long as a completed video accelerator
texture is available, it should be readable. We also specify that all calls to
ReadPixels must be made on the RGVAF message loop, like all other entry points,
and leave it up to the users of ReadPixels() to handle thread trampolining if
necessary.
BUG=306333
TEST=local build, run on CrOS snow; build, run unittests on desktop Linux
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247480
Patch Set 1 : 51676bf4 Initial. #
Total comments: 19
Patch Set 2 : 282a294f Rebase, rework, #
Total comments: 17
Patch Set 3 : 7d21c918 Updated. #Patch Set 4 : 2d3efbfa Rebase. #
Total comments: 4
Patch Set 5 : f77fc38a Unittest fixes. #
Total comments: 5
Patch Set 6 : d731420c Comment update, 'git cl format', rebase #
Total comments: 7
Patch Set 7 : faaba9c2 Removed BindToLoopSync #Patch Set 8 : 82aa015b Final rebase. #Patch Set 9 : db94efb8 Clean version at ToT. #Patch Set 10 : cf596ded Rebase. #Messages
Total messages: 66 (0 generated)
fischman@: the big one. PTAL. (Doesn't have to be right now; it ain't blocking M31...) https://chromiumcodereview.appspot.com/27420004/diff/3001/media/filters/gpu_v... File media/filters/gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/media/filters/gpu_v... media/filters/gpu_video_accelerator_factories.h:29: // called from any thread. I'm still a little iffy about this. My other thought is to make ReadPixels also require call on the factory message loop, and to make the user of the GpuVideoAcceleratorFactories handle the synchronous posting. This would make the interface here cleaner, but unfortunately require that we duplicate the synchronous posting bit to two locations (see: RTCVideoDecoder, GpuVideoDecoder).
- rtc_video_decoder_unittest.cc calls Abort() and needs updated. - There's a typo in the change description. s/RTCVideoDecoder::Release()/RTCVideoEecoder::Release()/ What thread calls read_pixels_cb_? As long as the media thread does not block on that thread ever, it is OK. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_decoder.cc:628: *vda = factories_->CreateVideoDecodeAccelerator(profile, this); Just use |vda_| here and remove the parameter? https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_decoder_factory.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_decoder_factory.cc:30: // factories here. Clone one instead. Remove the comment. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_encoder_factory.cc:89: // cannot create a new factory, clone one instead. Remove comment.
https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:43: // keeps us alive until this task completes. That the comment is a lie is a problem. The Unretained() in the next line is made unsafe by the lack of a wait following the post (deleted l.48). https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:164: void RendererGpuVideoAcceleratorFactories::ReadPixels(uint32 texture_id, I believe this is called from the compositing thread (or the renderer's main/Child thread if the threaded compositor is not enabled). I don't think the conditional trampoline is necessary (this should never be getting called from the media thread). https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:177: waiter.Wait(); This is only deadlock-safe if the media thread is guaranteed never to be waiting on the compositing/render thread. It used to be that CreateSharedMemory would be that deadlock-causer, because it trampolined to the main thread from the media thread, but that's no longer the case. I *think* this is safe now, but I am not _confident_ it the safety :( https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. I fail to understand the "proof" in the CL description. Would you mind rewriting it? https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:37: // the constructor-argument loop (the media thread). Paragraph is a lie. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:101: std::vector<gpu::Mailbox> created_texture_mailboxes_; Garbage https://chromiumcodereview.appspot.com/27420004/diff/3001/media/filters/gpu_v... File media/filters/gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/media/filters/gpu_v... media/filters/gpu_video_accelerator_factories.h:29: // called from any thread. On 2013/10/16 00:57:45, sheu wrote: > I'm still a little iffy about this. My other thought is to make ReadPixels also > require call on the factory message loop, and to make the user of the > GpuVideoAcceleratorFactories handle the synchronous posting. This would make > the interface here cleaner, but unfortunately require that we duplicate the > synchronous posting bit to two locations (see: RTCVideoDecoder, > GpuVideoDecoder). An alternative: make ReadPixels callable from only one thread, but make that thread be the render/compositor thread (the one where ReadPixels as actually called on!). I think the only thing in ReadPixels' impl today that requires it to run on the media thread is that it uses a context3d that's bound to that thread. I wonder if it makes sense to give GVAF an additional context3d, for using on the compositing thread? That would allow zero trampolining, either inside RGVAF or in its callers, for ReadPixels.
https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:43: // keeps us alive until this task completes. On 2013/10/17 22:22:17, Ami Fischman wrote: > That the comment is a lie is a problem. > The Unretained() in the next line is made unsafe by the lack of a wait following > the post (deleted l.48). Done. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:164: void RendererGpuVideoAcceleratorFactories::ReadPixels(uint32 texture_id, On 2013/10/17 22:22:17, Ami Fischman wrote: > I believe this is called from the compositing thread (or the renderer's > main/Child thread if the threaded compositor is not enabled). > I don't think the conditional trampoline is necessary (this should never be > getting called from the media thread). Gonna refactor the trampolines out of this class. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/10/17 22:22:17, Ami Fischman wrote: > I fail to understand the "proof" in the CL description. Would you mind > rewriting it? Heh, sure. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:37: // the constructor-argument loop (the media thread). On 2013/10/17 22:22:17, Ami Fischman wrote: > Paragraph is a lie. It is now. FWIW: this CL is based off the previous partial de-threading of the RGVAF. The comments are updated in the next version. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/renderer_gpu_video_accelerator_factories.h:101: std::vector<gpu::Mailbox> created_texture_mailboxes_; On 2013/10/17 22:22:17, Ami Fischman wrote: > Garbage Removed with rebase. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_decoder.cc:628: *vda = factories_->CreateVideoDecodeAccelerator(profile, this); On 2013/10/16 07:43:17, wuchengli wrote: > Just use |vda_| here and remove the parameter? Done. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_decoder_factory.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_decoder_factory.cc:30: // factories here. Clone one instead. On 2013/10/16 07:43:17, wuchengli wrote: > Remove the comment. Done. https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/3001/content/renderer/me... content/renderer/media/rtc_video_encoder_factory.cc:89: // cannot create a new factory, clone one instead. On 2013/10/16 07:43:17, wuchengli wrote: > Remove comment. Done.
- rtc_video_decoder_unittest.cc calls Abort() and needs updated. - There's a typo in the change description. s/RTCVideoDecoder::Release()/RTCVideoEecoder::Release()/ https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:39: // Wait for the context to be acquired. Update or remove the comment. This doesn't wait anymore. https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/rtc_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/rtc_video_decoder.cc:20: #include "third_party/skia/include/core/SkBitmap.h" Is this required? https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... File media/filters/gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... media/filters/gpu_video_accelerator_factories.h:63: // |ReadPixels()|. Update the comment because ReadPixels() also has to be called on the loop.
https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:35: if (message_loop_->BelongsToCurrentThread()) { This is never the case, right? But I guess you want to maintain the flexibility of the ctor being callable on any thread to allow either creation on the compositing thread (for GVD) or the libjingle worker thread (for RVD)? https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:42: base::Bind(&RendererGpuVideoAcceleratorFactories::AsyncBindContext, Would it make sense to have RenderThreadImpl::GetGpuFactories() PostTask to the media thread to BindToCurrentThread instead of monkeying around with conditional trampolining here? https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.h:35: // but subsequent calls to most public methods of the class must be called from s/most/all/ https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... File media/base/bind_to_loop.h.pump (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... media/base/bind_to_loop.h.pump:114: DCHECK(!loop->BelongsToCurrentThread()); This wants to live inside RunSync, not here, right? (it should be fine to bind a closure to the current loop, for synchronous execution, where the outermost Run() (and thus Wait()) is happening on another thread) https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... media/base/bind_to_loop.h.pump:119: static base::Callback<T> BindToCurrentLoop( ...which gives rise to wondering whether there's going to be a need for a BindToCurrentLoopSync someday. But apparently that day is not today, so no need to add this :) https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... File media/filters/gpu_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... media/filters/gpu_video_decoder.cc:445: factories_->GetMessageLoop(), base::Bind( Does it make for nicer callsites to invert the relationship between BindToLoopSync and GVAF::RP here? In other words, give GVAF a method that acts as a ReadPixelCB factory and call it here with no mention of bind or GetMessageLoop(). That would allow you to contain both impl details (BindToLoopSync & the need to use factories_->GetMessageLoop()) in the impl of GVAF instead of forcing them to bleed into the caller.
Updated. PTAL https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:35: if (message_loop_->BelongsToCurrentThread()) { On 2013/10/22 17:21:19, Ami Fischman wrote: > This is never the case, right? > But I guess you want to maintain the flexibility of the ctor being callable on > any thread to allow either creation on the compositing thread (for GVD) or the > libjingle worker thread (for RVD)? Superseded (see below) https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:39: // Wait for the context to be acquired. On 2013/10/21 05:59:01, wuchengli wrote: > Update or remove the comment. This doesn't wait anymore. Superseded (see below) https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:42: base::Bind(&RendererGpuVideoAcceleratorFactories::AsyncBindContext, On 2013/10/22 17:21:19, Ami Fischman wrote: > Would it make sense to have RenderThreadImpl::GetGpuFactories() PostTask to the > media thread to BindToCurrentThread instead of monkeying around with conditional > trampolining here? I'll do that actually. And along the way undo a previous change suggested by scherkus@ :-P https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.h:35: // but subsequent calls to most public methods of the class must be called from On 2013/10/22 17:21:19, Ami Fischman wrote: > s/most/all/ Done. https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/rtc_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/rtc_video_decoder.cc:20: #include "third_party/skia/include/core/SkBitmap.h" On 2013/10/21 05:59:01, wuchengli wrote: > Is this required? Unfortunately, yes. BindToLoopSync() requires that even its const ref parameters be defined. https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... File media/base/bind_to_loop.h.pump (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... media/base/bind_to_loop.h.pump:114: DCHECK(!loop->BelongsToCurrentThread()); On 2013/10/22 17:21:19, Ami Fischman wrote: > This wants to live inside RunSync, not here, right? > (it should be fine to bind a closure to the current loop, for synchronous > execution, where the outermost Run() (and thus Wait()) is happening on another > thread) That's true. Done.
Updated. PTAL https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:35: if (message_loop_->BelongsToCurrentThread()) { On 2013/10/22 17:21:19, Ami Fischman wrote: > This is never the case, right? > But I guess you want to maintain the flexibility of the ctor being callable on > any thread to allow either creation on the compositing thread (for GVD) or the > libjingle worker thread (for RVD)? Superseded (see below) https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:39: // Wait for the context to be acquired. On 2013/10/21 05:59:01, wuchengli wrote: > Update or remove the comment. This doesn't wait anymore. Superseded (see below) https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:42: base::Bind(&RendererGpuVideoAcceleratorFactories::AsyncBindContext, On 2013/10/22 17:21:19, Ami Fischman wrote: > Would it make sense to have RenderThreadImpl::GetGpuFactories() PostTask to the > media thread to BindToCurrentThread instead of monkeying around with conditional > trampolining here? I'll do that actually. And along the way undo a previous change suggested by scherkus@ :-P https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/renderer_gpu_video_accelerator_factories.h:35: // but subsequent calls to most public methods of the class must be called from On 2013/10/22 17:21:19, Ami Fischman wrote: > s/most/all/ Done. https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... File content/renderer/media/rtc_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/content/renderer/m... content/renderer/media/rtc_video_decoder.cc:20: #include "third_party/skia/include/core/SkBitmap.h" On 2013/10/21 05:59:01, wuchengli wrote: > Is this required? Unfortunately, yes. BindToLoopSync() requires that even its const ref parameters be defined. https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... File media/base/bind_to_loop.h.pump (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/base/bind_to... media/base/bind_to_loop.h.pump:114: DCHECK(!loop->BelongsToCurrentThread()); On 2013/10/22 17:21:19, Ami Fischman wrote: > This wants to live inside RunSync, not here, right? > (it should be fine to bind a closure to the current loop, for synchronous > execution, where the outermost Run() (and thus Wait()) is happening on another > thread) That's true. Done.
Updated. PTAL
https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... File media/filters/gpu_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... media/filters/gpu_video_decoder.cc:445: factories_->GetMessageLoop(), base::Bind( On 2013/10/22 17:21:19, Ami Fischman wrote: > Does it make for nicer callsites to invert the relationship between > BindToLoopSync and GVAF::RP here? In other words, give > GVAF a method that acts as a ReadPixelCB factory and call it here with no > mention of bind or GetMessageLoop(). > > That would allow you to contain both impl details (BindToLoopSync & the need to > use factories_->GetMessageLoop()) in the impl of GVAF instead of forcing them to > bleed into the caller. Did you miss this comment?
https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... File media/filters/gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... media/filters/gpu_video_accelerator_factories.h:63: // |ReadPixels()|. Remove ReadPixels exception. https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... media/filters/gpu_video_accelerator_factories.h:65: You probably missed the previous comment. rtc_video_decoder_unittest.cc uses Abort() and should be updated.
Updated. PTAL https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... File media/filters/gpu_video_decoder.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/14001/media/filters/gpu_... media/filters/gpu_video_decoder.cc:445: factories_->GetMessageLoop(), base::Bind( Right. I'd like to avoid making RGVAF have to deal with posting tasks at all, and put the onus on the users of the class to make sure it's being used on the right thread. Users are already aware that RGVAF has to be used on factories_->GetMessageLoop(), and if they need to do something special to make sure this applies to ReadPixels(), they shoudl do it explicitly. Put another way: the threading assumptions are already the responsibility of the caller. Hiding it behind a factory would likely require a BelongsToCurrentThread() to conditionally-trampoline, which I think is icky. I'd rather have the case where the client is aware of which thread it's executing from, and which thread the RGVAF interface must be posted to. https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... File media/filters/gpu_video_accelerator_factories.h (right): https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... media/filters/gpu_video_accelerator_factories.h:63: // |ReadPixels()|. On 2013/10/24 03:17:20, wuchengli wrote: > Remove ReadPixels exception. Done. https://chromiumcodereview.appspot.com/27420004/diff/314001/media/filters/gpu... media/filters/gpu_video_accelerator_factories.h:65: On 2013/10/24 03:17:20, wuchengli wrote: > You probably missed the previous comment. rtc_video_decoder_unittest.cc uses > Abort() and should be updated. Done.
LGTM. I didn't review bind_to_loop* files.
Only the bindtoloopsync comment (request for doco) is required; the rest are optional or things to think about. Assuming you update doco, LGTM. https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... File content/renderer/media/rtc_video_encoder_factory.h (right): https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... content/renderer/media/rtc_video_encoder_factory.h:16: Here and elsewhere the newlines seem bad to me, but whatever. https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... File content/renderer/render_thread_impl.cc (right): https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... content/renderer/render_thread_impl.cc:894: base::IgnoreResult(&cc::ContextProvider::BindToCurrentThread), I wonder if that IgnoreResult should be a CheckOnFailure or at least a SpamErrorLogOnFailure or something. Not sure what you'd do with the knowledge though. https://chromiumcodereview.appspot.com/27420004/diff/364001/media/base/bind_t... File media/base/bind_to_loop.h.pump (right): https://chromiumcodereview.appspot.com/27420004/diff/364001/media/base/bind_t... media/base/bind_to_loop.h.pump:28: // Typical usage: request to be called back on the current thread: Where's my requested doco update to 'splain the new Sync magic?
https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... File content/renderer/media/rtc_video_encoder_factory.h (right): https://chromiumcodereview.appspot.com/27420004/diff/364001/content/renderer/... content/renderer/media/rtc_video_encoder_factory.h:16: On 2013/10/26 19:30:26, Ami Fischman wrote: > Here and elsewhere the newlines seem bad to me, but whatever. I seemed to remember that we newlined curly brackets for namespace. Looked back on the style guide to make sure -- and even the style guide is inconsistent on this, in the examples. (There's no explicit rule). I guess the rule then is "whatever the file is already using". https://chromiumcodereview.appspot.com/27420004/diff/364001/media/base/bind_t... File media/base/bind_to_loop.h.pump (right): https://chromiumcodereview.appspot.com/27420004/diff/364001/media/base/bind_t... media/base/bind_to_loop.h.pump:28: // Typical usage: request to be called back on the current thread: On 2013/10/26 19:30:26, Ami Fischman wrote: > Where's my requested doco update to 'splain the new Sync magic? Done.
jamesr@: PTAL at content/renderer/render_thread_imp.cc; OWNERS check
LGTM
content/renderer/render_thread_impl.cc lgtm. I'm not sure about whether the bind primitives belong in media/base - adding jam/brett for comment.
They're not in base/ since apparently there was some discussion about them not belonging in base/, when they were first implemented. Ask fischman@ for the story.
On Mon, Oct 28, 2013 at 12:24 PM, <sheu@chromium.org> wrote: > They're not in base/ since apparently there was some discussion about them > not > belonging in base/, when they were first implemented. Ask fischman@ for > the > story. There's not much of a story; in July '11 fakalin/willchan/darin/ajwong had a thread that included the BindToLoop idea, then in Aug '12 I landed BindToLoop in media/ b/c I forgot the above thread, and then a couple of months after that I remembered the '11 thread and started another thread with the above usual suspects in Oct '12 asking whether they'd like me to move BindToLoop into base/. That thread petered out with no conclusion, so no action precipitated. If any base/OWNERS would like to see BindToLoop in base/ I'm happy to send a CL. > https://chromiumcodereview.**appspot.com/27420004/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... content/renderer/media/rtc_video_decoder.cc:134: waiter.Wait(); is this running on the main thread? https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.h File media/base/bind_to_loop.h (right): https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.... media/base/bind_to_loop.h:305: static base::Callback<T> BindToLoopSync( I'm concerned about this. This makes it easy for code to block one message loop on another, which is an anti-pattern. I guess if a background thread is blocked on another one, that may be ok. But what guarantee do we have that people won't use this on the UI or IO threads (in the browser) or on the main renderer thread? I'm also concerned that we have this parallel base utilities for bind stuff. Having multiple places to look for base-y things seems confusing and will lead to duplication.
https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... content/renderer/media/rtc_video_decoder.cc:134: waiter.Wait(); On 2013/10/28 20:11:08, jam wrote: > is this running on the main thread? It's running on WebRtc's main thread, which I believe is the same as the main thread. To fix this would require fixing WebRtc's threading model -- which is a nontrivial change and not in the scope here. For what it's worth it's not introducing new blocking -- it's just moving the blocking out from RendererGpuVideoAcceleratorFactories::CreateVideoDecodeAccelerator(), where it was previously being done. https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.h File media/base/bind_to_loop.h (right): https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.... media/base/bind_to_loop.h:305: static base::Callback<T> BindToLoopSync( On 2013/10/28 20:11:08, jam wrote: > I'm concerned about this. This makes it easy for code to block one message loop > on another, which is an anti-pattern. I guess if a background thread is blocked > on another one, that may be ok. But what guarantee do we have that people won't > use this on the UI or IO threads (in the browser) or on the main renderer > thread? There's no guarantee, other than a general exhortation not to. Is this danger sufficient to make BindToLoopSync() something that Should Not Be Done? > I'm also concerned that we have this parallel base utilities for bind stuff. > Having multiple places to look for base-y things seems confusing and will lead > to duplication. That depends on the discussion previously on whether this has the approvals to be put in base/. It probably could, but I think that should be a separate change where BindToLoop() goes into base/ simultaneously.
https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... content/renderer/media/rtc_video_decoder.cc:134: waiter.Wait(); On 2013/10/28 20:32:58, sheu wrote: > On 2013/10/28 20:11:08, jam wrote: > > is this running on the main thread? > > It's running on WebRtc's main thread, which I believe is the same as the main > thread. No, it's not. RTCVideoDecoder::Create() is called on Chrome_libJingle_WorkerThread (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...) which is a background thread dedicated to libjingle.
https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/27420004/diff/494001/content/renderer/media/r... content/renderer/media/rtc_video_decoder.cc:134: waiter.Wait(); On 2013/10/28 20:38:22, Ami Fischman wrote: > On 2013/10/28 20:32:58, sheu wrote: > > On 2013/10/28 20:11:08, jam wrote: > > > is this running on the main thread? > > > > It's running on WebRtc's main thread, which I believe is the same as the main > > thread. > > No, it's not. RTCVideoDecoder::Create() is called on > Chrome_libJingle_WorkerThread > (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...) > which is a background thread dedicated to libjingle. ah ok, good to hear, thanks https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.h File media/base/bind_to_loop.h (right): https://codereview.chromium.org/27420004/diff/494001/media/base/bind_to_loop.... media/base/bind_to_loop.h:305: static base::Callback<T> BindToLoopSync( On 2013/10/28 20:32:58, sheu wrote: > On 2013/10/28 20:11:08, jam wrote: > > I'm concerned about this. This makes it easy for code to block one message > loop > > on another, which is an anti-pattern. I guess if a background thread is > blocked > > on another one, that may be ok. But what guarantee do we have that people > won't > > use this on the UI or IO threads (in the browser) or on the main renderer > > thread? > > There's no guarantee, other than a general exhortation not to. Is this danger > sufficient to make BindToLoopSync() something that Should Not Be Done? If there's one thing I've learnt on Chrome, it's introducing a way to shoot yourself in the foot will be abused much faster than could ever be expected. So this is why I'm opposed to have a generic functionality to block one thread on another. Or, at the very least, we should only do this after we have a mechanism to enable it only for specific (background) threads, and to have a way that ensures that this never gets allowed for main/IO threads. > > > I'm also concerned that we have this parallel base utilities for bind stuff. > > Having multiple places to look for base-y things seems confusing and will lead > > to duplication. > > That depends on the discussion previously on whether this has the approvals to > be put in base/. It probably could, but I think that should be a separate > change where BindToLoop() goes into base/ simultaneously.
On Mon, Oct 28, 2013 at 1:43 PM, <jam@chromium.org> wrote: > So this is why I'm opposed to have a generic functionality to block one > thread on another. Or, at the very least, we should only do this after > we have a mechanism to enable it only for specific (background) threads, > and to have a way that ensures that this never gets allowed for main/IO > threads. > Does base::ThreadRestrictions::AssertWaitAllowed() not already do exactly this? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/28 20:55:53, Ami Fischman wrote: > On Mon, Oct 28, 2013 at 1:43 PM, <mailto:jam@chromium.org> wrote: > > > So this is why I'm opposed to have a generic functionality to block one > > thread on another. Or, at the very least, we should only do this after > > we have a mechanism to enable it only for specific (background) threads, > > and to have a way that ensures that this never gets allowed for main/IO > > threads. > > > > Does base::ThreadRestrictions::AssertWaitAllowed() not already do exactly > this? afaik this is only set in the browser process
On 2013/10/28 22:17:19, jam wrote: > On 2013/10/28 20:55:53, Ami Fischman wrote: > > On Mon, Oct 28, 2013 at 1:43 PM, <mailto:jam@chromium.org> wrote: > > > > > So this is why I'm opposed to have a generic functionality to block one > > > thread on another. Or, at the very least, we should only do this after > > > we have a mechanism to enable it only for specific (background) threads, > > > and to have a way that ensures that this never gets allowed for main/IO > > > threads. > > > > Does base::ThreadRestrictions::AssertWaitAllowed() not already do exactly > > this? > > afaik this is only set in the browser process Add DisallowWaiting() to IO threads in general, and the renderer main thread in particular? I don't see a good automatic enforcement mechanism other than ThreadRestrictions at this point, but I don't see excessive danger in BindToLoopSync() either. I figure that the name doesn't hide the fact that it's synchronous and implies waiting, so using this on a main/IO thread is as obviously bad as explicitly using a Wait(). The bar for using one is not lower than the bar for using the other.
On 2013/10/31 20:07:32, sheu wrote: > On 2013/10/28 22:17:19, jam wrote: > > On 2013/10/28 20:55:53, Ami Fischman wrote: > > > On Mon, Oct 28, 2013 at 1:43 PM, <mailto:jam@chromium.org> wrote: > > > > > > > So this is why I'm opposed to have a generic functionality to block one > > > > thread on another. Or, at the very least, we should only do this after > > > > we have a mechanism to enable it only for specific (background) threads, > > > > and to have a way that ensures that this never gets allowed for main/IO > > > > threads. > > > > > > Does base::ThreadRestrictions::AssertWaitAllowed() not already do exactly > > > this? > > > > afaik this is only set in the browser process > > Add DisallowWaiting() to IO threads in general, and the renderer main thread in > particular? yeah you can do this in ChildThread and ChildProcess (for IO thread) > > I don't see a good automatic enforcement mechanism other than ThreadRestrictions > at this point, Those are good enough, just right now they're not done in child processes. So without enforcing them, someone can wait. > but I don't see excessive danger in BindToLoopSync() either. I > figure that the name doesn't hide the fact that it's synchronous and implies > waiting, so using this on a main/IO thread is as obviously bad as explicitly > using a Wait(). The bar for using one is not lower than the bar for using the > other. The one issue is that practically speaking, having utilities in a globally visible header to do this will make it more likely that someone will call it. Can you hide these methdos somehow? They should only be used by code in media. If they're used by outside code, then at that point they would need to be in base.
jam: the utilities are already in media/base (not base/) and in the media:: namespace (not base::). To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/31 22:26:39, jam wrote: > On 2013/10/31 20:07:32, sheu wrote: > > > > Add DisallowWaiting() to IO threads in general, and the renderer main thread > in > > particular? > > yeah you can do this in ChildThread and ChildProcess (for IO thread) I guess my question is: should I do this? (In another CL perhaps)
jam@: would it be sufficient to add threading restrictions in another CL?
On 2013/11/01 23:55:33, sheu wrote: > jam@: would it be sufficient to add threading restrictions in another CL? I understand the utilities are in media/base, but there's nothing stopping any code in content, chrome, components from using these. So given past abuses of waiting in the chrome code base, I'm worried about future misuse of this code which makes an anti-pattern (one thread waiting on another) very easy to do. I'm not familiar with the media code, can you explain why a thread needs to wait on another? Is there absolutely no way this can be done in another manner?
On 2013/11/04 20:09:22, jam wrote: > On 2013/11/01 23:55:33, sheu wrote: > > jam@: would it be sufficient to add threading restrictions in another CL? > > I understand the utilities are in media/base, but there's nothing stopping any > code in content, chrome, components from using these. So given past abuses of > waiting in the chrome code base, I'm worried about future misuse of this code > which makes an anti-pattern (one thread waiting on another) very easy to do. > > I'm not familiar with the media code, can you explain why a thread needs to wait > on another? Is there absolutely no way this can be done in another manner? The video accelerator hardware support runs its 3D context on the renderer process's dedicated media thread. libjingle (the WebRTC interface stuff) runs on its own worker thread, and when it needs to do readback from a texture allocated by the video accelerator, it needs to do this readback (1) on the media 3D context, which runs on the media thread, and (2) in a blocking manner. IWBRN (it would be really nice) to have libjingle/webrtc's worker thread move to the media thread, in which case we could be rid of this blocking business. But that would be a pretty big change, and also for later.
fischman@ corrected me: readpixels happens from (and needs to block) the render thread, not the libjingle thread. The requirement for blocking comes from the JS API in question: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
fischman@ corrected me: readpixels happens from (and needs to block) the render thread, not the libjingle thread. The requirement for blocking comes from the JS API in question: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen...
On 2013/11/04 23:06:31, sheu wrote: > fischman@ corrected me: > > readpixels happens from (and needs to block) the render thread, not the > libjingle thread. The requirement for blocking comes from the JS API in > question: > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-elemen... hmm, I still don't follow. Can you please describe in detail why the renderer thread is blocked?
@jam: the canvas drawImage JS API is synchronous; when it returns the pixels need to have been read back from the texture to the canvas. Synchronous JS APIs block the render thread when their results can be used for rendering (e.g. launching an async readPixels and returning from readPixels without waiting for the readback would result in JS being inconsistent with painted UI). Would it help to have a chat over VC? On Tue, Nov 5, 2013 at 8:35 AM, <jam@chromium.org> wrote: > On 2013/11/04 23:06:31, sheu wrote: > >> fischman@ corrected me: >> > > readpixels happens from (and needs to block) the render thread, not the >> libjingle thread. The requirement for blocking comes from the JS API in >> question: >> > > http://www.whatwg.org/specs/web-apps/current-work/ > multipage/the-canvas-element.html#dom-context-2d-drawimage > > hmm, I still don't follow. Can you please describe in detail why the > renderer > thread is blocked? > > https://codereview.chromium.org/27420004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/05 16:56:56, Ami Fischman wrote: > @jam: the canvas drawImage JS API is synchronous; when it returns the > pixels need to have been read back from the texture to the canvas. > Synchronous JS APIs block the render thread when their results can be used > for rendering (e.g. launching an async readPixels and returning from > readPixels without waiting for the readback would result in JS being > inconsistent with painted UI). > > Would it help to have a chat over VC? yes, anytime that's free on my calendar is good
Updated, PTAL. One (functional no-op) difference made also: make ReadPixels() take a visible_rect argument instead of a size argument. This is in line with the assumption in ConvertVideoFrameToBitmap() (where ReadPixels() is used) that only the visible portion of the frame should be copied down.
PS#7 LGTM. FTR jam/sheu/fischman chatted off-reviewlog about BindToLoopSync and why the render thread must block. The latter was taken care of early (JS API requirement) but the former was discussed in more depth. jam@ was concerned that a blocking primitive would be abused, and suggested we implement something like ThreadRestrictions' DisallowWaiting, so that a master list of "friends" could be maintained by BindToLoopSync to ensure no unexpected abuse arises. The full-monty solution is to enable DisallowWaiting on the renderer thread, but that's a bigger kettle of fish than anyone in the meeting felt like frying (jam@ filed crbug.com/315137 for the future). Finally we agreed that the cost of duplicating the Wait logic in two places was lower than the worry cost about future abuse of a new primitive, and sheu@ implemented that in PS#7.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/974003
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/974003
Retried try job too often on linux_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/974003
Retried try job too often on linux_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/974003
Retried try job too often on linux_chromeos for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/1454001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/1454001
Retried try job too often on linux_chromeos for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Sorry for the thrash. I've looked at the failures multiple ways and it still looks like a series of unfortunate flakes. Rebasing and trying again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/1884001
Retried try job too often on linux_chromeos for step(s) browser_tests, content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
For reference: this CL is blocked on a content_browsertests bug that seems to be difficult to debug. chromium-dev context: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Gbkqa5Eyaws Bugs: https://code.google.com/p/chromium/issues/detail?id=281492, https://code.google.com/p/chromium/issues/detail?id=281268
@sheu: phoglund@ disabled a bunch of those tests recently; does that change the story for this test? On Mon, Dec 2, 2013 at 2:18 PM, <sheu@chromium.org> wrote: > For reference: this CL is blocked on a content_browsertests bug that seems > to be > difficult to debug. > > chromium-dev context: > https://groups.google.com/a/chromium.org/forum/#!topic/ > chromium-dev/Gbkqa5Eyaws > Bugs: https://code.google.com/p/chromium/issues/detail?id=281492, > https://code.google.com/p/chromium/issues/detail?id=281268 > > https://chromiumcodereview.appspot.com/27420004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It makes me antsy to be committing threading changes like this, while some relevant tests are disabled. I'd like to get resolution on those bugs first.
On 2013/12/02 22:23:24, sheu wrote: > It makes me antsy to be committing threading changes like this, while some > relevant tests are disabled. I'd like to get resolution on those bugs first. Given the rate of progress on those bugs I wouldn't expect these tests to get undisabled anytime soon. However, the tests are not disabled on windows, which should provide automated coverage of this change, and you can verify that your change is cool with those tests in your local checkout (AFAICT the borkedness only happens on bots, not on local checkouts). So, if I was you, I don't think I'd feel the antsyness you describe. Up to you, of course.
And update on this CL: So, the thing is that I can't get the failing tests to fail if I run them manually. They only fail if I trybot them here. Also, it appears that having LOG(ERROR) in the appropriate place causes the test to pass. Hence the spam with the uploaded CLs -- I'm bisecting exactly which log lines are causing the difference in behavior.
@sheu: I believe the webrtc test flakiness has been fixed. Can this land now?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/3664001
Retried try job too often on win_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/27420004/3664001
Message was sent while issue was closed.
Change committed as 247480
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/145103004/ by ricea@chromium.org. The reason for reverting is: Sorry, it looks like you broke the Win 7 Tests (dbg)(2) bot. http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28.... |
