|
|
Created:
4 years, 8 months ago by wuchengli Modified:
4 years, 8 months ago 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. |
DescriptionMake 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 #
Messages
Total messages: 44 (18 generated)
pbos@chromium.org changed reviewers: + pbos@chromium.org
some nits if I'm not misreading this https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:449: scoped_ptr<webrtc::EncodedImage> image(new webrtc::EncodedImage( This can be stack allocated now, right? https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:624: scoped_ptr<webrtc::EncodedImage> image, And this could be a const webrtc::EncodedImage&, no?
wuchengli@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org
Adding Pawel and Kuang-che so they can start reviewing. I'll address the review comments and improve the code about propagating status soon.
https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... 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: > This can be stack allocated now, right? Yes. I'll update the code in the next patchset. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:624: scoped_ptr<webrtc::EncodedImage> image, On 2016/03/31 09:47:21, pbos wrote: > And this could be a const webrtc::EncodedImage&, no? Yes. I'll update the code in the next patchset.
https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:290: } I don't understand the original code. There is no SignalAsyncWaiter(OK) for Impl::CreateAndInitializeVEA successful case. Why RTCVideoEncoder won't wait forever at line 720. Do I miss something? https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:302: SignalAsyncWaiter(status_); return here? https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:483: status_ = retval; In the original code, if any error occurs, RTCVideoEncoder will keep the error code and destroy Impl. With your change, Impl will be still alive after NotifyError. RTCVideoEncoder may still response OK for certain methods, ex. SetRates. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:684: impl_status_(WEBRTC_VIDEO_CODEC_UNINITIALIZED) { impl_status_ becomes useless https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:778: base::Bind(&RTCVideoEncoder::Impl::Destroy, impl_, &encode_waiter)); I'm wondering maybe we need to get async_retval here. This is the last chance for client to know anything going wrong. Consider this case: 1. client call ::Encode() and the frame is successfully enqueued. 2. something wrong in the encoder, it triggered NotifyError. But we just keep the error in status_. 3. if the client doesn't call ::Encode again, the client never know something wrong. Anyway, this is just corner case, may not worth to handle. up to you.
Addressed all review comments. I haven't tested it though. Note Mon and Tue are holidays in Taiwan. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:290: } On 2016/04/01 12:33:05, kcwu wrote: > I don't understand the original code. > > There is no SignalAsyncWaiter(OK) for Impl::CreateAndInitializeVEA successful > case. Why RTCVideoEncoder won't wait forever at line 720. Do I miss something? VEA will call RequireBitstreamBuffers after initialization. The waiter is signaled in RequireBitstreamBuffers. I've added a comment here. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:302: SignalAsyncWaiter(status_); On 2016/04/01 12:33:05, kcwu wrote: > return here? > Done. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:449: scoped_ptr<webrtc::EncodedImage> image(new webrtc::EncodedImage( On 2016/04/01 10:19:51, wuchengli wrote: > On 2016/03/31 09:47:21, pbos wrote: > > This can be stack allocated now, right? > Yes. I'll update the code in the next patchset. Done. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:483: status_ = retval; On 2016/04/01 12:33:05, kcwu wrote: > In the original code, if any error occurs, RTCVideoEncoder will keep the error > code and destroy Impl. > > With your change, Impl will be still alive after NotifyError. > RTCVideoEncoder may still response OK for certain methods, ex. SetRates. Moved impl_status_ to Impl. Now all methods will check Impl.status_. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:624: scoped_ptr<webrtc::EncodedImage> image, On 2016/04/01 10:19:51, wuchengli wrote: > On 2016/03/31 09:47:21, pbos wrote: > > And this could be a const webrtc::EncodedImage&, no? > Yes. I'll update the code in the next patchset. Done. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:684: impl_status_(WEBRTC_VIDEO_CODEC_UNINITIALIZED) { On 2016/04/01 12:33:05, kcwu wrote: > impl_status_ becomes useless Removed. Added Impl.status_. https://codereview.chromium.org/1845563006/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_video_encoder.cc:778: base::Bind(&RTCVideoEncoder::Impl::Destroy, impl_, &encode_waiter)); On 2016/04/01 12:33:05, kcwu wrote: > I'm wondering maybe we need to get async_retval here. This is the last chance > for client to know anything going wrong. > > Consider this case: > 1. client call ::Encode() and the frame is successfully enqueued. > 2. something wrong in the encoder, it triggered NotifyError. But we just keep > the error in status_. > 3. if the client doesn't call ::Encode again, the client never know something > wrong. > > Anyway, this is just corner case, may not worth to handle. up to you. I don't think we should return error in Release. Release should always succeed. But I haven't looked at webrtc code to confirm.
https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:704: : video_codec_type_(type), no longer used.
lgtm, but definitely wait until someone who is more familiar with this code has reviewed it https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:137: void Destroy(base::WaitableEvent* waiter); Should this also be named async_waiter for consistency with below? https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:311: if (status_.load() != WEBRTC_VIDEO_CODEC_OK) { Store this status_.load() instead of reading it twice. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:725: WebRTCVideoCodecToVideoCodecProfile(impl_->video_codec_type(), codec_settings); line length (run git cl format) https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:780: return encode_retval;; remove ; https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:799: return impl_->GetStatus(); I think this should still return WEBRTC_VIDEO_CODEC_OK because the other return values are not from SetChannelParameters(). Though this value is ignored regardless, so I think you can just return OK.
Description was changed from ========== Make RTCVideoEncoder usable from non-postable thread. BUG=chromium:595308 TEST=Revert https://codereview.webrtc.org/1821983002. Then try apprtc loopback and Hangout on oak. ========== to ========== 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. ==========
Description was changed from ========== 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. ========== to ========== 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. ==========
Description was changed from ========== 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. ========== to ========== 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. ==========
Patchset #3 (id:40001) has been deleted
wuchengli@chromium.org changed reviewers: + mcasas@chromium.org - posciak@chromium.org
All done. mcasas@ owner review. Thanks. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:137: void Destroy(base::WaitableEvent* waiter); On 2016/04/04 09:00:31, pbos wrote: > Should this also be named async_waiter for consistency with below? Done. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:311: if (status_.load() != WEBRTC_VIDEO_CODEC_OK) { On 2016/04/04 09:00:31, pbos wrote: > Store this status_.load() instead of reading it twice. Done. Actually this is fine because |status_| is only written on the thread that Impl runs on (|gpu_task_runnder_|). |status_| can be read in RtcVideoEncoder::SetRates on another thread. I still use a local variable here to make the code clear. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:704: : video_codec_type_(type), On 2016/04/03 18:53:46, kcwu wrote: > no longer used. Done. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:725: WebRTCVideoCodecToVideoCodecProfile(impl_->video_codec_type(), codec_settings); On 2016/04/04 09:00:31, pbos wrote: > line length (run git cl format) Done. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:780: return encode_retval;; On 2016/04/04 09:00:31, pbos wrote: > remove ; Done. https://codereview.chromium.org/1845563006/diff/20001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:799: return impl_->GetStatus(); On 2016/04/04 09:00:31, pbos wrote: > I think this should still return WEBRTC_VIDEO_CODEC_OK because the other return > values are not from SetChannelParameters(). > > Though this value is ignored regardless, so I think you can just return OK. Done.
lgtm
mcasas@: Ping, do you have time to take a look?
The CQ bit was checked by wuchengli@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
mcasas@chromium.org changed reviewers: + emircan@chromium.org
passing the token to emircan@ who has worked recently in the RTCVideoEncoder class.
On 2016/04/11 23:36:17, mcasas wrote: > passing the token to emircan@ who has > worked recently in the RTCVideoEncoder > class. emircan@: Can you try to take a look asap? mcasas@: We still need you for owners' rubber btw. :)
https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:250: std::atomic<int32_t> status_; std::atomic looks banned for now, I think you have to use base/atomicops.h https://chromium-cpp.appspot.com/#library-blacklist
lgtm % nits below. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:95: // thread on which the instance was constructed. "Callbacks to RTCVideoEncoder are ... constructed" AFAIU, there aren't callbacks posted there any more. Safe to remove? https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:144: int32_t GetStatus(); Mark method as const. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:196: Can we add a comment about this being attached to gpu_task_runner_, but not the thread class is constructed on. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:642: if (status_.load() == WEBRTC_VIDEO_CODEC_OK) { Remove { } since it is a one-liner. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:694: int32_t retval = encoded_image_callback_->Encoded(image, &info, &header); const https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:806: int32_t retval = impl_->GetStatus(); const https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:811: A question: Should l.371 return WEBRTC_VIDEO_CODEC_OK as it is right now or some error?
Addressed all review comments. Tomorrow I'll test again and merge this. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:95: // thread on which the instance was constructed. On 2016/04/12 20:15:02, emircan wrote: > "Callbacks to RTCVideoEncoder are ... constructed" AFAIU, there aren't callbacks > posted there any more. Safe to remove? Yes. Removed. Also updated other comments here. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:144: int32_t GetStatus(); On 2016/04/12 20:15:02, emircan wrote: > Mark method as const. Done. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:196: On 2016/04/12 20:15:02, emircan wrote: > Can we add a comment about this being attached to gpu_task_runner_, but not the > thread class is constructed on. Done. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:250: std::atomic<int32_t> status_; On 2016/04/12 11:25:59, pbos wrote: > std::atomic looks banned for now, I think you have to use base/atomicops.h > > https://chromium-cpp.appspot.com/#library-blacklist Thanks. The comments in base/atomicops.h do not encourage people to use it. I used a lock instead. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:642: if (status_.load() == WEBRTC_VIDEO_CODEC_OK) { On 2016/04/12 20:15:03, emircan wrote: > Remove { } since it is a one-liner. Done. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:694: int32_t retval = encoded_image_callback_->Encoded(image, &info, &header); On 2016/04/12 20:15:02, emircan wrote: > const Done. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:806: int32_t retval = impl_->GetStatus(); On 2016/04/12 20:15:03, emircan wrote: > const Done. https://codereview.chromium.org/1845563006/diff/80001/content/renderer/media/... content/renderer/media/rtc_video_encoder.cc:811: On 2016/04/12 20:15:02, emircan wrote: > A question: Should l.371 return WEBRTC_VIDEO_CODEC_OK as it is right now or some > error? After l.371 (IsBitrateTooHigh) returns false, |status_| will be set to WEBRTC_VIDEO_CODEC_ERR_PARAMETER. The next RtcVideoEncoder::Encode will return the error. RTCVideoEncoder::Impl::RequestEncodingParametersChange is called frequently (maybe once per second). It's better not to change it from asynchronous to synchronous. If it should be changed, it should be another CL and needs some heavy testing.
The CQ bit was checked by wuchengli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@chromium.org, kcwu@chromium.org, emircan@chromium.org Link to the patchset: https://codereview.chromium.org/1845563006/#ps100001 (title: "address all review comments")
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mcasas@ need rubber stamp LGT-M from you. Thanks.
On 2016/04/14 05:32:18, wuchengli wrote: > mcasas@ need rubber stamp LGT-M from you. Thanks. RS LGTM (apologies, thought wuchengli@ was in OWNERS already)
The CQ bit was checked by wuchengli@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by pbos@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7aa249c90c143f50b554905c5dd515a8b25ef0ce Cr-Commit-Position: refs/heads/master@{#388463} |