Let ViEEncoder handle resolution changes.

Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from WebRtcVideoSendStream to ViEEncoder.

With this change, many variables in WebRtcVideoSendStream no longer need to be locked.

BUG=webrtc:5687, webrtc:6371, webrtc:5332
Committed: https://crrev.com/26105b41b4f97642ee30cb067dc786c2737709ad
Cr-Commit-Position: refs/heads/master@{#14445}

[perkj_webrtc, 2016-09-21 10:35:18]

Patchset #4 (id:60001) has been deleted

[perkj_webrtc, 2016-09-21 13:22:22]

Description was changed from

==========
Let ViEEncoder handle resolution changes.

BUG=
==========

to

==========
Let ViEEncoder handle resolution changes.

BUG=
==========

[perkj_webrtc, 2016-09-21 13:22:22]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org, sprang@webrtc.org, stefan@webrtc.org

[perkj_webrtc, 2016-09-21 13:24:50]

Description was changed from

==========
Let ViEEncoder handle resolution changes.

BUG=
==========

to

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371
==========

[perkj_webrtc, 2016-09-21 13:28:55]

Can you please review the actual implementation? 

There are some tests that I need to write and the perf tests to fix but I would
like some initial comments.

[perkj_webrtc, 2016-09-21 13:28:56]

Can you please review the actual implementation? 

There are some tests that I need to write and the perf tests to fix but I would
like some initial comments.

[sprang_webrtc, 2016-09-22 13:06:17]

Overall looks good, or as good as one can hope for :{
All the various factory implementations become a bit verbose though, can those
be simplified somehow? Also, does it need to be a scoped refptr?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:656: // First time initialization. Frame size is
now known.
now? or not?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/base/fakevi...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/base/fakevi...
webrtc/media/base/fakevideocapturer.h:127: cricket::VideoFormat supported;
Not needed anymore?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:198: codec_settings_set_ =
config.encoder_specific_settings != NULL;
nit: nullptr

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1712: if (options_present ||
source_changing) {
This looks redundant?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2841: };
This looks like a duplicate from above. Also very similar to few other places.
Can we break it out to an external factory class for tests?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:353: }
This seems a bit roundabout.. If "preferred" target bitrate is a concept we want
to propagate around, maybe we can put that calculation in a better place and
just propagate the value here. Why do it via stats? Do we want it as actual
stats or was it just a shortcut in order to propagate it up to
WebrtcVideoSendStream?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:177: bool pending_encoder_reconfiguration_
ACCESS_ON(&encoder_queue_);
Is encoder_config_ only is in ReconfigureEncoder()? In that case, perhaps remove
this boolean and use an Optional<VideoEncoderConfig> instead?

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:180: unsigned int encoder_start_bitrate_bps_
ACCESS_ON(&encoder_queue_);
nit: uint32_t

[perkj_webrtc, 2016-09-26 12:09:42]

ptal

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:656: // First time initialization. Frame size is
now known.
On 2016/09/22 13:06:16, språng wrote:
> now? or not?

Done.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/base/fakevi...
File webrtc/media/base/fakevideocapturer.h (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/base/fakevi...
webrtc/media/base/fakevideocapturer.h:127: cricket::VideoFormat supported;
On 2016/09/22 13:06:16, språng wrote:
> Not needed anymore?

Done.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.cc:198: codec_settings_set_ =
config.encoder_specific_settings != NULL;
On 2016/09/22 13:06:16, språng wrote:
> nit: nullptr

Done.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1712: if (options_present ||
source_changing) {
On 2016/09/22 13:06:16, språng wrote:
> This looks redundant?

Done.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2841: };
On 2016/09/22 13:06:16, språng wrote:
> This looks like a duplicate from above. Also very similar to few other places.
> Can we break it out to an external factory class for tests?

yes but these tests override the default. That is why they are duplicated. Added
comment about that.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:353: }
On 2016/09/22 13:06:16, språng wrote:
> This seems a bit roundabout.. If "preferred" target bitrate is a concept we
want
> to propagate around, maybe we can put that calculation in a better place and
> just propagate the value here. Why do it via stats? Do we want it as actual
> stats or was it just a shortcut in order to propagate it up to
> WebrtcVideoSendStream?

This is a stat in WebrtcVideoSendStream and and it seems to be used by   at
least one wellknown product. So this moved the calculation from
WebrtcVideoSendStream::GetStats since it could no longer be calculated there.

I don't understand why preffered_bitrate != sum(stream.max_bitrate).

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:177: bool pending_encoder_reconfiguration_
ACCESS_ON(&encoder_queue_);
On 2016/09/22 13:06:16, språng wrote:
> Is encoder_config_ only is in ReconfigureEncoder()? In that case, perhaps
remove
> this boolean and use an Optional<VideoEncoderConfig> instead?

no- also when the input size change. The the encoder_config is also needed.
Added comment.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:180: unsigned int encoder_start_bitrate_bps_
ACCESS_ON(&encoder_queue_);
On 2016/09/22 13:06:16, språng wrote:
> nit: uint32_t

Done.

[sprang_webrtc, 2016-09-26 13:47:37]

lg with the discussed "preferred bitrate" calculation moved.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2841: };
On 2016/09/26 12:09:42, perkj_webrtc wrote:
> On 2016/09/22 13:06:16, språng wrote:
> > This looks like a duplicate from above. Also very similar to few other
places.
> > Can we break it out to an external factory class for tests?
> 
> yes but these tests override the default. That is why they are duplicated.
Added
> comment about that.

