Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740723002:
Rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper (Closed)
DescriptionRewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper
BUG=webrtc:7301
Review-Url: https://codereview.webrtc.org/2740723002
Cr-Commit-Position: refs/heads/master@{#17161}
Committed: https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa18518e3114c1b8babe3
Patch Set 1 #
Total comments: 5
Patch Set 2 : Move QueuedTask related code from header to .cc file #
Total comments: 6
Patch Set 3 : Implement Sprang@ comments #
Total comments: 2
Patch Set 4 : Implement Tommi@ comments #Messages
Total messages: 38 (16 generated)
ilnik@webrtc.org changed reviewers: + kjellander@webrtc.org, sprang@webrtc.org
It's needed for cases then we create a lot of VideoSendStream's and FrameGeneratorCapturer's on windows because it runs out of multimedia timers and we cant use EventTimeWrapper anymore. I tried to do it as efficiently as possible, but I may have missed some aspects of TaskQueue.
I'll give a rubberstamp lgtm for webrtc/test so please have sprang@ review this, as I've never done any coding with TaskQueue
On 2017/03/08 10:28:47, kjellander_webrtc wrote: > I'll give a rubberstamp lgtm for webrtc/test so please have sprang@ review this, > as I've never done any coding with TaskQueue Erik, just a kindly reminder, that you still need to review this.
https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... webrtc/test/frame_generator_capturer.h:89: }; Don't need to declare this here. Move it to the cc file. I think you may even get away with not declaring a class at all, but can post a lambda capturing this instead?
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... webrtc/test/frame_generator_capturer.h:88: bool repeat_; nit: this could be made const. However, as Erik points out, you could use a lambda, which might make the code even simpler.
On 2017/03/08 10:09:39, ilnik wrote: > It's needed for cases then we create a lot of VideoSendStream's and > FrameGeneratorCapturer's on windows because it runs out of multimedia timers and > we cant use EventTimeWrapper anymore. > > I tried to do it as efficiently as possible, but I may have missed some aspects > of TaskQueue. oh, btw the issue with limited number of multimedia timers can still apply to TaskQueue on Windows (although there's always a fallback on WM_TIMER), but I have a CL in the works that mitigates that limit for the most part (for TQ that is).
https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... webrtc/test/frame_generator_capturer.h:88: bool repeat_; On 2017/03/09 17:46:52, tommi (webrtc) wrote: > nit: this could be made const. > However, as Erik points out, you could use a lambda, which might make the code > even simpler. Done. https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... webrtc/test/frame_generator_capturer.h:89: }; On 2017/03/09 17:40:05, språng wrote: > Don't need to declare this here. Move it to the cc file. > I think you may even get away with not declaring a class at all, but can post a > lambda capturing this instead? I'll try to do it with lambdas, but the issue is how could I repost the task inside the lambda?
https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... webrtc/test/frame_generator_capturer.h:89: }; On 2017/03/09 18:25:56, ilnik wrote: > On 2017/03/09 17:40:05, språng wrote: > > Don't need to declare this here. Move it to the cc file. > > I think you may even get away with not declaring a class at all, but can post > a > > lambda capturing this instead? > > I'll try to do it with lambdas, but the issue is how could I repost the task > inside the lambda? Oh, I didn't realize that on the first pass. Right, you can't do that from a lambda - but it's nice to see you're taking advantage of that. Can you add a comment where you use the repeat_ variable that points out to the reader what's going on?
On 2017/03/09 18:56:02, tommi (webrtc) wrote: > https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... > File webrtc/test/frame_generator_capturer.h (right): > > https://codereview.webrtc.org/2740723002/diff/1/webrtc/test/frame_generator_c... > webrtc/test/frame_generator_capturer.h:89: }; > On 2017/03/09 18:25:56, ilnik wrote: > > On 2017/03/09 17:40:05, språng wrote: > > > Don't need to declare this here. Move it to the cc file. > > > I think you may even get away with not declaring a class at all, but can > post > > a > > > lambda capturing this instead? > > > > I'll try to do it with lambdas, but the issue is how could I repost the task > > inside the lambda? > > Oh, I didn't realize that on the first pass. Right, you can't do that from a > lambda - but it's nice to see you're taking advantage of that. Can you add a > comment where you use the repeat_ variable that points out to the reader what's > going on? New version has TaskQueue related staff moved to .cc file. Lambda's didn't work out, but now there are comments describing all non-trivial staff. I hope it's OK now.
The CQ bit was checked by ilnik@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/...
looks good, just some nits https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.cc (right): https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:48: webrtc::test::FrameGeneratorCapturer* frame_generator_capturer_; nit: webrtc::test::FrameGeneratorCapturer* const frame_generator_capturer_; ? https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:49: const uint32_t repeat_interval_ms_; nit: extra whitespace, also we mostly use int64_t type for ms timestamps so might be good to do the same here https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.h:77: friend class InsertFrameTask; Isn't it enough to forward declare it as an inner class?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.cc (right): https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:49: const uint32_t repeat_interval_ms_; On 2017/03/10 09:18:45, språng wrote: > nit: extra whitespace, also we mostly use int64_t type for ms timestamps so > might be good to do the same here PostDealyedTask takes uin32_t. Therefore it's better to make interface clear on that. https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.h (right): https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.h:77: friend class InsertFrameTask; On 2017/03/10 09:18:45, språng wrote: > Isn't it enough to forward declare it as an inner class? Done.
lgtm https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.cc (right): https://codereview.webrtc.org/2740723002/diff/20001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:49: const uint32_t repeat_interval_ms_; On 2017/03/10 09:27:19, ilnik wrote: > On 2017/03/10 09:18:45, språng wrote: > > nit: extra whitespace, also we mostly use int64_t type for ms timestamps so > > might be good to do the same here > > PostDealyedTask takes uin32_t. Therefore it's better to make interface clear on > that. Right, makes sense. Also I guess it's a delta, not an absolute time.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2740723002/#ps40001 (title: "Implement Sprang@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm. nice change and thanks for removing one more use of EventTimerWrapper :) https://codereview.webrtc.org/2740723002/diff/40001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.cc (right): https://codereview.webrtc.org/2740723002/diff/40001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:29: // Repeats in |repeat_interval_ms|. One-time if |repeat_interval_ms| <= 0. nit: |repeat_interval_ms_| (missing underscore), also it can't be < 0
The CQ bit was unchecked by ilnik@webrtc.org
The CQ bit was checked by ilnik@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/...
https://codereview.webrtc.org/2740723002/diff/40001/webrtc/test/frame_generat... File webrtc/test/frame_generator_capturer.cc (right): https://codereview.webrtc.org/2740723002/diff/40001/webrtc/test/frame_generat... webrtc/test/frame_generator_capturer.cc:29: // Repeats in |repeat_interval_ms|. One-time if |repeat_interval_ms| <= 0. On 2017/03/10 09:36:32, tommi (webrtc) wrote: > nit: |repeat_interval_ms_| (missing underscore), also it can't be < 0 Nice catch about negative values. The naming is correct: it's supposed to be the parameter of the constructor.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, sprang@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2740723002/#ps60001 (title: "Implement Tommi@ comments")
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": 60001, "attempt_start_ts": 1489138813146360,
"parent_rev": "05a087802b3494ec448f191722c2c57f9b3a507c", "commit_rev":
"7c5503a8b30ae43a8ccaa18518e3114c1b8babe3"}
Message was sent while issue was closed.
Description was changed from ========== Rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper BUG=webrtc:7031 ========== to ========== Rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper BUG=webrtc:7031 Review-Url: https://codereview.webrtc.org/2740723002 Cr-Commit-Position: refs/heads/master@{#17161} Committed: https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851...
Message was sent while issue was closed.
On 2017/03/10 10:21:59, commit-bot: I haz the power wrote: > Committed patchset #4 (id:60001) as > https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... I received an update on the bug webrtc:7031 but I think that it is not related to this CL so we should probably change the BUG field in the description.
Message was sent while issue was closed.
Description was changed from ========== Rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper BUG=webrtc:7031 Review-Url: https://codereview.webrtc.org/2740723002 Cr-Commit-Position: refs/heads/master@{#17161} Committed: https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... ========== to ========== Rewrite frame generator capturer to use TaskQueue instead of EventTimeWrapper BUG=webrtc:7301 Review-Url: https://codereview.webrtc.org/2740723002 Cr-Commit-Position: refs/heads/master@{#17161} Committed: https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... ==========
Message was sent while issue was closed.
On 2017/03/10 11:35:38, mbonadei wrote: > On 2017/03/10 10:21:59, commit-bot: I haz the power wrote: > > Committed patchset #4 (id:60001) as > > > https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... > > I received an update on the bug webrtc:7031 but I think that it is not related > to > this CL so we should probably change the BUG field in the description. That should be 7301 I believe :)
Message was sent while issue was closed.
On 2017/03/10 11:35:38, mbonadei wrote: > On 2017/03/10 10:21:59, commit-bot: I haz the power wrote: > > Committed patchset #4 (id:60001) as > > > https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... > > I received an update on the bug webrtc:7031 but I think that it is not related > to > this CL so we should probably change the BUG field in the description. Sorry, that was a typo.
Message was sent while issue was closed.
On 2017/03/10 12:00:51, ilnik wrote: > On 2017/03/10 11:35:38, mbonadei wrote: > > On 2017/03/10 10:21:59, commit-bot: I haz the power wrote: > > > Committed patchset #4 (id:60001) as > > > > > > https://chromium.googlesource.com/external/webrtc/+/7c5503a8b30ae43a8ccaa1851... > > > > I received an update on the bug webrtc:7031 but I think that it is not related > > to > > this CL so we should probably change the BUG field in the description. > > Sorry, that was a typo. This commit somehow broke CallPerfTest.ReceivesCpuOveruseAndUnderuse on Android. Reverting.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.webrtc.org/2739393002/ by ilnik@webrtc.org. The reason for reverting is: Less precise FrameGeneratorCapturer broke CallPerfTest.ReceivesCpuOveruseAndUnderuse on Android. |
