Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(273)

Issue 1845563006: Make RTCVideoEncoder usable from non-postable thread. (Closed)

Created:
4 years, 8 months ago by wuchengli
Modified:
4 years, 8 months ago
Reviewers:
emircan, kcwu, pbos, mcasas
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch+vc_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, Pawel Osciak
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make RTCVideoEncoder usable from non-postable thread. - The only thing RtcVideoEncoder should rely on is it being synchronized from the outside (never called concurrently). It cannot assume the calling thread is postable. - RtcVideoEncoder needs to synchronize RegisterEncodeCompleteCallback and encode complete callback. BUG=chromium:595308 TEST=Revert https://codereview.webrtc.org/1821983002. Then try apprtc loopback and Hangout on oak. Committed: https://crrev.com/7aa249c90c143f50b554905c5dd515a8b25ef0ce Cr-Commit-Position: refs/heads/master@{#388463}

Patch Set 1 #

Total comments: 16

Patch Set 2 : address review comments #

Total comments: 12

Patch Set 3 : address review comments and small changes #

Patch Set 4 : rebase #

Total comments: 16

Patch Set 5 : address all review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -179 lines) Patch
M content/renderer/media/rtc_video_encoder.h View 1 2 3 2 chunks +2 lines, -32 lines 0 comments Download
M content/renderer/media/rtc_video_encoder.cc View 1 2 3 4 18 chunks +175 lines, -147 lines 0 comments Download

Messages

Total messages: 44 (18 generated)
pbos
some nits if I'm not misreading this https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc#newcode449 content/renderer/media/rtc_video_encoder.cc:449: scoped_ptr<webrtc::EncodedImage> image(new ...
4 years, 8 months ago (2016-03-31 09:47:21 UTC) #2
wuchengli
Adding Pawel and Kuang-che so they can start reviewing. I'll address the review comments and ...
4 years, 8 months ago (2016-04-01 09:33:23 UTC) #4
wuchengli
https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc#newcode449 content/renderer/media/rtc_video_encoder.cc:449: scoped_ptr<webrtc::EncodedImage> image(new webrtc::EncodedImage( On 2016/03/31 09:47:21, pbos wrote: > ...
4 years, 8 months ago (2016-04-01 10:19:51 UTC) #5
kcwu
https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_video_encoder.cc#newcode290 content/renderer/media/rtc_video_encoder.cc:290: } I don't understand the original code. There is ...
4 years, 8 months ago (2016-04-01 12:33:05 UTC) #6
wuchengli
Addressed all review comments. I haven't tested it though. Note Mon and Tue are holidays ...
4 years, 8 months ago (2016-04-03 14:28:01 UTC) #7
kcwu
https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/rtc_video_encoder.cc#newcode704 content/renderer/media/rtc_video_encoder.cc:704: : video_codec_type_(type), no longer used.
4 years, 8 months ago (2016-04-03 18:53:46 UTC) #8
pbos
lgtm, but definitely wait until someone who is more familiar with this code has reviewed ...
4 years, 8 months ago (2016-04-04 09:00:31 UTC) #9
wuchengli
All done. mcasas@ owner review. Thanks. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/rtc_video_encoder.cc#newcode137 content/renderer/media/rtc_video_encoder.cc:137: void Destroy(base::WaitableEvent* waiter); ...
4 years, 8 months ago (2016-04-06 07:51:58 UTC) #15
kcwu
lgtm
4 years, 8 months ago (2016-04-06 08:52:44 UTC) #16
pbos
mcasas@: Ping, do you have time to take a look?
4 years, 8 months ago (2016-04-11 09:38:52 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845563006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845563006/80001
4 years, 8 months ago (2016-04-11 14:34:31 UTC) #19
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/202771)
4 years, 8 months ago (2016-04-11 15:01:13 UTC) #21
mcasas
passing the token to emircan@ who has worked recently in the RTCVideoEncoder class.
4 years, 8 months ago (2016-04-11 23:36:17 UTC) #23
pbos
On 2016/04/11 23:36:17, mcasas wrote: > passing the token to emircan@ who has > worked ...
4 years, 8 months ago (2016-04-12 09:34:28 UTC) #24
pbos
https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/rtc_video_encoder.cc#newcode250 content/renderer/media/rtc_video_encoder.cc:250: std::atomic<int32_t> status_; std::atomic looks banned for now, I think ...
4 years, 8 months ago (2016-04-12 11:25:59 UTC) #25
emircan
lgtm % nits below. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/rtc_video_encoder.cc#newcode95 content/renderer/media/rtc_video_encoder.cc:95: // thread on which the ...
4 years, 8 months ago (2016-04-12 20:15:03 UTC) #26
wuchengli
Addressed all review comments. Tomorrow I'll test again and merge this. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/rtc_video_encoder.cc File content/renderer/media/rtc_video_encoder.cc (right): ...
4 years, 8 months ago (2016-04-13 15:26:26 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845563006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845563006/100001
4 years, 8 months ago (2016-04-14 05:06:37 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/168474)
4 years, 8 months ago (2016-04-14 05:15:31 UTC) #32
wuchengli
mcasas@ need rubber stamp LGT-M from you. Thanks.
4 years, 8 months ago (2016-04-14 05:32:18 UTC) #33
mcasas
On 2016/04/14 05:32:18, wuchengli wrote: > mcasas@ need rubber stamp LGT-M from you. Thanks. RS ...
4 years, 8 months ago (2016-04-18 18:14:11 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845563006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845563006/100001
4 years, 8 months ago (2016-04-19 00:47:11 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/55906)
4 years, 8 months ago (2016-04-19 01:39:57 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845563006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845563006/100001
4 years, 8 months ago (2016-04-20 09:42:28 UTC) #40
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years, 8 months ago (2016-04-20 10:41:12 UTC) #42
commit-bot: I haz the power
4 years, 8 months ago (2016-04-22 19:22:21 UTC) #44
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/7aa249c90c143f50b554905c5dd515a8b25ef0ce
Cr-Commit-Position: refs/heads/master@{#388463}

Powered by Google App Engine
This is Rietveld 408576698