FrameBuffer2 now has Start/Stop methods.

FrameBuffer2 now has Start/Stop methods.

The Stop method is used to signal any thread that is waiting in the
NextFrame function and will cause it to return immediately.

BUG=webrtc:5514
R=pbos@webrtc.org

Committed: https://crrev.com/504c47d750b4516ca81c93ffb637d9a4c8fb6ea9
Cr-Commit-Position: refs/heads/master@{#13349}

[philipel, 2016-06-29 12:42:36]

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

[pbos-webrtc, 2016-06-29 15:51:26]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
Can you replace these explicit enters with rtc::CritScope? One before waiting
and one after, just add more scopes:

{
  CritScope ...;
}

wait

{ CS ...;}

[philipel, 2016-06-30 09:36:56]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
On 2016/06/29 15:51:26, pbos-webrtc wrote:
> Can you replace these explicit enters with rtc::CritScope? One before waiting
> and one after, just add more scopes:
> 
> {
>   CritScope ...;
> }
> 
> wait
> 
> { CS ...;}

Sorry, scopes wont work throughout this functions, so I'd rather keep to Enter()
and Leave() to keep things consistent.

[pbos-webrtc, 2016-06-30 09:44:07]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
On 2016/06/30 09:36:56, philipel wrote:
> On 2016/06/29 15:51:26, pbos-webrtc wrote:
> > Can you replace these explicit enters with rtc::CritScope? One before
waiting
> > and one after, just add more scopes:
> > 
> > {
> >   CritScope ...;
> > }
> > 
> > wait
> > 
> > { CS ...;}
> 
> Sorry, scopes wont work throughout this functions, so I'd rather keep to
Enter()
> and Leave() to keep things consistent.

What do you mean? Have one scoped between line 56 here and line 83 in this
patchset. Then one inside the scope at line 90.

All other functions use CritScope, this would make it more consistent?

[philipel, 2016-06-30 11:37:35]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
On 2016/06/30 09:44:07, pbos-webrtc wrote:
> On 2016/06/30 09:36:56, philipel wrote:
> > On 2016/06/29 15:51:26, pbos-webrtc wrote:
> > > Can you replace these explicit enters with rtc::CritScope? One before
> waiting
> > > and one after, just add more scopes:
> > > 
> > > {
> > >   CritScope ...;
> > > }
> > > 
> > > wait
> > > 
> > > { CS ...;}
> > 
> > Sorry, scopes wont work throughout this functions, so I'd rather keep to
> Enter()
> > and Leave() to keep things consistent.
> 
> What do you mean? Have one scoped between line 56 here and line 83 in this
> patchset. Then one inside the scope at line 90.
> 
> All other functions use CritScope, this would make it more consistent?

The problem is the |next_frame| iterator which we need after the timeout occurs.
If we use a CritScope then |next_frame| will go out of scope as we leave the
critical section, and if we declare |next_frame| outside of the CritScope then
we can't assign it to frames_.end() since we must hold the lock in order to use
|frames_|, so either it would have to be an uninitialized iterator, or we have
to lock extra time when we enter the function to initialize |next_frame|.

[pbos-webrtc, 2016-06-30 12:04:54]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
On 2016/06/30 11:37:35, philipel wrote:
> On 2016/06/30 09:44:07, pbos-webrtc wrote:
> > On 2016/06/30 09:36:56, philipel wrote:
> > > On 2016/06/29 15:51:26, pbos-webrtc wrote:
> > > > Can you replace these explicit enters with rtc::CritScope? One before
> > waiting
> > > > and one after, just add more scopes:
> > > > 
> > > > {
> > > >   CritScope ...;
> > > > }
> > > > 
> > > > wait
> > > > 
> > > > { CS ...;}
> > > 
> > > Sorry, scopes wont work throughout this functions, so I'd rather keep to
> > Enter()
> > > and Leave() to keep things consistent.
> > 
> > What do you mean? Have one scoped between line 56 here and line 83 in this
> > patchset. Then one inside the scope at line 90.
> > 
> > All other functions use CritScope, this would make it more consistent?
> 
> The problem is the |next_frame| iterator which we need after the timeout
occurs.
> If we use a CritScope then |next_frame| will go out of scope as we leave the
> critical section, and if we declare |next_frame| outside of the CritScope then
> we can't assign it to frames_.end() since we must hold the lock in order to
use
> |frames_|, so either it would have to be an uninitialized iterator, or we have
> to lock extra time when we enter the function to initialize |next_frame|.

Can frames_' size be modified while the lock is dropped? If so I think checking
frames_.end() after reacquiring the lock is not safe?

If you call InsertFrame while this lock is dropped that invalidates previous
iterators, right?

[philipel, 2016-06-30 12:20:04]

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

https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
On 2016/06/30 12:04:53, pbos-webrtc wrote:
> On 2016/06/30 11:37:35, philipel wrote:
> > On 2016/06/30 09:44:07, pbos-webrtc wrote:
> > > On 2016/06/30 09:36:56, philipel wrote:
> > > > On 2016/06/29 15:51:26, pbos-webrtc wrote:
> > > > > Can you replace these explicit enters with rtc::CritScope? One before
> > > waiting
> > > > > and one after, just add more scopes:
> > > > > 
> > > > > {
> > > > >   CritScope ...;
> > > > > }
> > > > > 
> > > > > wait
> > > > > 
> > > > > { CS ...;}
> > > > 
> > > > Sorry, scopes wont work throughout this functions, so I'd rather keep to
> > > Enter()
> > > > and Leave() to keep things consistent.
> > > 
> > > What do you mean? Have one scoped between line 56 here and line 83 in this
> > > patchset. Then one inside the scope at line 90.
> > > 
> > > All other functions use CritScope, this would make it more consistent?
> > 
> > The problem is the |next_frame| iterator which we need after the timeout
> occurs.
> > If we use a CritScope then |next_frame| will go out of scope as we leave the
> > critical section, and if we declare |next_frame| outside of the CritScope
then
> > we can't assign it to frames_.end() since we must hold the lock in order to
> use
> > |frames_|, so either it would have to be an uninitialized iterator, or we
have
> > to lock extra time when we enter the function to initialize |next_frame|.
> 
> Can frames_' size be modified while the lock is dropped? If so I think
checking
> frames_.end() after reacquiring the lock is not safe?
> 
> If you call InsertFrame while this lock is dropped that invalidates previous
> iterators, right?

You can't call frames_.size() while not holding the lock and iterators are not
invalidated when a new frame is inserted/removed.

[pbos-webrtc, 2016-06-30 12:23:27]

On 2016/06/30 12:20:04, philipel wrote:
>
https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
> File webrtc/modules/video_coding/frame_buffer2.cc (right):
> 
>
https://codereview.webrtc.org/2105323002/diff/1/webrtc/modules/video_coding/f...
> webrtc/modules/video_coding/frame_buffer2.cc:56: crit_.Enter();
> On 2016/06/30 12:04:53, pbos-webrtc wrote:
> > On 2016/06/30 11:37:35, philipel wrote:
> > > On 2016/06/30 09:44:07, pbos-webrtc wrote:
> > > > On 2016/06/30 09:36:56, philipel wrote:
> > > > > On 2016/06/29 15:51:26, pbos-webrtc wrote:
> > > > > > Can you replace these explicit enters with rtc::CritScope? One
before
> > > > waiting
> > > > > > and one after, just add more scopes:
> > > > > > 
> > > > > > {
> > > > > >   CritScope ...;
> > > > > > }
> > > > > > 
> > > > > > wait
> > > > > > 
> > > > > > { CS ...;}
> > > > > 
> > > > > Sorry, scopes wont work throughout this functions, so I'd rather keep
to
> > > > Enter()
> > > > > and Leave() to keep things consistent.
> > > > 
> > > > What do you mean? Have one scoped between line 56 here and line 83 in
this
> > > > patchset. Then one inside the scope at line 90.
> > > > 
> > > > All other functions use CritScope, this would make it more consistent?
> > > 
> > > The problem is the |next_frame| iterator which we need after the timeout
> > occurs.
> > > If we use a CritScope then |next_frame| will go out of scope as we leave
the
> > > critical section, and if we declare |next_frame| outside of the CritScope
> then
> > > we can't assign it to frames_.end() since we must hold the lock in order
to
> > use
> > > |frames_|, so either it would have to be an uninitialized iterator, or we
> have
> > > to lock extra time when we enter the function to initialize |next_frame|.
> > 
> > Can frames_' size be modified while the lock is dropped? If so I think
> checking
> > frames_.end() after reacquiring the lock is not safe?
> > 
> > If you call InsertFrame while this lock is dropped that invalidates previous
> > iterators, right?
> 
> You can't call frames_.size() while not holding the lock and iterators are not
> invalidated when a new frame is inserted/removed.

Gotcha, didn't know that existing iterators remained valid. Prefer having an
uninitialized next_frame iterator that is initialized under the lock then.

[philipel, 2016-06-30 12:51:03]

Hmmm... don't like this style, I think it is cleaner to use Enter/Leave in this
case instead of working around CritScope...

[pbos-webrtc, 2016-06-30 12:56:30]

I think it's better to clearly see when/where a lock is taken/not taken and not
needing to remember .Leave() in before every return.

lgtm as is now w/ nit, but you can bring in someone else if you want a second
opinion on CritScope/not CritScope.

https://codereview.webrtc.org/2105323002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2105323002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:60: {
Move next_frame before this line (inside the while loop)

[stefan-webrtc, 2016-06-30 13:19:00]

stefan@webrtc.org changed reviewers:
+ stefan@webrtc.org

[stefan-webrtc, 2016-06-30 13:19:01]

I agree that scoped critsects are preferable whenever possible.

[philipel, 2016-06-30 13:19:24]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-06-30 13:19:24]

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/2105323002/#ps40001 (title:
"Nit fix")

