Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2991533002:
Make FlexfecReceiveStreamImpl::started_ into std::atomic<bool> (Closed)
DescriptionMake FlexfecReceiveStreamImpl::started_ into std::atomic<bool>
FlexfecReceiveStreamImpl::crit_ was only protecting one boolean, so it's probably better to just make sure that boolean is atomic.
BUG=None
Review-Url: https://codereview.webrtc.org/2991533002
Cr-Commit-Position: refs/heads/master@{#19217}
Committed: https://chromium.googlesource.com/external/webrtc/+/3d4c28778e5f262ccc82f5ba4ebb671ca8496c7b
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use explicit store/load. #
Total comments: 3
Messages
Total messages: 18 (8 generated)
danilchap@webrtc.org changed reviewers: + danilchap@webrtc.org
neat! lgtm https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... File webrtc/call/flexfec_receive_stream_impl.cc (right): https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream_impl.cc:171: if (!started_) prefer .load() over operator bool to be explicit it is an atomic https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream_impl.cc:191: started_ = true; same here: prefer store
https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... File webrtc/call/flexfec_receive_stream_impl.cc (right): https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream_impl.cc:171: if (!started_) On 2017/07/24 13:40:23, danilchap wrote: > prefer .load() over operator bool to be explicit it is an atomic Done. https://codereview.webrtc.org/2991533002/diff/1/webrtc/call/flexfec_receive_s... webrtc/call/flexfec_receive_stream_impl.cc:191: started_ = true; On 2017/07/24 13:40:23, danilchap wrote: > same here: prefer store Done.
Description was changed from ========== Make FlexfecReceiveStreamImpl::started_ into std::atomic<bool> FlexfecReceiveStreamImpl::crit_ was only protecting one boolean, so it's probably better to just make sure that boolean is atomic. BUG=None ========== to ========== Make FlexfecReceiveStreamImpl::started_ into std::atomic<bool> FlexfecReceiveStreamImpl::crit_ was only protecting one boolean, so it's probably better to just make sure that boolean is atomic. BUG=None ==========
eladalon@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan, could you please provide the owner-lgtm?
brandtr@webrtc.org changed reviewers: + brandtr@webrtc.org
https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... File webrtc/call/flexfec_receive_stream_impl.h (right): https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_impl.h:57: std::atomic<bool> started_; The start/stop functionality doesn't really serve a purpose for the FlexfecReceiveStream. I have discussed this with nisse, and he is planning on removing it completely.
https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... File webrtc/call/flexfec_receive_stream_impl.h (right): https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_impl.h:57: std::atomic<bool> started_; On 2017/08/01 07:08:02, brandtr wrote: > The start/stop functionality doesn't really serve a purpose for the > FlexfecReceiveStream. I have discussed this with nisse, and he is planning on > removing it completely. Okay, thanks. In the meantime, do you think should I scrap the CL, or that I should land it?
lgtm https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... File webrtc/call/flexfec_receive_stream_impl.h (right): https://codereview.webrtc.org/2991533002/diff/20001/webrtc/call/flexfec_recei... webrtc/call/flexfec_receive_stream_impl.h:57: std::atomic<bool> started_; On 2017/08/01 08:27:46, eladalon wrote: > On 2017/08/01 07:08:02, brandtr wrote: > > The start/stop functionality doesn't really serve a purpose for the > > FlexfecReceiveStream. I have discussed this with nisse, and he is planning on > > removing it completely. > > Okay, thanks. > In the meantime, do you think should I scrap the CL, or that I should land it? Might as well land it, since it's a nice improvement and you have done the work already :)
Cool; pinging +stefan@ for the owner-lgtm.
lgtm
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2991533002/#ps20001 (title: "Use explicit store/load.")
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": 20001, "attempt_start_ts": 1501681435947590,
"parent_rev": "a1eef11516add6cb1fc92dd576fa75d3a1ddef7d", "commit_rev":
"3d4c28778e5f262ccc82f5ba4ebb671ca8496c7b"}
Message was sent while issue was closed.
Description was changed from ========== Make FlexfecReceiveStreamImpl::started_ into std::atomic<bool> FlexfecReceiveStreamImpl::crit_ was only protecting one boolean, so it's probably better to just make sure that boolean is atomic. BUG=None ========== to ========== Make FlexfecReceiveStreamImpl::started_ into std::atomic<bool> FlexfecReceiveStreamImpl::crit_ was only protecting one boolean, so it's probably better to just make sure that boolean is atomic. BUG=None Review-Url: https://codereview.webrtc.org/2991533002 Cr-Commit-Position: refs/heads/master@{#19217} Committed: https://chromium.googlesource.com/external/webrtc/+/3d4c28778e5f262ccc82f5ba4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/3d4c28778e5f262ccc82f5ba4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
