Release the critial section between recursive calls in FrameBuffer::NextFrame.

Release the critial section between recursive calls in FrameBuffer::NextFrame.

This is a revert of https://codereview.webrtc.org/2749563002 and a fix to not
keep the lock in between recursive calls.

BUG=webrtc:7331
Review-Url: https://codereview.webrtc.org/2750033002
Cr-Commit-Position: refs/heads/master@{#17256}
Committed: https://chromium.googlesource.com/external/webrtc/+/29f730ebd835240493ef8a86cccf90b436140af5

[philipel, 2017-03-15 12:07:48]

philipel@webrtc.org changed reviewers:
+ tommi@webrtc.org

[philipel, 2017-03-15 12:07:49]

philipel@webrtc.org changed reviewers:
+ tommi@webrtc.org

[philipel, 2017-03-15 12:07:49]

PTAL

[philipel, 2017-03-15 12:07:49]

PTAL

[tommi, 2017-03-15 13:33:48]

lgtm - great debugging!

I've got one comment that I hope won't come off too soap-boxy :)

https://codereview.webrtc.org/2750033002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2750033002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:184: rtc::CritScope lock(&crit_);
nit: the things that were outside the lock, can still be outside the lock.

For locks, we should generally ask ourselves:
* Do we really really really need a lock?
  - Sometimes we don't need a lock even if a variable is used on more than one
thread. In our code, I've seen several cases of variables being set, a thread is
started and while that thread runs, the variable does not change value.  Then
the thread is stopped and then the variable is modified again.  In cases like
that, we today incorporate locks, but a lock is actually not needed.  Ensuring
the code is correct however, can be achieved by thoroughly [D]CHECKing thread
correctness, which also has the benefit of documenting the execution context of
the methods for anyone that reads the code.

* Is the scope as small as it can be?
 - I think that in this case it can be smaller.  |stats_callback_| is const, so
its value will never change.  It also represents an external implementation.

* Are we calling out to other functions while calling the lock?
 - Calling out to OnCompleteFrame, while holding the lock, could leave us
vulnerable to things like lock inversion or potentially slowing things down for
reasons that don't matter for InsertFrame.  It could also mask an issue with the
external implementation by introducing synchronization for code that doesn't
have it but should.

[philipel, 2017-03-15 14:10:28]

https://codereview.webrtc.org/2750033002/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2750033002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:184: rtc::CritScope lock(&crit_);
On 2017/03/15 13:33:48, tommi (webrtc) wrote:
> nit: the things that were outside the lock, can still be outside the lock.
> 
> For locks, we should generally ask ourselves:
> * Do we really really really need a lock?
>   - Sometimes we don't need a lock even if a variable is used on more than one
> thread. In our code, I've seen several cases of variables being set, a thread
is
> started and while that thread runs, the variable does not change value.  Then
> the thread is stopped and then the variable is modified again.  In cases like
> that, we today incorporate locks, but a lock is actually not needed.  Ensuring
> the code is correct however, can be achieved by thoroughly [D]CHECKing thread
> correctness, which also has the benefit of documenting the execution context
of
> the methods for anyone that reads the code.
> 
> * Is the scope as small as it can be?
>  - I think that in this case it can be smaller.  |stats_callback_| is const,
so
> its value will never change.  It also represents an external implementation.
> 
> * Are we calling out to other functions while calling the lock?
>  - Calling out to OnCompleteFrame, while holding the lock, could leave us
> vulnerable to things like lock inversion or potentially slowing things down
for
> reasons that don't matter for InsertFrame.  It could also mask an issue with
the
> external implementation by introducing synchronization for code that doesn't
> have it but should.

Fixed!

Comments like these are great, it is one of the main sources of learning I have
ever since I started working, so they are much appreciated. :)

[philipel, 2017-03-15 14:10:31]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-03-15 14:10:31]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2750033002/#ps20001 (title:
"Shrink critical section.")

[commit-bot: I haz the power, 2017-03-15 14:10:43]

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, 2017-03-15 15:10:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489587031549800,
"parent_rev": "5b3e49a29e442bb6b5cb4a8a1d9496566887e212", "commit_rev":
"29f730ebd835240493ef8a86cccf90b436140af5"}

[commit-bot: I haz the power, 2017-03-15 15:10:10]

Description was changed from

==========
Release the critial section between recursive calls in FrameBuffer::NextFrame.

This is a revert of https://codereview.webrtc.org/2749563002 and a fix to not
keep the lock in between recursive calls.

BUG=webrtc:7331
==========

to

==========
Release the critial section between recursive calls in FrameBuffer::NextFrame.

This is a revert of https://codereview.webrtc.org/2749563002 and a fix to not
keep the lock in between recursive calls.

BUG=webrtc:7331

Review-Url: https://codereview.webrtc.org/2750033002
Cr-Commit-Position: refs/heads/master@{#17256}
Committed:
https://chromium.googlesource.com/external/webrtc/+/29f730ebd835240493ef8a86c...
==========

[commit-bot: I haz the power, 2017-03-15 15:10:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/29f730ebd835240493ef8a86c...