Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2891983002:
Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner (Closed)
|
Created:
3 years, 7 months ago by Sébastien Marchand Modified:
3 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner
Quoting gab@ from https://codereview.chromium.org/2885173002/diff/40001/media/audio/audio_debug_file_writer.h:
AudioFileWriter::AudioFileWriter() merely takes the task runner to assert it's
running on it, that can easily be turned into a SequencedTaskRunner but even
better than that would be to have it use a SequenceChecker to check that all of
its methods run in sequence (will need to DetachFromSequence in
AudioFileWriter::AudioFileWriter() as it's not constructed on same sequence it's
used -- see sequence_checker.h for details).
SequenceChecker + implicit AssertIOAllowed() calls from using base file methods
guarantees this class is being used in sequence with MayBlock() trait. That's
preferred and cleaner than taking a task_runner just to assume running on it.
Then there's OnThreadDeleter which could easily be made OnSequenceDeleter +
SequenceTaskRunner.
BUG=689520
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2891983002
Cr-Commit-Position: refs/heads/master@{#473264}
Committed: https://chromium.googlesource.com/chromium/src/+/45167d8df4bc03bdbecbc9009901528ce9349226
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address Gab's comments #Patch Set 3 : make the style checker happy. #
Total comments: 2
Patch Set 4 : move semantic #Messages
Total messages: 32 (24 generated)
Description was changed from ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner BUG=689520 ========== to ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by sebmarchand@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 ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner Quoting gab@ from https://codereview.chromium.org/2885173002/diff/40001/media/audio/audio_debug...: AudioFileWriter::AudioFileWriter() merely takes the task runner to assert it's running on it, that can easily be turned into a SequencedTaskRunner but even better than that would be to have it use a SequenceChecker to check that all of its methods run in sequence (will need to DetachFromSequence in AudioFileWriter::AudioFileWriter() as it's not constructed on same sequence it's used -- see sequence_checker.h for details). SequenceChecker + implicit AssertIOAllowed() calls from using base file methods guarantees this class is being used in sequence with MayBlock() trait. That's preferred and cleaner than taking a task_runner just to assume running on it. Then there's OnThreadDeleter which could easily be made OnSequenceDeleter + SequenceTaskRunner. BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
gab@chromium.org changed reviewers: + gab@chromium.org
lgtm w/ nit and suggestions for more cleanup :) https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... File media/audio/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.cc:189: : task_runner_(task_runner) {} std::move(task_runner) https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.h:16: #include "base/single_thread_task_runner.h" Fix include https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.h:71: OnSequenceDeleter(const OnSequenceDeleter& other); I'm pretty sure this is only needed to pass it by value in std::unique_ptr which should support move-semantics as of C++11. A nice cleanup would be to make this OnSequenceDeleter(OnSequenceDeleter&& other) = default; and remove the impl.
The CQ bit was checked by sebmarchand@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...
sebmarchand@chromium.org changed reviewers: + henrika@chromium.org, tommi@chromium.org
+ henrika/tommi for owner approval. https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... File media/audio/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.cc:189: : task_runner_(task_runner) {} On 2017/05/18 20:38:17, gab wrote: > std::move(task_runner) Done. https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.h:16: #include "base/single_thread_task_runner.h" On 2017/05/18 20:38:17, gab wrote: > Fix include Done. https://codereview.chromium.org/2891983002/diff/1/media/audio/audio_debug_fil... media/audio/audio_debug_file_writer.h:71: OnSequenceDeleter(const OnSequenceDeleter& other); On 2017/05/18 20:38:17, gab wrote: > I'm pretty sure this is only needed to pass it by value in std::unique_ptr which > should support move-semantics as of C++11. > > A nice cleanup would be to make this > > OnSequenceDeleter(OnSequenceDeleter&& other) = default; > > and remove the impl. Done, I also had to provide the OnSequenceDeleter& operator=(const OnSequenceDeleter&) function (which is normally deleted).
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...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by sebmarchand@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...
https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug... media/audio/audio_debug_file_writer.h:73: OnSequenceDeleter& operator=(const OnSequenceDeleter&); Actually you want the matching move assignment operator, not copy asssignment: OnSequenceDeleter& operator= (OnSequenceDeleter &&)
https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2891983002/diff/40001/media/audio/audio_debug... media/audio/audio_debug_file_writer.h:73: OnSequenceDeleter& operator=(const OnSequenceDeleter&); On 2017/05/18 22:11:21, gab wrote: > Actually you want the matching move assignment operator, not copy asssignment: > > OnSequenceDeleter& operator= (OnSequenceDeleter &&) Ha! Time for me to re learn C++11 and the move semantic.
The CQ bit was checked by sebmarchand@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: This issue passed the CQ dry run.
henrika@chromium.org changed reviewers: + grunell@chromium.org - henrika@chromium.org
sebmarchand@chromium.org changed reviewers: + tguilbert@chromium.org
+tguilbert@ for owner approval.
On 2017/05/19 14:48:43, Sébastien Marchand wrote: > +tguilbert@ for owner approval. media -- LGTM
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2891983002/#ps60001 (title: "move semantic ")
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": 60001, "attempt_start_ts": 1495219977362610,
"parent_rev": "b212c4f3f222c4bf89640a8796f0248e81245a8a", "commit_rev":
"45167d8df4bc03bdbecbc9009901528ce9349226"}
Message was sent while issue was closed.
Description was changed from ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner Quoting gab@ from https://codereview.chromium.org/2885173002/diff/40001/media/audio/audio_debug...: AudioFileWriter::AudioFileWriter() merely takes the task runner to assert it's running on it, that can easily be turned into a SequencedTaskRunner but even better than that would be to have it use a SequenceChecker to check that all of its methods run in sequence (will need to DetachFromSequence in AudioFileWriter::AudioFileWriter() as it's not constructed on same sequence it's used -- see sequence_checker.h for details). SequenceChecker + implicit AssertIOAllowed() calls from using base file methods guarantees this class is being used in sequence with MayBlock() trait. That's preferred and cleaner than taking a task_runner just to assume running on it. Then there's OnThreadDeleter which could easily be made OnSequenceDeleter + SequenceTaskRunner. BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make AudioDebugFileWriter::AudioFileWriter use a SequencedTaskRunner Quoting gab@ from https://codereview.chromium.org/2885173002/diff/40001/media/audio/audio_debug...: AudioFileWriter::AudioFileWriter() merely takes the task runner to assert it's running on it, that can easily be turned into a SequencedTaskRunner but even better than that would be to have it use a SequenceChecker to check that all of its methods run in sequence (will need to DetachFromSequence in AudioFileWriter::AudioFileWriter() as it's not constructed on same sequence it's used -- see sequence_checker.h for details). SequenceChecker + implicit AssertIOAllowed() calls from using base file methods guarantees this class is being used in sequence with MayBlock() trait. That's preferred and cleaner than taking a task_runner just to assume running on it. Then there's OnThreadDeleter which could easily be made OnSequenceDeleter + SequenceTaskRunner. BUG=689520 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2891983002 Cr-Commit-Position: refs/heads/master@{#473264} Committed: https://chromium.googlesource.com/chromium/src/+/45167d8df4bc03bdbecbc9009901... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/45167d8df4bc03bdbecbc9009901... |
