Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710133003:
Replace FFmpegDemuxer thread per element with base::TaskScheduler. (Closed)
|
Created:
3 years, 10 months ago by DaleCurtis Modified:
3 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace FFmpegDemuxer thread per element with base::TaskScheduler.
I don't think we need to keep starting a thread per <audio> or
<video> element anymore. Instead we can use a shared pool that
only spins up a new thread when needed.
This change switches FFmpegDemuxer from creating its own thread
to using base::TaskScheduler for executing blocking network reads.
The TaskScheduler API is perfectly suited for our use case and
required little changes.
Doing this required some changes to our BlockingUrlProtocol to
make it safe to access after FFmpegDemuxer and potentially the
DataSource have been destroyed.
This should reduce the number of out of threads crashes we see on
a fairly regular basis.
BUG=61293
TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com
Review-Url: https://codereview.chromium.org/2710133003
Cr-Original-Commit-Position: refs/heads/master@{#453470}
Committed: https://chromium.googlesource.com/chromium/src/+/5384b9fe1cf67869e1bf98ab8ff6bd21ae4216da
Review-Url: https://codereview.chromium.org/2710133003
Cr-Commit-Position: refs/heads/master@{#454452}
Committed: https://chromium.googlesource.com/chromium/src/+/f4a6dbf44108f1855faf5a78d4fb06c349ebd70d
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use base::TaskScheduler. #
Total comments: 12
Patch Set 3 : Comments. Fix flaky test. #
Total comments: 19
Patch Set 4 : Fix comments. #Patch Set 5 : Fix gn check. #Patch Set 6 : Fix chromecast build. #
Total comments: 4
Patch Set 7 : Address all comments. #Patch Set 8 : Give ffmpeg unittests time to release glue. #Patch Set 9 : Give WeakPtr to URLProtocol. #
Total comments: 3
Messages
Total messages: 61 (31 generated)
dalecurtis@chromium.org changed reviewers: + gab@chromium.org, xhwang@chromium.org
This is still early, but all tests pass (% some mock issues in BlockingUrlProtocol) so looking for feedback: gab@ you seem most knowledgable about SWP, so please let me know if I'm doing something dumb with the SequencedWorkerPool and please recommend whether we should provide a shutdown strategy for when the pool is no longer in use or just let it be leaky. xhwang@ please let me know if I've forgotten any lifetime issues around how we manage our demuxer objects. Thanks!
On 2017/02/23 03:00:18, DaleCurtis wrote: > This is still early, but all tests pass (% some mock issues in > BlockingUrlProtocol) so looking for feedback: > > gab@ you seem most knowledgable about SWP, so please let me know if I'm doing > something dumb with the SequencedWorkerPool and please recommend whether we > should provide a shutdown strategy for when the pool is no longer in use or just > let it be leaky. SequencedWorkerPool is being phased out in favor of the TaskScheduler API (base/task_scheduler/post_task.h). There should be a TaskScheduler instance in every process at this point so you just use it :), please let me know if this API doesn't suit your needs and we'll make sure it does (it's about to become the one and only API to post tasks/get TaskRunners from in Chromium -- sorry for lack of explicit documentation beyond post_task.h/task_traits.h, it's in the works as well). But, yes, this is definitely the right thing to do (getting rid of your dedicated threads) -- we're also sending those to TaskScheduler these days :) Thanks, Gab > > xhwang@ please let me know if I've forgotten any lifetime issues around how we > manage our demuxer objects. > > Thanks!
On 2017/02/23 at 20:08:57, gab wrote: > On 2017/02/23 03:00:18, DaleCurtis wrote: > > This is still early, but all tests pass (% some mock issues in > > BlockingUrlProtocol) so looking for feedback: > > > > gab@ you seem most knowledgable about SWP, so please let me know if I'm doing > > something dumb with the SequencedWorkerPool and please recommend whether we > > should provide a shutdown strategy for when the pool is no longer in use or just > > let it be leaky. > > SequencedWorkerPool is being phased out in favor of the TaskScheduler API (base/task_scheduler/post_task.h). There should be a TaskScheduler instance in every process at this point so you just use it :), please let me know if this API doesn't suit your needs and we'll make sure it does (it's about to become the one and only API to post tasks/get TaskRunners from in Chromium -- sorry for lack of explicit documentation beyond post_task.h/task_traits.h, it's in the works as well). Thanks for the advice! I'll look over the API and modify this as needed. > > But, yes, this is definitely the right thing to do (getting rid of your dedicated threads) -- we're also sending those to TaskScheduler these days :) > > Thanks, > Gab > > > > > xhwang@ please let me know if I've forgotten any lifetime issues around how we > > manage our demuxer objects. > > > > Thanks!
Description was changed from ========== Replace FFmpegDemuxer thread per element with lazy blocking pool. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::SequencedWorkerPool for executing blocking network reads. I've capped the pool size at 6 threads for now since that's the maximum number of loads that could happen per-origin; this may be too small for some pages. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=none TEST=TBD, needs benchmarking with many tags per page. ========== to ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=none TEST=TBD, needs benchmarking with many tags per page. ==========
Actually that was super easy. PTAL! Thanks for the cool API. I still need to come up with a benchmark page to see how this performs, but certainly is simple to implement.
Woohoo :) https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... File media/base/run_all_perftests.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... media/base/run_all_perftests.cc:29: base::TaskScheduler::CreateAndSetSimpleTaskScheduler(1); For tests see base::test::ScopedTaskScheduler, also you inspired me to make this documentation clearer https://codereview.chromium.org/2709163006/ :). Anymore feedback you have on things that were unclear is welcome! https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:854: base::TaskTraits().MayBlock().WithBaseSyncPrimitives().WithPriority( Trusting you and your reviewer on these since I'm not familiar with your tasks at all :) https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:855: base::TaskPriority::USER_VISIBLE))), Is this used for live video/audio? i.e TaskPriority::USER_BLOCKING? https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:882: blocking_task_runner_->DeleteSoon(FROM_HERE, glue_.release()); Have you considered binding those tasks to WeakPtrs? It's unfortunate to have to execute work if it's no longer relevant. Actually looks like you do use WeakPtrs (ref. Stop())? https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:299: // Blocking pool on which all blocking FFmpeg operations are executed. nit: A SequencedTaskRunner isn't a "pool", it's a "sequence". A TaskRunner would be a "pool" but would run your tasks in parallel (depends on whether you can accommodate for the thread-safety implications of that). https://codereview.chromium.org/2710133003/diff/20001/media/test/run_all_unit... File media/test/run_all_unittests.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/test/run_all_unit... media/test/run_all_unittests.cc:60: base::TaskScheduler::CreateAndSetSimpleTaskScheduler(1); base::test::ScopedTaskScheduler (we typically prefer to only put it in fixtures that need it but if all your tests need it it's your call if you want it here)
PS: which process is this running in?
Thanks, going through feedback now. This is in the renderer process.
https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... File media/base/run_all_perftests.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... media/base/run_all_perftests.cc:29: base::TaskScheduler::CreateAndSetSimpleTaskScheduler(1); On 2017/02/23 at 21:37:09, gab wrote: > For tests see base::test::ScopedTaskScheduler, also you inspired me to make this documentation clearer https://codereview.chromium.org/2709163006/ :). Anymore feedback you have on things that were unclear is welcome! Thanks! Switched, though I think this omits a bit of test coverage since by forcing an additional thread we get coverage on potential races that the fake task scheduler will not provide by using the same message loop as the test control functionality. https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:855: base::TaskPriority::USER_VISIBLE))), On 2017/02/23 at 21:37:10, gab wrote: > Is this used for live video/audio? i.e TaskPriority::USER_BLOCKING? It could be live audio video. The user may even want this frame immediately. But I took BLOCKING to mean it's literally preventing UI from rendering or something like that. If the user is actively watching the video or audio it could definitely be considered blocking. Defer to your judgement. https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:882: blocking_task_runner_->DeleteSoon(FROM_HERE, glue_.release()); On 2017/02/23 at 21:37:10, gab wrote: > Have you considered binding those tasks to WeakPtrs? It's unfortunate to have to execute work if it's no longer relevant. > > Actually looks like you do use WeakPtrs (ref. Stop())? Can't; these tasks are bound to static methods. We'd need to setup an indirection layer; for which I wouldn't expect it matters much in these cases. These methods will all have failed quickly by this point since we fail all future reads during Stop(). We use the WeakPtrs for binding to the media thread and the FFmpegDemuxer class itself, but all ffmpeg API calls happen on the blocking task runner. https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:299: // Blocking pool on which all blocking FFmpeg operations are executed. On 2017/02/23 at 21:37:10, gab wrote: > nit: A SequencedTaskRunner isn't a "pool", it's a "sequence". A TaskRunner would be a "pool" but would run your tasks in parallel (depends on whether you can accommodate for the thread-safety implications of that). We definitely want sequence here. Updated the comment.
Cool, do I get it correct that the newly created SequencedTaskRunner will handle threading and dynamically allocate/destroy threads as needed? CL LG, just a few nits/comments https://codereview.chromium.org/2710133003/diff/1/media/filters/blocking_url_... File media/filters/blocking_url_protocol.cc (right): https://codereview.chromium.org/2710133003/diff/1/media/filters/blocking_url_... media/filters/blocking_url_protocol.cc:25: // |aborted_|. nit: This comment style looks odd :) https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... File media/filters/blocking_url_protocol.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:21: // asynchronous DataSource::Read() operation completes. nit: Can we have some documentation on the threading model for this class? https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:48: base::Lock data_source_lock_; Can you provide a comment why we need this lock, e.g. |data_source_| can be access on which threads. https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:49: DataSource* data_source_; nit: add an empty line here so that it's more clear that the lock only protects |data_source_| https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... File media/filters/blocking_url_protocol_unittest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol_unittest.cc:121: base::Unretained(this)))); I am a bit lost, why do you need to create a new BlockingUrlProtocol here? https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:855: base::TaskPriority::USER_VISIBLE))), this is cool :)
On 2017/02/23 23:45:42, xhwang_slow wrote: > Cool, do I get it correct that the newly created SequencedTaskRunner will handle > threading and dynamically allocate/destroy threads as needed? Yep :). Your SequencedTaskRunner guarantees thread-safety (not thread-affinity), which thread(s) things run on is an implementation detail of TaskSchedulerImpl :). https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... File media/base/run_all_perftests.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/base/run_all_perf... media/base/run_all_perftests.cc:29: base::TaskScheduler::CreateAndSetSimpleTaskScheduler(1); On 2017/02/23 23:07:06, DaleCurtis wrote: > On 2017/02/23 at 21:37:09, gab wrote: > > For tests see base::test::ScopedTaskScheduler, also you inspired me to make > this documentation clearer https://codereview.chromium.org/2709163006/ :). > Anymore feedback you have on things that were unclear is welcome! > > Thanks! Switched, though I think this omits a bit of test coverage since by > forcing an additional thread we get coverage on potential races that the fake > task scheduler will not provide by using the same message loop as the test > control functionality. You can use ScopedAsyncTaskScheduler if you want that behavior :). The reasoning behind ScopedTaskScheduler using the main thread is that it allows determinism in unit tests (and thus simplicity). IMO fuzzing/testing races is more the role of browser/integration tests. https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:855: base::TaskPriority::USER_VISIBLE))), On 2017/02/23 23:07:06, DaleCurtis wrote: > On 2017/02/23 at 21:37:10, gab wrote: > > Is this used for live video/audio? i.e TaskPriority::USER_BLOCKING? > > It could be live audio video. The user may even want this frame immediately. But > I took BLOCKING to mean it's literally preventing UI from rendering or something > like that. If the user is actively watching the video or audio it could > definitely be considered blocking. Defer to your judgement. IMO "blocking UI from rendering" and "decoding a live video" is equivalent. And since you're running on a sequence you don't have to worry about saturating the threads so USER_BLOCKING would be reasonable here I think. https://codereview.chromium.org/2710133003/diff/40001/media/base/demuxer_perf... File media/base/demuxer_perftest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/base/demuxer_perf... media/base/demuxer_perftest.cc:199: base::Bind(&QuitLoopWithStatus, &message_loop), Use RunLoop::QuitClosure() (from the RunLoop on 213 -- i.e. instantiate it above and still Run() it below). Then you can use the implicit MessageLoop from ScopedTaskScheduler https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:244: base::MessageLoop message_loop_; Drop this member (ScopedTaskScheduler will provide an implicit one). We're trying to get rid of explicit MessageLoop usage. Replace message_loop_.task_runner() with ThreadTaskRunnerHandle::Get(). https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:133: base::MessageLoop message_loop_; If you feel like cleaning this up: ThreadTaskRunnerHandle::Get() can replace message_loop_->task_runner() on main thread and MessageLoop::Quit() paradigm should be replaced by RunLoop()+QuitClosure(), then you can use ML provided by base::test::ScopedTaskScheduler (soon to become base::test::ScopedTaskEnvironment for the very reason that it's not strictly for the scheduler -- yeah kind of midflight...). https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:137: std::unique_ptr<base::test::ScopedTaskScheduler> task_scheduler_; Why does it have to be delay initialized?
https://codereview.chromium.org/2710133003/diff/40001/media/base/demuxer_perf... File media/base/demuxer_perftest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/base/demuxer_perf... media/base/demuxer_perftest.cc:199: base::Bind(&QuitLoopWithStatus, &message_loop), On 2017/02/24 at 01:09:05, gab wrote: > Use RunLoop::QuitClosure() (from the RunLoop on 213 -- i.e. instantiate it above and still Run() it below). > > Then you can use the implicit MessageLoop from ScopedTaskScheduler Done. https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... File media/filters/blocking_url_protocol.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:21: // asynchronous DataSource::Read() operation completes. On 2017/02/23 23:45:42, xhwang_slow wrote: > nit: Can we have some documentation on the threading model for this class? Done. https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:48: base::Lock data_source_lock_; On 2017/02/23 at 23:45:42, xhwang_slow wrote: > Can you provide a comment why we need this lock, e.g. |data_source_| can be access on which threads. Done. https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol.h:49: DataSource* data_source_; On 2017/02/23 at 23:45:42, xhwang_slow wrote: > nit: add an empty line here so that it's more clear that the lock only protects |data_source_| Done. https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... File media/filters/blocking_url_protocol_unittest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/blocking_... media/filters/blocking_url_protocol_unittest.cc:121: base::Unretained(this)))); On 2017/02/23 at 23:45:42, xhwang_slow wrote: > I am a bit lost, why do you need to create a new BlockingUrlProtocol here? Because I switched IsStreaming() to be set at construction time instead of always being queried from the DataSource; this also makes it clear that this value can never change. https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/2710133003/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:244: base::MessageLoop message_loop_; On 2017/02/24 at 01:09:05, gab wrote: > Drop this member (ScopedTaskScheduler will provide an implicit one). > > We're trying to get rid of explicit MessageLoop usage. > > Replace message_loop_.task_runner() with ThreadTaskRunnerHandle::Get(). Done. https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:133: base::MessageLoop message_loop_; On 2017/02/24 01:09:05, gab wrote: > If you feel like cleaning this up: ThreadTaskRunnerHandle::Get() can replace > message_loop_->task_runner() on main thread and MessageLoop::Quit() paradigm > should be replaced by RunLoop()+QuitClosure(), then you can use ML provided by > base::test::ScopedTaskScheduler (soon to become > base::test::ScopedTaskEnvironment for the very reason that it's not strictly for > the scheduler -- yeah kind of midflight...). Too big of a change to do this in this CL, will poke at it for a followup. The whole class and subclasses makes repeated use of RunUntilIdle() and bare Run() calls. https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:137: std::unique_ptr<base::test::ScopedTaskScheduler> task_scheduler_; On 2017/02/24 at 01:09:06, gab wrote: > Why does it have to be delay initialized? It's only needed for FFmpegDemuxer; which half of these integration tests don't use.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=none TEST=TBD, needs benchmarking with many tags per page. ========== to ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=none TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
dalecurtis@chromium.org changed reviewers: + halliwell@chromium.org
+halliwell for chromecast/ change.
lgtm
On 2017/02/25 01:25:39, xhwang_slow wrote: > lgtm lgtm
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
TaskScheduler usage lgtm w/ comments, thanks! https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:137: std::unique_ptr<base::test::ScopedTaskScheduler> task_scheduler_; On 2017/02/24 23:40:29, DaleCurtis wrote: > On 2017/02/24 at 01:09:06, gab wrote: > > Why does it have to be delay initialized? > > It's only needed for FFmpegDemuxer; which half of these integration tests don't > use. I see, add a comment to that effect on this member? https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... File chromecast/media/cma/test/frame_segmenter_for_test.cc (right): https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... chromecast/media/cma/test/frame_segmenter_for_test.cc:318: base::test::ScopedTaskScheduler task_scheduler(base::MessageLoop::current()); There should really just be one ScopedTaskScheduler in the whole scope of a test, bringing it in&out in a helper method is unexpected. Can we put one in the fixture of audio_video_pipeline_device_unittest.cc where this seems to be used instead? https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... File media/base/demuxer_perftest.cc (right): https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... media/base/demuxer_perftest.cc:217: run_loop.Run(); This whole block can be replaced by base::RunLoop().RunUntilIdle(); I think (unless you really care to strictly only run tasks already posted but not tasks posted from those, if any?)
halliwell: PTAL - gab@'s last request required significant changes to the unittest. https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... File media/test/pipeline_integration_test_base.h (right): https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... media/test/pipeline_integration_test_base.h:137: std::unique_ptr<base::test::ScopedTaskScheduler> task_scheduler_; On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > On 2017/02/24 23:40:29, DaleCurtis wrote: > > On 2017/02/24 at 01:09:06, gab wrote: > > > Why does it have to be delay initialized? > > > > It's only needed for FFmpegDemuxer; which half of these integration tests don't > > use. > > I see, add a comment to that effect on this member? Done. https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... File chromecast/media/cma/test/frame_segmenter_for_test.cc (right): https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... chromecast/media/cma/test/frame_segmenter_for_test.cc:318: base::test::ScopedTaskScheduler task_scheduler(base::MessageLoop::current()); On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > There should really just be one ScopedTaskScheduler in the whole scope of a test, bringing it in&out in a helper method is unexpected. > > Can we put one in the fixture of audio_video_pipeline_device_unittest.cc where this seems to be used instead? Done, but took a few more changes. https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... File media/base/demuxer_perftest.cc (right): https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... media/base/demuxer_perftest.cc:217: run_loop.Run(); On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > This whole block can be replaced by base::RunLoop().RunUntilIdle(); I think (unless you really care to strictly only run tasks already posted but not tasks posted from those, if any?) Done.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/27 21:26:30, DaleCurtis wrote: > halliwell: PTAL - gab@'s last request required significant changes to the > unittest. > > https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... > File media/test/pipeline_integration_test_base.h (right): > > https://codereview.chromium.org/2710133003/diff/40001/media/test/pipeline_int... > media/test/pipeline_integration_test_base.h:137: > std::unique_ptr<base::test::ScopedTaskScheduler> task_scheduler_; > On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > > On 2017/02/24 23:40:29, DaleCurtis wrote: > > > On 2017/02/24 at 01:09:06, gab wrote: > > > > Why does it have to be delay initialized? > > > > > > It's only needed for FFmpegDemuxer; which half of these integration tests > don't > > > use. > > > > I see, add a comment to that effect on this member? > > Done. > > https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... > File chromecast/media/cma/test/frame_segmenter_for_test.cc (right): > > https://codereview.chromium.org/2710133003/diff/100001/chromecast/media/cma/t... > chromecast/media/cma/test/frame_segmenter_for_test.cc:318: > base::test::ScopedTaskScheduler task_scheduler(base::MessageLoop::current()); > On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > > There should really just be one ScopedTaskScheduler in the whole scope of a > test, bringing it in&out in a helper method is unexpected. > > > > Can we put one in the fixture of audio_video_pipeline_device_unittest.cc where > this seems to be used instead? > > Done, but took a few more changes. > > https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... > File media/base/demuxer_perftest.cc (right): > > https://codereview.chromium.org/2710133003/diff/100001/media/base/demuxer_per... > media/base/demuxer_perftest.cc:217: run_loop.Run(); > On 2017/02/26 at 18:01:35, gab (UTC plus 9 Tokyo) wrote: > > This whole block can be replaced by base::RunLoop().RunUntilIdle(); I think > (unless you really care to strictly only run tasks already posted but not tasks > posted from those, if any?) > > Done. still lgtm
The CQ bit was unchecked by dalecurtis@chromium.org
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2710133003/#ps120001 (title: "Address all comments.")
Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=none TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com ========== to ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=61293 TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, gab@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2710133003/#ps140001 (title: "Give ffmpeg unittests time to release glue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488240540364000,
"parent_rev": "8efedb10c4620871e01944c79c294be9ef04db07", "commit_rev":
"5384b9fe1cf67869e1bf98ab8ff6bd21ae4216da"}
Message was sent while issue was closed.
Description was changed from ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=61293 TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com ========== to ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=61293 TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com Review-Url: https://codereview.chromium.org/2710133003 Cr-Commit-Position: refs/heads/master@{#453470} Committed: https://chromium.googlesource.com/chromium/src/+/5384b9fe1cf67869e1bf98ab8ff6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5384b9fe1cf67869e1bf98ab8ff6...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2719883005/ by dvadym@chromium.org. The reason for reverting is: It's most likely a culprit for failing test: http/tests/media/video-in-iframe-crash.html starting from https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu... Error message: [ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"FFmpegDemuxer: data source error"}.
Message was sent while issue was closed.
On 2017/02/28 at 10:13:18, dvadym wrote: > A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2719883005/ by dvadym@chromium.org. > > The reason for reverting is: It's most likely a culprit for failing test: > http/tests/media/video-in-iframe-crash.html starting from > https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu... > > Error message: > [ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"FFmpegDemuxer: data source error"}. Booo :( Traveling today, will take a look and see what's happening tomorrow.
Okay, issue resolved, the problem was a base::Unretained() by the blocking pool, it should have been switched to a WeakPtr; fixed and verified with local asan layout tests. This was the source of the layout test and cluster-fuzz issues. xhwang: Want to look over ffmpeg_demuxer.cc again?
lg % nit https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:901: &FFmpegDemuxer::OnDataSourceError, weak_this_)))); We have mixed use of weak_this_ and weak_factory_.GetWeakPtr() in this file. Shall we consolidate them?
The CQ bit was checked by dalecurtis@chromium.org
https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:901: &FFmpegDemuxer::OnDataSourceError, weak_this_)))); On 2017/03/02 at 22:05:01, xhwang_slow wrote: > We have mixed use of weak_this_ and weak_factory_.GetWeakPtr() in this file. Shall we consolidate them? weak_this_ is bound to a factory which should only be invalidated during Stop() prior to destruction, while weak_factory_ is invalidated regularly during AbortPendingReads(). This isn't documented very well and I agree some cleanup is necessary here. I'll see what I can do in a followup CL.
The patchset sent to the CQ was uploaded after l-g-t-m from halliwell@chromium.org, gab@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2710133003/#ps160001 (title: "Give WeakPtr to URLProtocol.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2710133003/diff/160001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:901: &FFmpegDemuxer::OnDataSourceError, weak_this_)))); On 2017/03/02 22:48:53, DaleCurtis wrote: > On 2017/03/02 at 22:05:01, xhwang_slow wrote: > > We have mixed use of weak_this_ and weak_factory_.GetWeakPtr() in this file. > Shall we consolidate them? > > weak_this_ is bound to a factory which should only be invalidated during Stop() > prior to destruction, while weak_factory_ is invalidated regularly during > AbortPendingReads(). This isn't documented very well and I agree some cleanup is > necessary here. I'll see what I can do in a followup CL. Ah, that's tricky and I didn't even notice it. A followup cleanup CL would be nice. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488494932135780,
"parent_rev": "37716386769b230a22b267a9789d68946b094552", "commit_rev":
"f4a6dbf44108f1855faf5a78d4fb06c349ebd70d"}
Message was sent while issue was closed.
Description was changed from ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=61293 TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com Review-Url: https://codereview.chromium.org/2710133003 Cr-Commit-Position: refs/heads/master@{#453470} Committed: https://chromium.googlesource.com/chromium/src/+/5384b9fe1cf67869e1bf98ab8ff6... ========== to ========== Replace FFmpegDemuxer thread per element with base::TaskScheduler. I don't think we need to keep starting a thread per <audio> or <video> element anymore. Instead we can use a shared pool that only spins up a new thread when needed. This change switches FFmpegDemuxer from creating its own thread to using base::TaskScheduler for executing blocking network reads. The TaskScheduler API is perfectly suited for our use case and required little changes. Doing this required some changes to our BlockingUrlProtocol to make it safe to access after FFmpegDemuxer and potentially the DataSource have been destroyed. This should reduce the number of out of threads crashes we see on a fairly regular basis. BUG=61293 TEST=existing tests all pass, no slowdown noticed on pages like gfycat.com Review-Url: https://codereview.chromium.org/2710133003 Cr-Original-Commit-Position: refs/heads/master@{#453470} Committed: https://chromium.googlesource.com/chromium/src/+/5384b9fe1cf67869e1bf98ab8ff6... Review-Url: https://codereview.chromium.org/2710133003 Cr-Commit-Position: refs/heads/master@{#454452} Committed: https://chromium.googlesource.com/chromium/src/+/f4a6dbf44108f1855faf5a78d4fb... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f4a6dbf44108f1855faf5a78d4fb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
