Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3003643002:
Allow external TaskQueue implementations on Linux (Closed)
|
Created:
3 years, 4 months ago by perkj_webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis cl refactor TaskQueues to use a PIMPL implementation on linux/Android.
In later steps the Win/Mac implementation will also be refactored.
The rtc_task_queue target is split up in three separate targets:
rtc_task_queue_api:
Contains the header file task_queue.h but no implementation.
Only external TaskQueue implementations should directly depend on this target.
rtc_task_queue_impl:
Contains the default implementation of task_queue.h.
Only external application targets should directly depend on this target.
rtc_task_queue:
WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl.
BUG=webrtc:8160
Review-Url: https://codereview.webrtc.org/3003643002
Cr-Commit-Position: refs/heads/master@{#19516}
Committed: https://chromium.googlesource.com/external/webrtc/+/650fdae91c690fb9904f72873daeb81415ff9ba1
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix deps #Patch Set 3 : Switched to use rtc_source_set instead. #
Total comments: 14
Patch Set 4 : Add rtc_task_queue_api target. #
Total comments: 3
Patch Set 5 : Removed special link for libjingle_peerconnection. Add comments to build targets. #Patch Set 6 : .. #
Messages
Total messages: 49 (32 generated)
The CQ bit was checked by perkj@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/...
perkj@webrtc.org changed reviewers: + kwiberg@webrtc.org, mbonadei@webrtc.org, nisse@webrtc.org
Here is the link time cl.
On 2017/08/22 13:06:51, perkj_webrtc wrote: > Here is the link time cl. But I have to fix the deps in a better way.....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24172)
https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (left): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/BUILD.gn#oldc... webrtc/rtc_base/BUILD.gn:304: "../../webrtc_overrides/webrtc/rtc_base/task_queue.cc", Ah, Chrome already overrides task queue, I had forgotten about that. Can we arrange so that it use the new mechanism too (and not override any header files)? Otherwise, we get into a mess with multiple ways the task queue implementation can be replaced. https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue.h File webrtc/rtc_base/task_queue.h (right): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue.h#... webrtc/rtc_base/task_queue.h:268: const scoped_refptr<Impl> impl_; I would have expected a unique_ptr here. Who else will keep a reference to the implementation object? https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... File webrtc/rtc_base/task_queue_libevent.cc (left): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... webrtc/rtc_base/task_queue_libevent.cc:277: bool TaskQueue::IsCurrent(const char* queue_name) { Not new in this cl, but this looks questionable to me, with string ids. Where is it used? https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... File webrtc/rtc_base/task_queue_libevent.cc (right): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... webrtc/rtc_base/task_queue_libevent.cc:494: TaskQueue::TaskQueue(const char* queue_name, Priority priority) So an overriding implementation is supposed to define rtc::TaskQueue::TaskQueue (constructor) rtc::TaskQueue::Impl (private class) and it would also need to copy the other TaskQueue::* wrapper methods. I'd prefer a separate namespace for the implementation, which will be populated by either application or webrtc, but not both. As far as I understand, that interface would have to be plain functions (like a C library), not a class, due to limitations in how C++ works.
https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (left): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/BUILD.gn#oldc... webrtc/rtc_base/BUILD.gn:304: "../../webrtc_overrides/webrtc/rtc_base/task_queue.cc", On 2017/08/22 13:39:44, nisse-webrtc wrote: > Ah, Chrome already overrides task queue, I had forgotten about that. Can we > arrange so that it use the new mechanism too (and not override any header > files)? Otherwise, we get into a mess with multiple ways the task queue > implementation can be replaced. yes, if I can get some help. https://cs.chromium.org/chromium/src/third_party/webrtc_overrides/webrtc/rtc_... https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue.h File webrtc/rtc_base/task_queue.h (right): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue.h#... webrtc/rtc_base/task_queue.h:268: const scoped_refptr<Impl> impl_; On 2017/08/22 13:39:44, nisse-webrtc wrote: > I would have expected a unique_ptr here. Who else will keep a reference to the > implementation object? To be used to implement PostAndReply on a different response queue. I already have an implementation that uses it. The chrome implementation do something similar but in another level of indirection. the tq implementation holds a uniqe_ptr to a thread. but the chrome thread has a refcounted task runner that the reply tasks holds a reference to. https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... File webrtc/rtc_base/task_queue_libevent.cc (left): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... webrtc/rtc_base/task_queue_libevent.cc:277: bool TaskQueue::IsCurrent(const char* queue_name) { On 2017/08/22 13:39:44, nisse-webrtc wrote: > Not new in this cl, but this looks questionable to me, with string ids. Where is > it used? it probably isnt. https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... File webrtc/rtc_base/task_queue_libevent.cc (right): https://codereview.webrtc.org/3003643002/diff/1/webrtc/rtc_base/task_queue_li... webrtc/rtc_base/task_queue_libevent.cc:494: TaskQueue::TaskQueue(const char* queue_name, Priority priority) On 2017/08/22 13:39:44, nisse-webrtc wrote: > So an overriding implementation is supposed to define > > rtc::TaskQueue::TaskQueue (constructor) > rtc::TaskQueue::Impl (private class) > > and it would also need to copy the other TaskQueue::* wrapper methods. > > I'd prefer a separate namespace for the implementation, which will be populated > by either application or webrtc, but not both. As far as I understand, that > interface would have to be plain functions (like a C library), not a class, due > to limitations in how C++ works. I think it is ok to copy paste the boiler plate code but if not I would much prefer a pure virtual interface to using function pointers. Similare to the other cl. And yes, it can be a separate class, not an inner class if you prefer.
The CQ bit was checked by perkj@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: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
The CQ bit was checked by perkj@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.
Description was changed from ========== Test of how a TaskQueue PIMPL implementation could look like. Selecting implementation durign link time. BUG=none ========== to ========== Test of how a TaskQueue PIMPL implementation could look like. Selecting implementation durign link time. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:bot1,bot2;master.tryserver.chromium.mac:bot3 BUG=none ==========
The CQ bit was checked by perkj@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: bot2 on master.tryserver.chromium.linux (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Description was changed from ========== Test of how a TaskQueue PIMPL implementation could look like. Selecting implementation durign link time. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:bot1,bot2;master.tryserver.chromium.mac:bot3 BUG=none ========== to ========== Test of how a TaskQueue PIMPL implementation could look like. Selecting implementation durign link time. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:bot1 BUG=none ==========
The CQ bit was checked by perkj@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: bot1 on master.tryserver.chromium.linux (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
I'm waiting for Karl's comments, he had read up on the recommended way to do pimpl, which probably is pretty close to what you do in this cl. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. Why? It's not a test target, right? https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:303: public_deps += [ ":rtc_task_queue_impl" ] I was thinking of adding the dependency on rtc_task_queue_impl in the top-level library-like targets. But maybe it makes more sense here. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:328: "task_queue.h", It's not so nice to task_queue.h in multiple targets (do we do that anywhere else?). Would it make sense with a separate _api target for just the header file?
https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. On 2017/08/23 11:00:25, nisse-webrtc wrote: > Why? It's not a test target, right? To not have to update all projects that depend on this. We can add a new target don't have this deps if we want. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:303: public_deps += [ ":rtc_task_queue_impl" ] On 2017/08/23 11:00:25, nisse-webrtc wrote: > I was thinking of adding the dependency on rtc_task_queue_impl in the top-level > library-like targets. But maybe it makes more sense here. That is what I did in a previous cl. But I think its nicer that targets does not have to care if the implementation is linked or not and we don't need to change the existing deps. If you want to have a tq you simply depend on this target and do not have to care. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:328: "task_queue.h", On 2017/08/23 11:00:25, nisse-webrtc wrote: > It's not so nice to task_queue.h in multiple targets (do we do that anywhere > else?). Would it make sense with a separate _api target for just the header > file? Sure
Looks pretty good. We need to agree on scoped_ref_ptr vs unique_ptr, and then I'd only ask for doc updates: Update description (including correct spelling of "during"), and any appropriate TODO comments regarding conversion of the mac, windows and chromium implementations. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. On 2017/08/23 11:12:37, perkj_webrtc wrote: > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > Why? It's not a test target, right? > > To not have to update all projects that depend on this. We can add a new target > don't have this deps if we want. Ok, we can refine it later. As far as I understand, we should have an unconditional dependency for targets that build tests and our own example apps, but not for library targets intended to be used by external applications. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:303: public_deps += [ ":rtc_task_queue_impl" ] On 2017/08/23 11:12:37, perkj_webrtc wrote: > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > I was thinking of adding the dependency on rtc_task_queue_impl in the > top-level > > library-like targets. But maybe it makes more sense here. > > If you want to have a tq you simply depend on this target and > do not have to care. Ok. We could refine it to say that whoever instantiates a task queue needs this target, while code that only get a task queue passed in can depend on an api target only. I'd suggest we move task_queue.h to api/ later on, and then we can revisit this.
https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:328: "task_queue.h", On 2017/08/23 11:12:37, perkj_webrtc wrote: > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > It's not so nice to task_queue.h in multiple targets (do we do that anywhere > > else?). Would it make sense with a separate _api target for just the header > > file? > > Sure I agree, we should create a separate _api target. With that I think we will be able to avoid to expose :rtc_task_queue_impl in the public_deps list of rtc_task_queue.
Looks very good! But it's obvious that we have a long series of cleanup CLs ahead of us once this sort of thing has landed, to make *all* the various implementations use the same mechanism... https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. On 2017/08/23 11:24:46, nisse-webrtc wrote: > On 2017/08/23 11:12:37, perkj_webrtc wrote: > > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > > Why? It's not a test target, right? > > > > To not have to update all projects that depend on this. We can add a new > target > > don't have this deps if we want. > > Ok, we can refine it later. As far as I understand, we should have an > unconditional dependency for targets that build tests and our own example apps, > but not for library targets intended to be used by external applications. Hmm. I wonder if a gn variable (rtc_link_task_queue_impl) is too coarse-grained? It will force the same setting on all build targets---or workarounds like this one, which make the meaning of the gn variable unpleasantly complex. Is it fair to say that what we want long-term is for our test binaries to unconditionally link with the default TQ implementation, and for the library (this target?) to unconditionally *not* link with the default TQ implementation? *All* users of the library target would then have to manually link with their preferred TQ implementation. For the shorter term, we might have a gn flag that says whether the library should link with the default TQ implementation. First this would default to true, then to false, and finally we can remove it completely. https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/task_queue.h File webrtc/rtc_base/task_queue.h (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/task_queu... webrtc/rtc_base/task_queue.h:268: const scoped_refptr<Impl> impl_; const---nice! https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/task_queu... File webrtc/rtc_base/task_queue_libevent.cc (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/rtc_base/task_queu... webrtc/rtc_base/task_queue_libevent.cc:115: Priority priority = Priority::NORMAL); In a local function like this, which doesn't have that many callers, it's usually more readable to not have default values for arguments. https://codereview.webrtc.org/3003643002/diff/60001/webrtc/rtc_base/BUILD.gn File webrtc/rtc_base/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/60001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:308: } I think it would be better to not have this target, and instead have all TQ users depend on _api. That way, our test targets can unconditionally depend on _impl, and our library target(s) can depend on _impl if rtc_link_task_queue_impl is true. (See the long comment in another file where I expand on this.) https://codereview.webrtc.org/3003643002/diff/60001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:310: rtc_source_set("rtc_task_queue_api") { Note in a comment that anyone who uses this needs to link with a TQ implementation? The normal convention is that build targets have dependencies on everything they need, and this one is an exception to that rule. https://codereview.webrtc.org/3003643002/diff/60001/webrtc/rtc_base/BUILD.gn#... webrtc/rtc_base/BUILD.gn:325: rtc_source_set("rtc_task_queue_impl") { Excellent with the _api and _impl targets. Things are split between them just like I would want.
https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. On 2017/08/23 13:23:25, kwiberg-webrtc wrote: > On 2017/08/23 11:24:46, nisse-webrtc wrote: > > On 2017/08/23 11:12:37, perkj_webrtc wrote: > > > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > > > Why? It's not a test target, right? > > > > > > To not have to update all projects that depend on this. We can add a new > > target > > > don't have this deps if we want. > > > > Ok, we can refine it later. As far as I understand, we should have an > > unconditional dependency for targets that build tests and our own example > apps, > > but not for library targets intended to be used by external applications. > > Hmm. I wonder if a gn variable (rtc_link_task_queue_impl) is too coarse-grained? > It will force the same setting on all build targets---or workarounds like this > one, which make the meaning of the gn variable unpleasantly complex. > > Is it fair to say that what we want long-term is for our test binaries to > unconditionally link with the default TQ implementation, and for the library > (this target?) to unconditionally *not* link with the default TQ implementation? > *All* users of the library target would then have to manually link with their > preferred TQ implementation. > > For the shorter term, we might have a gn flag that says whether the library > should link with the default TQ implementation. First this would default to > true, then to false, and finally we can remove it completely. For an application, it makes sense to have a single webrtc "thing" to link with, be that a gn target, or a shared or static library, to get everything needed to use webrtc. That target/library ought to include the default tq. And if it's a important use case to have all of webrtc except the default task queue, a flag doing just that could make sense. Then, in addition, we'll want more fine-grained targets too, to let applications grab only the building blocks they need. These fine-grained targets shouldn't depend on the task_queue implementation. gn doesn't have any notion of alternatives or weak dependencies? Ideally, it would be nice to express to gn that a piece of code needs some task queue implementation, with a weak dependency on the default implementation, meaning that if none of the regular transitive dependencies for a top-level target brings in any task queue implementation, the default should be linked in.
> gn doesn't have any notion of alternatives or weak dependencies? Ideally, it > would be nice to express to gn that a piece of code needs some task queue > implementation, with a weak dependency on the default implementation, meaning > that if none of the regular transitive dependencies for a top-level target > brings in any task queue implementation, the default should be linked in. No, unfortunately I am not aware of something like this in gn.
The CQ bit was checked by perkj@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: bot1 on master.tryserver.chromium.linux (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Description was changed from ========== Test of how a TaskQueue PIMPL implementation could look like. Selecting implementation durign link time. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:bot1 BUG=none ========== to ========== This cl refactor TaskQueues to use a PIMPL implementation on linux/Android. In later steps the Win/Mac implementation will also be refactored. The rtc_task_queue target is split up in three separate targets: rtc_task_queue_api: Contains the header file task_queue.h but no implementation. Only external TaskQueue implementations should directly depend on this target. rtc_task_queue_impl: Contains the default implementation of task_queue.h. Only external application targets should directly depend on this target. rtc_task_queue: WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl. BUG=none ==========
Description was changed from ========== This cl refactor TaskQueues to use a PIMPL implementation on linux/Android. In later steps the Win/Mac implementation will also be refactored. The rtc_task_queue target is split up in three separate targets: rtc_task_queue_api: Contains the header file task_queue.h but no implementation. Only external TaskQueue implementations should directly depend on this target. rtc_task_queue_impl: Contains the default implementation of task_queue.h. Only external application targets should directly depend on this target. rtc_task_queue: WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl. BUG=none ========== to ========== This cl refactor TaskQueues to use a PIMPL implementation on linux/Android. In later steps the Win/Mac implementation will also be refactored. The rtc_task_queue target is split up in three separate targets: rtc_task_queue_api: Contains the header file task_queue.h but no implementation. Only external TaskQueue implementations should directly depend on this target. rtc_task_queue_impl: Contains the default implementation of task_queue.h. Only external application targets should directly depend on this target. rtc_task_queue: WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl. BUG=webrtc:8160 ==========
ok, I would like to land this now. Can you please review? https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/3003643002/diff/40001/webrtc/pc/BUILD.gn#newcod... webrtc/pc/BUILD.gn:230: # regardless of the flag |rtc_link_task_queue_impl|. On 2017/08/23 14:16:01, nisse-webrtc wrote: > On 2017/08/23 13:23:25, kwiberg-webrtc wrote: > > On 2017/08/23 11:24:46, nisse-webrtc wrote: > > > On 2017/08/23 11:12:37, perkj_webrtc wrote: > > > > On 2017/08/23 11:00:25, nisse-webrtc wrote: > > > > > Why? It's not a test target, right? > > > > > > > > To not have to update all projects that depend on this. We can add a new > > > target > > > > don't have this deps if we want. > > > > > > Ok, we can refine it later. As far as I understand, we should have an > > > unconditional dependency for targets that build tests and our own example > > apps, > > > but not for library targets intended to be used by external applications. > > > > Hmm. I wonder if a gn variable (rtc_link_task_queue_impl) is too > coarse-grained? > > It will force the same setting on all build targets---or workarounds like this > > one, which make the meaning of the gn variable unpleasantly complex. > > > > Is it fair to say that what we want long-term is for our test binaries to > > unconditionally link with the default TQ implementation, and for the library > > (this target?) to unconditionally *not* link with the default TQ > implementation? > > *All* users of the library target would then have to manually link with their > > preferred TQ implementation. > > > > For the shorter term, we might have a gn flag that says whether the library > > should link with the default TQ implementation. First this would default to > > true, then to false, and finally we can remove it completely. > > For an application, it makes sense to have a single webrtc "thing" to link with, > be that a gn target, or a shared or static library, to get everything needed to > use webrtc. That target/library ought to include the default tq. And if it's a > important use case to have all of webrtc except the default task queue, a flag > doing just that could make sense. > > Then, in addition, we'll want more fine-grained targets too, to let applications > grab only the building blocks they need. These fine-grained targets shouldn't > depend on the task_queue implementation. > > gn doesn't have any notion of alternatives or weak dependencies? Ideally, it > would be nice to express to gn that a piece of code needs some task queue > implementation, with a weak dependency on the default implementation, meaning > that if none of the regular transitive dependencies for a top-level target > brings in any task queue implementation, the default should be linked in. It turns out that we don't need this exception. The flag can just mean, link or not link.
lgtm
lgtm. I haven't seen any objections to your use of scoped_ref_ptr, but maybe check with tommi before landing?
On 2017/08/25 11:26:11, nisse-webrtc wrote: > lgtm. > > I haven't seen any objections to your use of scoped_ref_ptr, but maybe check > with tommi before landing? na,,,
The CQ bit was checked by perkj@webrtc.org
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": 100001, "attempt_start_ts": 1503660432921800,
"parent_rev": "ee95f874885a8489bebc54bae9c3bec26884f858", "commit_rev":
"650fdae91c690fb9904f72873daeb81415ff9ba1"}
Message was sent while issue was closed.
Description was changed from ========== This cl refactor TaskQueues to use a PIMPL implementation on linux/Android. In later steps the Win/Mac implementation will also be refactored. The rtc_task_queue target is split up in three separate targets: rtc_task_queue_api: Contains the header file task_queue.h but no implementation. Only external TaskQueue implementations should directly depend on this target. rtc_task_queue_impl: Contains the default implementation of task_queue.h. Only external application targets should directly depend on this target. rtc_task_queue: WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl. BUG=webrtc:8160 ========== to ========== This cl refactor TaskQueues to use a PIMPL implementation on linux/Android. In later steps the Win/Mac implementation will also be refactored. The rtc_task_queue target is split up in three separate targets: rtc_task_queue_api: Contains the header file task_queue.h but no implementation. Only external TaskQueue implementations should directly depend on this target. rtc_task_queue_impl: Contains the default implementation of task_queue.h. Only external application targets should directly depend on this target. rtc_task_queue: WebRTC targets should depend on this target. It unconditionally depend on rtc_task_queue_api and depending on the new build flag,|rtc_link_task_queue_impl|, depend on rtc_task_queue_impl. BUG=webrtc:8160 Review-Url: https://codereview.webrtc.org/3003643002 Cr-Commit-Position: refs/heads/master@{#19516} Committed: https://chromium.googlesource.com/external/webrtc/+/650fdae91c690fb9904f72873... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/650fdae91c690fb9904f72873... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