Acknowledged.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:353: }
On 2016/09/26 12:09:42, perkj_webrtc wrote:
> On 2016/09/22 13:06:16, språng wrote:
> > This seems a bit roundabout.. If "preferred" target bitrate is a concept we
> want
> > to propagate around, maybe we can put that calculation in a better place and
> > just propagate the value here. Why do it via stats? Do we want it as actual
> > stats or was it just a shortcut in order to propagate it up to
> > WebrtcVideoSendStream?
> 
> This is a stat in WebrtcVideoSendStream and and it seems to be used by   at
> least one wellknown product. So this moved the calculation from
> WebrtcVideoSendStream::GetStats since it could no longer be calculated there.
> 
> I don't understand why preffered_bitrate != sum(stream.max_bitrate).

As discussed offline, move to vie encoder and use rate allocator to calculate
the target value instead.

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/160001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:177: bool pending_encoder_reconfiguration_
ACCESS_ON(&encoder_queue_);
On 2016/09/26 12:09:42, perkj_webrtc wrote:
> On 2016/09/22 13:06:16, språng wrote:
> > Is encoder_config_ only is in ReconfigureEncoder()? In that case, perhaps
> remove
> > this boolean and use an Optional<VideoEncoderConfig> instead?
> 
> no- also when the input size change. The the encoder_config is also needed.
> Added comment.

Acknowledged.

[mflodman, 2016-09-27 11:28:01]

I need to take another look at vie_encoder.cc, but here are my other comments.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:665: if (encoder_inits_ == 3) {
This can be an else if instead.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/rampup_tests...
webrtc/call/rampup_tests.cc:17: #include "webrtc/test/encoder_settings.h"
Alphabetic order.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/config.h#newcode128
webrtc/config.h:128: struct VideoEncoderConfig {
Seems like this should be a class instead of a struct now.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:338: is_screencast_(is_screencast),
We could replace 'is_screencast_' and 'conference_mode_' with something like
'layered_screencast_'. And also DCHECK that this flag shouldn't' be set if
number_of_streams is set.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:324: // we currently does not express
CPU overuse using SinkWants in lower
s/does/do

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:67: size_t width_;
Can you add GUARDED_BY(...) here.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:422: // Reconfigure the encoder now if the encoder
has an internal source or
Great to get this comment here!

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:107: VideoRotation rotation,
Should include webrtc/common_video/rotation.h.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:176: unsigned int encoder_start_bitrate_bps_
ACCESS_ON(&encoder_queue_);
uint32_t ?

[perkj_webrtc, 2016-09-27 13:45:18]

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:665: if (encoder_inits_ == 3) {
On 2016/09/27 11:28:00, mflodman wrote:
> This can be an else if instead.

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/rampup_tests.cc
File webrtc/call/rampup_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/call/rampup_tests...
webrtc/call/rampup_tests.cc:17: #include "webrtc/test/encoder_settings.h"
On 2016/09/27 11:28:00, mflodman wrote:
> Alphabetic order.

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/config.h#newcode128
webrtc/config.h:128: struct VideoEncoderConfig {
On 2016/09/27 11:28:00, mflodman wrote:
> Seems like this should be a class instead of a struct now.

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/config.h#newcode161
webrtc/config.h:161: rtc::scoped_refptr<VideoStreamFactoryInterface>
encoder_stream_factory;
renamed video_stream_factory.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:338: is_screencast_(is_screencast),
On 2016/09/27 11:28:00, mflodman wrote:
> We could replace 'is_screencast_' and 'conference_mode_' with something like
> 'layered_screencast_'. And also DCHECK that this flag shouldn't' be set if
> number_of_streams is set.

On line 380 |is_screencast_| is used on its own to change something vp9 related.

I can dcheck number of streams if you want.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.h:324: // we currently does not express
CPU overuse using SinkWants in lower
On 2016/09/27 11:28:00, mflodman wrote:
> s/does/do

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:67: size_t width_;
On 2016/09/27 11:28:01, mflodman wrote:
> Can you add GUARDED_BY(...) here.

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:107: VideoRotation rotation,
On 2016/09/27 11:28:01, mflodman wrote:
> Should include webrtc/common_video/rotation.h.

Done.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:176: unsigned int encoder_start_bitrate_bps_
ACCESS_ON(&encoder_queue_);
On 2016/09/27 11:28:01, mflodman wrote:
> uint32_t ?

Done.

https://codereview.webrtc.org/2351633002/diff/220001/webrtc/test/encoder_sett...
File webrtc/test/encoder_settings.h (right):

https://codereview.webrtc.org/2351633002/diff/220001/webrtc/test/encoder_sett...
webrtc/test/encoder_settings.h:37: // Creates |encoder_config.number_of_streams|
VideoStreams where layer 0 has
fix this comment

[perkj_webrtc, 2016-09-27 14:29:57]

ptal

[mflodman, 2016-09-28 13:17:07]

Two more comments/questions, then LGTM.

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:338: is_screencast_(is_screencast),
On 2016/09/27 13:45:17, perkj_webrtc wrote:
> On 2016/09/27 11:28:00, mflodman wrote:
> > We could replace 'is_screencast_' and 'conference_mode_' with something like
> > 'layered_screencast_'. And also DCHECK that this flag shouldn't' be set if
> > number_of_streams is set.
> 
> On line 380 |is_screencast_| is used on its own to change something vp9
related.
> 
> I can dcheck number of streams if you want.

Right, I missed that one!

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:362: stream.max_framerate =
max_framerate_;
Do we need a DCHECK for non zero fps here?

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:103: class ConfigureEncoderTask;
Alphabetic order.

[sprang_webrtc, 2016-09-28 15:51:43]

lgtm with nits and a fix for the tsan breakage :)

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1302:
expect_resolution_set_.Wait(VideoSendStreamTest::kDefaultTimeoutMs));
Technically, you should verify that you actually woke because of the right
resolution being set, but...

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1312: expect_resolution_set_.Set();
nit: {}

[perkj_webrtc, 2016-09-29 12:12:38]

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

[commit-bot: I haz the power, 2016-09-29 12:12:43]

Dry run: 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-09-29 12:17:13]

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

[commit-bot: I haz the power, 2016-09-29 12:17:14]

Dry run: Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18708)

