Avoid race in VideoReceiveStream shutdown

Avoid race in VideoReceiveStream shutdown

The decode thread should be stopped before triggering shutdown of the
video receiver, so that the decoder doesn't try to insert a new frame
while the jitter buffer is being shut down.

BUG=webrtc:6102
Committed: https://crrev.com/22691e0d3273649a7bc5e6822e4c82f3b02ad5ae
Cr-Commit-Position: refs/heads/master@{#13467}

[sprang_webrtc, 2016-07-13 12:57:05]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-07-13 13:06:35]

https://codereview.webrtc.org/2146883002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2146883002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:270: decode_thread_.Stop();
That's harder than this. TriggerDecoderShutdown() is what's causing
decode_thread_ to stop faster. With this change TriggerDecoderShutdown doesn't
make sense since it won't make any thread stop faster.

[sprang_webrtc, 2016-07-13 16:17:06]

I've made some refactoring to the jitter buffer, so frame buffers are reset
immediately when recycled. Frames in incomplete or decodable state are recycled
on Start() instead of on Stop(), otherwise just destroyed as per usual.
Some other minor cleanups as well.

ptal

https://codereview.webrtc.org/2146883002/diff/20001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2146883002/diff/20001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:270: decode_thread_.Stop();
On 2016/07/13 13:06:35, pbos-webrtc wrote:
> That's harder than this. TriggerDecoderShutdown() is what's causing
> decode_thread_ to stop faster. With this change TriggerDecoderShutdown doesn't
> make sense since it won't make any thread stop faster.

You're right. I've added a comment to that effect here.

[pbos-webrtc, 2016-07-13 16:43:06]

lgtm

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:335: {
Remove the {}s, the time for calling event->set() should be negligible.

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:270:
video_receiver_.TriggerDecoderShutdown();  // Releases waiting decoder thread.
Put something to the effect of this making decode_thread_ stop directly (instead
of waiting for a timeout), and that it should be called before joining the
thread.

[sprang_webrtc, 2016-07-13 16:51:32]

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/jitter_buffer.cc (right):

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/jitter_buffer.cc:335: {
On 2016/07/13 16:43:05, pbos-webrtc wrote:
> Remove the {}s, the time for calling event->set() should be negligible.

Done.

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2146883002/diff/40001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.cc:270:
video_receiver_.TriggerDecoderShutdown();  // Releases waiting decoder thread.
On 2016/07/13 16:43:05, pbos-webrtc wrote:
> Put something to the effect of this making decode_thread_ stop directly
(instead
> of waiting for a timeout), and that it should be called before joining the
> thread.

Done.

[sprang_webrtc, 2016-07-13 16:51:50]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-07-13 16:51:50]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2146883002/#ps60001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-07-13 16:51:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-13 17:57:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 17:57:29]

Description was changed from

==========
Avoid race in VideoReceiveStream shutdown

The decode thread should be stopped before triggering shutdown of the
video receiver, so that the decoder doesn't try to insert a new frame
while the jitter buffer is being shut down.

BUG=webrtc:6102
==========

to

==========
Avoid race in VideoReceiveStream shutdown

The decode thread should be stopped before triggering shutdown of the
video receiver, so that the decoder doesn't try to insert a new frame
while the jitter buffer is being shut down.

BUG=webrtc:6102

Committed: https://crrev.com/22691e0d3273649a7bc5e6822e4c82f3b02ad5ae
Cr-Commit-Position: refs/heads/master@{#13467}
==========

[commit-bot: I haz the power, 2016-07-13 17:57:31]

Patchset 4 (id:??) landed as
https://crrev.com/22691e0d3273649a7bc5e6822e4c82f3b02ad5ae
Cr-Commit-Position: refs/heads/master@{#13467}