Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686103002:
Do not encode frames in MultithreadedFakeH264Encoder after Release(). (Closed)
DescriptionDo not encode frames in MultithreadedFakeH264Encoder after Release().
Other minor changes:
- Define locks after stuff it is protecting
- Use explicit default dtors
- Replace unnecessary lock in DelayedEncoder with SequencedTaskChecker
BUG=webrtc:7130
Review-Url: https://codereview.webrtc.org/2686103002
Cr-Commit-Position: refs/heads/master@{#16554}
Committed: https://chromium.googlesource.com/external/webrtc/+/49ce67c9923aa7848d723bbbc147610ce1ed1a7b
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove atomicops.h include. #Patch Set 3 : Rebase. #
Total comments: 2
Patch Set 4 : sprang comments 1. #
Messages
Total messages: 30 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Do not encode frames in MultithreadedFakeH264Encoder after Release(). BUG=webrtc:7130 ========== to ========== Do not encode frames in MultithreadedFakeH264Encoder after Release(). Other minor changes: - Define locks after stuff it is protecting - Use explicit default dtors - Replace unnecessary lock in DelayedEncoder with SequencedTaskChecker BUG=webrtc:7130 ==========
Patchset #1 (id:80001) has been deleted
brandtr@webrtc.org changed reviewers: + sprang@webrtc.org
Please have a look :) Since I wasn't able to reproduce the race locally, I'm not sure this fix helps. But your theory in the bug seems reasonable to me.
https://codereview.webrtc.org/2686103002/diff/100001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2686103002/diff/100001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:351: queue1_.reset(); This should stop the posted tasks being executed: https://cs.chromium.org/chromium/src/third_party/webrtc/base/task_queue.h?l=154
Rebase.
lgtm https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:279: // The encoder could be created on a different thread than it being used on. nit: ...than it is being used on.
Thanks for quick feedback! https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder... webrtc/test/fake_encoder.cc:279: // The encoder could be created on a different thread than it being used on. On 2017/02/10 09:54:47, språng wrote: > nit: ...than it is being used on. Done. I was to eager to fit this on one line :(
The CQ bit was checked by brandtr@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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/2686103002/#ps160001 (title: "sprang comments 1.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/02/10 10:00:10, brandtr wrote: > Thanks for quick feedback! > > https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder.cc > File webrtc/test/fake_encoder.cc (right): > > https://codereview.webrtc.org/2686103002/diff/140001/webrtc/test/fake_encoder... > webrtc/test/fake_encoder.cc:279: // The encoder could be created on a different > thread than it being used on. > On 2017/02/10 09:54:47, språng wrote: > > nit: ...than it is being used on. > > Done. I was to eager to fit this on one line :( *too
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13283)
brandtr@webrtc.org changed reviewers: + stefan@webrtc.org
+stefan for OWNER approval
lgtm
The CQ bit was checked by brandtr@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": 160001, "attempt_start_ts": 1486801280401980,
"parent_rev": "6607d84b443a26dc1415c767229d73d8d56bdaeb", "commit_rev":
"49ce67c9923aa7848d723bbbc147610ce1ed1a7b"}
Message was sent while issue was closed.
Description was changed from ========== Do not encode frames in MultithreadedFakeH264Encoder after Release(). Other minor changes: - Define locks after stuff it is protecting - Use explicit default dtors - Replace unnecessary lock in DelayedEncoder with SequencedTaskChecker BUG=webrtc:7130 ========== to ========== Do not encode frames in MultithreadedFakeH264Encoder after Release(). Other minor changes: - Define locks after stuff it is protecting - Use explicit default dtors - Replace unnecessary lock in DelayedEncoder with SequencedTaskChecker BUG=webrtc:7130 Review-Url: https://codereview.webrtc.org/2686103002 Cr-Commit-Position: refs/heads/master@{#16554} Committed: https://chromium.googlesource.com/external/webrtc/+/49ce67c9923aa7848d723bbbc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/49ce67c9923aa7848d723bbbc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