[commit-bot: I haz the power, 2016-06-30 13:19:27]

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

[philipel, 2016-06-30 13:21:08]

https://codereview.webrtc.org/2105323002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2105323002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:60: {
On 2016/06/30 12:56:30, pbos-webrtc wrote:
> Move next_frame before this line (inside the while loop)

Done.

[commit-bot: I haz the power, 2016-06-30 13:28:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 13:28:37]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14676)

[commit-bot: I haz the power, 2016-06-30 15:33:23]

Description was changed from

==========
FrameBuffer2 now has Start/Stop methods.

The Stop method is used to signal any thread that is waiting in the
NextFrame function and will cause it to return immediately.

BUG=webrtc:5514
==========

to

==========
FrameBuffer2 now has Start/Stop methods.

The Stop method is used to signal any thread that is waiting in the
NextFrame function and will cause it to return immediately.

BUG=webrtc:5514
R=pbos@webrtc.org

Committed: https://crrev.com/504c47d750b4516ca81c93ffb637d9a4c8fb6ea9
Cr-Commit-Position: refs/heads/master@{#13349}
==========

[philipel, 2016-06-30 15:33:23]

Description was changed from

==========
FrameBuffer2 now has Start/Stop methods.

The Stop method is used to signal any thread that is waiting in the
NextFrame function and will cause it to return immediately.

BUG=webrtc:5514
==========

to

==========
FrameBuffer2 now has Start/Stop methods.

The Stop method is used to signal any thread that is waiting in the
NextFrame function and will cause it to return immediately.

BUG=webrtc:5514
R=pbos@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/504c47d750b4516ca81c93ffb...
==========

[commit-bot: I haz the power, 2016-06-30 15:33:24]

Patchset 3 (id:??) landed as
https://crrev.com/504c47d750b4516ca81c93ffb637d9a4c8fb6ea9
Cr-Commit-Position: refs/heads/master@{#13349}

[philipel, 2016-06-30 15:33:28]

Committed patchset #3 (id:40001) manually as
504c47d750b4516ca81c93ffb637d9a4c8fb6ea9 (presubmit successful).