[perkj_webrtc, 2016-09-29 12:51:32]

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

[commit-bot: I haz the power, 2016-09-29 12:51:38]

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

[perkj_webrtc, 2016-09-29 12:56:49]

Description was changed from

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371
==========

to

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371, webrtc:5332
==========

[commit-bot: I haz the power, 2016-09-29 14:52:21]

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

[commit-bot: I haz the power, 2016-09-29 14:52:22]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[perkj_webrtc, 2016-09-29 15:05:18]

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:362: stream.max_framerate =
max_framerate_;
On 2016/09/28 13:17:07, mflodman wrote:
> Do we need a DCHECK for non zero fps here?

I don't think it should be here if its needed. It should be where it is actually
used for anything and the requirement is that it is > 0.

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1302:
expect_resolution_set_.Wait(VideoSendStreamTest::kDefaultTimeoutMs));
On 2016/09/28 15:51:43, språng wrote:
> Technically, you should verify that you actually woke because of the right
> resolution being set, but...

rewritten.

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:1312: expect_resolution_set_.Set();
On 2016/09/28 15:51:43, språng wrote:
> nit: {}

rewritten

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2351633002/diff/280001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:103: class ConfigureEncoderTask;
On 2016/09/28 13:17:07, mflodman wrote:
> Alphabetic order.

Done.

[perkj_webrtc, 2016-09-29 15:05:26]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-09-29 15:05:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2351633002/#ps340001 (title:
"Remove bad dchecks from ramp up test.")

[commit-bot: I haz the power, 2016-09-29 15:05:32]

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-09-29 17:06:00]

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

[commit-bot: I haz the power, 2016-09-29 17:06:02]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[perkj_webrtc, 2016-09-30 05:29:58]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2016-09-30 05:30:08]

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-09-30 05:39:14]

Description was changed from

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371, webrtc:5332
==========

to

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371, webrtc:5332
==========

[commit-bot: I haz the power, 2016-09-30 05:39:15]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2016-09-30 05:39:21]

Description was changed from

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder. 

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked. 

BUG=webrtc:5687, webrtc:6371, webrtc:5332
==========

to

==========
Let ViEEncoder handle resolution changes.

This cl move codec reconfiguration due to video frame size changes from
WebRtcVideoSendStream to ViEEncoder.

With this change, many variables in WebRtcVideoSendStream no longer need to be
locked.

BUG=webrtc:5687, webrtc:6371, webrtc:5332

Committed: https://crrev.com/26105b41b4f97642ee30cb067dc786c2737709ad
Cr-Commit-Position: refs/heads/master@{#14445}
==========

[commit-bot: I haz the power, 2016-09-30 05:39:23]

Patchset 17 (id:??) landed as
https://crrev.com/26105b41b4f97642ee30cb067dc786c2737709ad
Cr-Commit-Position: refs/heads/master@{#14445}

[perkj_webrtc, 2016-09-30 06:25:24]

A revert of this CL (patchset #17 id:340001) has been created in
https://codereview.webrtc.org/2383493005/ by perkj@webrtc.org.

The reason for reverting is: Fails on a content_browsertest (and also
webrtc_perf?)

https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds...

https://build.chromium.org/p/client.webrtc/builders/Linux64%20Release%20%5Bla...
[  FAILED  ] FullStackTest.ParisQcifWithoutPacketLoss (59436 ms).