Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 7 months ago by aleloi Modified:
3 years, 7 months ago Reviewers:
the sun, tommi, danilchap, tommi (sloooow) - chröme, perkj_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, danilchap Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionActivate 'offload debug dump recordings from audio thread to TaskQueue'.
A low priority task queue is added to WebRTCVoiceEngine. The
start/stop debug calls make file logging happen on the task queue.
In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory,
so that it can be shared for low priority tasks between different
subcomponents. It will require some changes to MediaEngine,
CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal
projects.
A task queue must be created and destroyed from the same thread. With
this CL that will be the worker thread, which creates and destroys
WebRTCVoiceEngine. With the dependent CL, it will probably change to
the signaling thread.
NOTRY=True # tests just passed
BUG=webrtc:7404
Review-Url: https://codereview.webrtc.org/2896813002
Cr-Commit-Position: refs/heads/master@{#18252}
Committed: https://chromium.googlesource.com/external/webrtc/+/c61bf947b4ac31f3500858ffcae6fee39d799930
Patch Set 1 : Adding strict mock expectations. #
Total comments: 5
Patch Set 2 : Rebase on top of latest. #
Total comments: 1
Patch Set 3 : StartRecording should return 'true' when starting is successfull. #
Total comments: 4
Patch Set 4 : Give queue shorter name. #
Created: 3 years, 7 months ago
Messages
Total messages: 53 (42 generated)
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/13888) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/25679)
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
Description was changed from ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL, the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. BUG=webrtc:7404 ========== to ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. BUG=webrtc:7404 ==========
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
Hi, can you PTAL? There still is 'compile-time polymorphism', but I can't see how to get rid of it. We are only allowed to depend on 'audioproc_debug_proto' when 'rtc_enable_protobuf' is on. But there is only one version of webrtcvoiceengine.cc, which has to make the same calls on all compilation flags.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:668: return !!aec_dump; since you have moved from aec_dump at this point, aren't the contents undefined? https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:115: rtc::TaskQueue low_priority_worker_queue_; On some platforms this means we're adding yet another thread. What other options have you considered?
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
New patch (minimal changes) & responses to comments. PTAL! https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:668: return !!aec_dump; On 2017/05/22 22:38:00, the sun wrote: > since you have moved from aec_dump at this point, aren't the contents undefined? I don't know what I was thinking... Thanks! This was also wrong because '!!aec_dump' always returned 'false'. A moved-from unique_ptr has defined semantics (this SO thread quotes relevant documents https://stackoverflow.com/questions/36071220/). It's always empty. Which was the wrong property to test for. https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:115: rtc::TaskQueue low_priority_worker_queue_; On 2017/05/22 22:38:00, the sun wrote: > On some platforms this means we're adding yet another thread. What other options > have you considered? The file logging has to be done on *some* thread different from the real-time one. Using a TaskQueue seemed the best solution, because * using a custom (possibly existing) thread would require adding signaling mechanisms for passing the thread down to APM/AecDump and communicating with it. Compare the RTC event log: https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... * using a task queue could limit the total number of threads if the task queue was passed from elsewhere. Other tasks could be run on the same task queue. The current plan is to move the task queue in PeerConnectionFactory created/destroyed by the signaling thread. This TQ could then be used for similar tasks in different sub-modules (I think RTC event logging). There is a dependent CL which attempts to do just that: https://codereview.webrtc.org/2888303003. A few problems still remain, mainly the case of using sub-components of WebRTC separately from PeerConnectionFactory. That happens in internal projects.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
Patchset #4 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
solenberg@webrtc.org changed reviewers: + danilchap@webrtc.org, tommi@webrtc.org
lgtm % comments But check if Danil/Tommi have any concerns about adding another TQ instance. https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2896813002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:115: rtc::TaskQueue low_priority_worker_queue_; On 2017/05/23 11:05:49, aleloi wrote: > On 2017/05/22 22:38:00, the sun wrote: > > On some platforms this means we're adding yet another thread. What other > options > > have you considered? > > The file logging has to be done on *some* thread different from the real-time > one. Using a TaskQueue seemed the best solution, because > > * using a custom (possibly existing) thread would require adding signaling > mechanisms for passing the thread down to APM/AecDump and communicating with it. > Compare the RTC event log: > https://cs.chromium.org/chromium/src/third_party/webrtc/logging/rtc_event_log... Sorry about the confusion - that's not what I'm suggesting. Rather something like what you reference below - sharing a thread/task queue for low prio tasks. I'm looping in Danil and Tommi for thoughts on how TaskQueue is implemented - IIRC in Chromium it is overridden with the Chromium impl - does that mean that there is more thread pooling going on behind the scenes than is the case with the pure WebRTC impl? > * using a task queue could limit the total number of threads if the task queue > was passed from elsewhere. Other tasks could be run on the same task queue. > > The current plan is to move the task queue in PeerConnectionFactory > created/destroyed by the signaling thread. This TQ could then be used for > similar tasks in different sub-modules (I think RTC event logging). > > There is a dependent CL which attempts to do just that: > https://codereview.webrtc.org/2888303003. A few problems still remain, mainly > the case of using sub-components of WebRTC separately from > PeerConnectionFactory. That happens in internal projects. https://codereview.webrtc.org/2896813002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2896813002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:696: return true; did you mean 'false'?
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by aleloi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
danilchap@webrtc.org changed reviewers: + perkj@webrtc.org - danilchap@webrtc.org
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
I do not feel like TaskQueue specialist, Per likely has better opinion than me.
tommi@chromium.org changed reviewers: + tommi@chromium.org
lgtm with a couple of requests. Thanks for the offline context, good to know that this will replace several threads down the line. https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:232: : low_priority_worker_queue_("low-prio-worker-queue", can we rename this to e.g. "rtc-low-prio"? 'rtc' is good to distinguish from other threads/queues. 15 chars or less is good to be friendly to some OSen such as Linux. "worker-queue" is kind of redundant since the name will only show up for a worker queue or thread. https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:707: if (aec_dump) { nit: {} not necessary (chromium code usually omits them for 2 line conditionals)
https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:232: : low_priority_worker_queue_("low-prio-worker-queue", On 2017/05/24 08:37:26, tommi (sloooow) - chröme wrote: > can we rename this to e.g. "rtc-low-prio"? > > 'rtc' is good to distinguish from other threads/queues. > 15 chars or less is good to be friendly to some OSen such as Linux. > "worker-queue" is kind of redundant since the name will only show up for a > worker queue or thread. Done. https://codereview.webrtc.org/2896813002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:707: if (aec_dump) { On 2017/05/24 08:37:26, tommi (sloooow) - chröme wrote: > nit: {} not necessary (chromium code usually omits them for 2 line conditionals) The style guide encourages to be consistent with surrounding code. In this file, I've only spotted one place that omits the brackets. I'll keep them for now...
Description was changed from ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. BUG=webrtc:7404 ========== to ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. NOTRY=True # tests just passed BUG=webrtc:7404 ==========
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2896813002/#ps140001 (title: "StartRecording should return 'true' when starting is successfull.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, tommi@chromium.org Link to the patchset: https://codereview.webrtc.org/2896813002/#ps160001 (title: "Give queue shorter name.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495615520564620,
"parent_rev": "d5115e0cacac492ff837bf8aeb26ce13b9b39f47", "commit_rev":
"c61bf947b4ac31f3500858ffcae6fee39d799930"}
Message was sent while issue was closed.
Description was changed from ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. NOTRY=True # tests just passed BUG=webrtc:7404 ========== to ========== Activate 'offload debug dump recordings from audio thread to TaskQueue'. A low priority task queue is added to WebRTCVoiceEngine. The start/stop debug calls make file logging happen on the task queue. In a dependent CL (https://codereview.webrtc.org/2888303003), the task queue is moved to PeerConnectionFactory, so that it can be shared for low priority tasks between different subcomponents. It will require some changes to MediaEngine, CompositeMediaEngine, WebRTCVoiceEngine, and changes in internal projects. A task queue must be created and destroyed from the same thread. With this CL that will be the worker thread, which creates and destroys WebRTCVoiceEngine. With the dependent CL, it will probably change to the signaling thread. NOTRY=True # tests just passed BUG=webrtc:7404 Review-Url: https://codereview.webrtc.org/2896813002 Cr-Commit-Position: refs/heads/master@{#18252} Committed: https://chromium.googlesource.com/external/webrtc/+/c61bf947b4ac31f3500858ffc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/c61bf947b4ac31f3500858ffc...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:160001) has been created in https://codereview.webrtc.org/2904893002/ by aleloi@webrtc.org. The reason for reverting is: Breaks internal project.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
