Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402663003:
Avoid race in VideoReceiveStream shutdown (Closed)
|
Created:
4 years, 2 months ago by sprang_webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, perkj_webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid race in VideoReceiveStream shutdown
The decoder implementation may have its own thread for asynchrouns
callbacks. We need to stop it (by releasing the decoder) when stopping
the decoder thread, othweise it may call incoming_video_stream_ after
it has been destroyed.
BUG=webrtc:6501
Committed: https://crrev.com/0d348d69e6c6cf58e07e12b43a06566de580bdfc
Cr-Commit-Position: refs/heads/master@{#14577}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 16 (8 generated)
sprang@webrtc.org changed reviewers: + stefan@webrtc.org
mflodman@webrtc.org changed reviewers: + mflodman@webrtc.org
LGTM to get this in. Is there a way we can test this?
On 2016/10/07 14:57:31, mflodman wrote: > LGTM to get this in. > > Is there a way we can test this? It's difficult with these kind of race conditions, where the dependent parts aren't exposed in a clean way. I'm thinking we could add an external codec wrapper with an internal thread, which then just delegates to a wrapped encoder. We could do a stress test with a fake encoder or run some end to end or perf tests with the wrapped one. wdyt? Will make a follow-up cl for that though.
Description was changed from ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 ========== to ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 ==========
sprang@webrtc.org changed reviewers: - stefan@webrtc.org
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 ========== to ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 ========== to ========== Avoid race in VideoReceiveStream shutdown The decoder implementation may have its own thread for asynchrouns callbacks. We need to stop it (by releasing the decoder) when stopping the decoder thread, othweise it may call incoming_video_stream_ after it has been destroyed. BUG=webrtc:6501 Committed: https://crrev.com/0d348d69e6c6cf58e07e12b43a06566de580bdfc Cr-Commit-Position: refs/heads/master@{#14577} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0d348d69e6c6cf58e07e12b43a06566de580bdfc Cr-Commit-Position: refs/heads/master@{#14577}
Message was sent while issue was closed.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
Message was sent while issue was closed.
drive by question https://codereview.webrtc.org/2402663003/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2402663003/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:220: for (const Decoder& decoder : config_.decoders) { should this loop be moved into Start()? As is, we don't seem to have symmetry and two cycles of Start/Stop, would not do exactly the same thing.
Message was sent while issue was closed.
https://codereview.webrtc.org/2402663003/diff/1/webrtc/video/video_receive_st... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2402663003/diff/1/webrtc/video/video_receive_st... webrtc/video/video_receive_stream.cc:220: for (const Decoder& decoder : config_.decoders) { On 2016/10/07 19:45:40, tommi (webrtc) wrote: > should this loop be moved into Start()? As is, we don't seem to have symmetry > and two cycles of Start/Stop, would not do exactly the same thing. Yes, agree that should be done. Good catch Tommi! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
