Move keyframe requests outside encoder mutex.

Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org

Committed: https://crrev.com/233bfd2da4acef06ad9de1cbb1d4ba4a0bf7380e
Cr-Commit-Position: refs/heads/master@{#11294}

[pbos-webrtc, 2016-01-18 14:02:26]

test fixes

[pbos-webrtc, 2016-01-18 14:06:03]

PTAL

[pbos-webrtc, 2016-01-18 14:06:15]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-18 14:06:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1600553003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1600553003/20001

[pbos-webrtc, 2016-01-18 14:47:53]

lock send_crit_ for internalsource encoders

[stefan-webrtc, 2016-01-18 15:42:36]

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:131: encoder_has_internal_source_ =
_encoder && _encoder->InternalSource();
Can _encoder ever be null here?

It's also not entirely clear to me why we can't query _encoder instead? Probably
deserves a comment.

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:334: // send_crit_.
I don't understand how params_crit_ have been dropped?

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:338: return -1;
Do you think this actually can happen?

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:227: FrameType frame_type =
stream == -2 ? kVideoFrameKey : kVideoFrameDelta;
-2??? :)
Please add another parameter instead.

[pbos-webrtc, 2016-01-18 16:38:52]

stefan@ feedback

[pbos-webrtc, 2016-01-18 16:38:56]

PTAL

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:131: encoder_has_internal_source_ =
_encoder && _encoder->InternalSource();
On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> Can _encoder ever be null here?
> 
> It's also not entirely clear to me why we can't query _encoder instead?
Probably
> deserves a comment.

Commented and added a DCHECK for _encoder instead.

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:334: // send_crit_.
On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> I don't understand how params_crit_ have been dropped?

Outside of the above {} scope.

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:338: return -1;
On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> Do you think this actually can happen?

Yes, if SetSendCodec is called between the check above and here with another 
number of simulcast streams.

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:227: FrameType frame_type =
stream == -2 ? kVideoFrameKey : kVideoFrameDelta;
On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> -2??? :)
> Please add another parameter instead.

Done!

[stefan-webrtc, 2016-01-18 18:40:09]

lgtm with comments addressed.

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:131: encoder_has_internal_source_ =
_encoder && _encoder->InternalSource();
On 2016/01/18 16:38:55, pbos-webrtc wrote:
> On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> > Can _encoder ever be null here?
> > 
> > It's also not entirely clear to me why we can't query _encoder instead?
> Probably
> > deserves a comment.
> 
> Commented and added a DCHECK for _encoder instead.

Do you think it would make sense to rename send_crit_ to encoder_crit_?

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:338: return -1;
On 2016/01/18 16:38:55, pbos-webrtc wrote:
> On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> > Do you think this actually can happen?
> 
> Yes, if SetSendCodec is called between the check above and here with another 
> number of simulcast streams.

Ah

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender_unittest.cc (right):

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:228: void
ExpectInitialKeyframes() {
...KeyFrames()

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:245:
assert(intra_request_stream < kNumberOfStreams);
Please change to ASSERT_...()

[pbos-webrtc, 2016-01-18 19:13:03]

stefan@ feedback round 2

[pbos-webrtc, 2016-01-18 19:13:35]

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

https://codereview.webrtc.org/1600553003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:131: encoder_has_internal_source_ =
_encoder && _encoder->InternalSource();
On 2016/01/18 18:40:08, stefan-webrtc (holmer) wrote:
> On 2016/01/18 16:38:55, pbos-webrtc wrote:
> > On 2016/01/18 15:42:36, stefan-webrtc (holmer) wrote:
> > > Can _encoder ever be null here?
> > > 
> > > It's also not entirely clear to me why we can't query _encoder instead?
> > Probably
> > > deserves a comment.
> > 
> > Commented and added a DCHECK for _encoder instead.
> 
> Do you think it would make sense to rename send_crit_ to encoder_crit_?

Done. Note that some methods access it under thread_checker_, so I can't add
GUARDED_BY to the encoder.

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender_unittest.cc (right):

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:228: void
ExpectInitialKeyframes() {
On 2016/01/18 18:40:08, stefan-webrtc (holmer) wrote:
> ...KeyFrames()

Done.

https://codereview.webrtc.org/1600553003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:245:
assert(intra_request_stream < kNumberOfStreams);
On 2016/01/18 18:40:08, stefan-webrtc (holmer) wrote:
> Please change to ASSERT_...()

Done.

[pbos-webrtc, 2016-01-18 19:15:13]

Description was changed from

==========
Move keyframe requests from send_crit_.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org
==========

to

==========
Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org
==========

[pbos-webrtc, 2016-01-18 19:23:56]

Description was changed from

==========
Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org
==========

to

==========
Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/233bfd2da4acef06ad9de1cbb...
==========

[commit-bot: I haz the power, 2016-01-18 19:23:56]

Description was changed from

==========
Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/233bfd2da4acef06ad9de1cbb...
==========

to

==========
Move keyframe requests outside encoder mutex.

Enables faster keyframe requests since they are no longer blocked by
calls to the encoder.

BUG=webrtc:5410
R=stefan@webrtc.org

Committed: https://crrev.com/233bfd2da4acef06ad9de1cbb1d4ba4a0bf7380e
Cr-Commit-Position: refs/heads/master@{#11294}
==========

[commit-bot: I haz the power, 2016-01-18 19:23:57]

Patchset 5 (id:??) landed as
https://crrev.com/233bfd2da4acef06ad9de1cbb1d4ba4a0bf7380e
Cr-Commit-Position: refs/heads/master@{#11294}

[pbos-webrtc, 2016-01-18 19:23:57]

Committed patchset #5 (id:80001) manually as
233bfd2da4acef06ad9de1cbb1d4ba4a0bf7380e (presubmit successful).