Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562983002:
Remove sequenced task checker from FlexfecSender. (Closed)
|
Created:
4 years ago by brandtr Modified:
4 years ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove sequenced task checker from FlexfecSender.
The packetization parts of this class are accessed from the
encoder thread, which might change under different occasions.
The use of a sequenced task checker here is thus incorrect, since
that requires the access to always be on the same thread, whenever
a task queue is not used.
The access to the instantiated object of this class, at least when
it comes to the RTP packetization parts, is however synchronized
using the lock in PayloadRouter::OnEncodedImage. We can therefore
safely remove the sequenced task checker.
BUG=webrtc:5654
Committed: https://crrev.com/fe793eb2d1b8c42c557ddef5c1d760bbec23de2d
Cr-Commit-Position: refs/heads/master@{#15549}
Patch Set 1 : Fix problem in FlexfecSender by removing DCHECKs. #Patch Set 2 : Revert PS1. Add VSS test with fake multithreaded encoder. #Patch Set 3 : Add PS1 back. #Patch Set 4 : Rebase. #Patch Set 5 : Move test to https://codereview.webrtc.org/2573453002/ #
Dependent Patchsets: Messages
Total messages: 30 (15 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Remove sequenced task checker from FlexfecSender. This class can be accessed on different threads in certain occasions, but we can rely on the lock in PayloadRouter::OnEncodedImage for the synchronization. BUG=webrtc:5654 ========== to ========== Remove sequenced task checker from FlexfecSender. The packetization parts of this class are accessed from the encoder thread, which might change under different occasions. The use of a sequenced task checker here is thus incorrect, since that requires the access to always be on the same thread, whenever a task queue is not used. The access to the instantiated object of this class, at least when it comes to the RTP packetization parts, is however synchronized using the lock in PayloadRouter::OnEncodedImage. We can therefore safely remove the sequenced task checker. BUG=webrtc:5654 ==========
brandtr@webrtc.org changed reviewers: + stefan@webrtc.org
holmer, do you agree with the reasoning in the commit message?
On 2016/12/09 13:10:15, brandtr wrote: > holmer, do you agree with the reasoning in the commit message? Here is another approach, where I add locks instead: https://codereview.webrtc.org/2565693002/. That should not be needed though, as long as the lock in the PayloadRouter is still there...
lgtm lgtm, but perhaps we should build a test in video send stream tests which uses a multi threaded fake encoder and would have triggered this? I think sprang has built such a fake encoder
Revert PS1. Add VSS test with fake multithreaded encoder.
Patchset #2 (id:40001) has been deleted
Revert PS1. Add VSS test with fake multithreaded encoder.
Add PS1 back.
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Revert PS1. Add VSS test with fake multithreaded encoder.
Add PS1 back.
On 2016/12/09 13:39:23, stefan-webrtc (holmer) wrote: > lgtm > > lgtm, but perhaps we should build a test in video send stream tests which uses a > multi threaded fake encoder and would have triggered this? I think sprang has > built such a fake encoder He built an asynchronous decoder wrapper, which is unfortunately not useful here. I made a rudimentary version of such a fake encoder in this CL instead, together with an accompanying test. Please have a second look.
Rebase.
This one :)
Move test to https://codereview.webrtc.org/2573453002/
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: This issue passed the CQ dry run.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2562983002/#ps160001 (title: "Move test to https://codereview.webrtc.org/2573453002/")
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": 1481555535252500,
"parent_rev": "509495ce542d9b6af6a5c7f9d45906c0e07f70e9", "commit_rev":
"c4878dddcc230177dbd75b615fa164f5a95b1292"}
Message was sent while issue was closed.
Description was changed from ========== Remove sequenced task checker from FlexfecSender. The packetization parts of this class are accessed from the encoder thread, which might change under different occasions. The use of a sequenced task checker here is thus incorrect, since that requires the access to always be on the same thread, whenever a task queue is not used. The access to the instantiated object of this class, at least when it comes to the RTP packetization parts, is however synchronized using the lock in PayloadRouter::OnEncodedImage. We can therefore safely remove the sequenced task checker. BUG=webrtc:5654 ========== to ========== Remove sequenced task checker from FlexfecSender. The packetization parts of this class are accessed from the encoder thread, which might change under different occasions. The use of a sequenced task checker here is thus incorrect, since that requires the access to always be on the same thread, whenever a task queue is not used. The access to the instantiated object of this class, at least when it comes to the RTP packetization parts, is however synchronized using the lock in PayloadRouter::OnEncodedImage. We can therefore safely remove the sequenced task checker. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2562983002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Remove sequenced task checker from FlexfecSender. The packetization parts of this class are accessed from the encoder thread, which might change under different occasions. The use of a sequenced task checker here is thus incorrect, since that requires the access to always be on the same thread, whenever a task queue is not used. The access to the instantiated object of this class, at least when it comes to the RTP packetization parts, is however synchronized using the lock in PayloadRouter::OnEncodedImage. We can therefore safely remove the sequenced task checker. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2562983002 ========== to ========== Remove sequenced task checker from FlexfecSender. The packetization parts of this class are accessed from the encoder thread, which might change under different occasions. The use of a sequenced task checker here is thus incorrect, since that requires the access to always be on the same thread, whenever a task queue is not used. The access to the instantiated object of this class, at least when it comes to the RTP packetization parts, is however synchronized using the lock in PayloadRouter::OnEncodedImage. We can therefore safely remove the sequenced task checker. BUG=webrtc:5654 Committed: https://crrev.com/fe793eb2d1b8c42c557ddef5c1d760bbec23de2d Cr-Commit-Position: refs/heads/master@{#15549} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fe793eb2d1b8c42c557ddef5c1d760bbec23de2d Cr-Commit-Position: refs/heads/master@{#15549} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
