Remove ViEncoder::Pause / Start

Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a BitrateObserver and VideoSendStream::Stop removes the stream as observer.

That also means that start will trigger a VideoEncoder::SetRate call with the most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.

BUG=webrtc:5687
b/28636240

Committed: https://crrev.com/57c21f9b44621b6415d81d353923eb3975276703
Cr-Commit-Position: refs/heads/master@{#13192}

[perkj_webrtc, 2016-06-16 12:52:50]

Description was changed from

==========
Remove ViEncoder::Pause / Start and replace with reporting zero bitrate.

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

to

==========
Remove ViEEncoder::Pause / Start
Instead, we call ViEEncoer.SetRate with zero bitrate.
If the encoder has an internal source, this trickles down to
VideoEncoder::SetRate. 


BUG=webrtc:5687 b/28636240
==========

[perkj_webrtc, 2016-06-16 12:52:50]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org, pbos@webrtc.org

[perkj_webrtc, 2016-06-16 12:53:08]

Can you please review?

[pbos-webrtc, 2016-06-16 13:16:28]

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

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:60: uint32_t
ProtectionBitrateCalculator::SetTargetRates(
This part of the code should be run synchronously (e.g. no concurrent
SetTargetRates), should we add a thread-checker or similar?

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

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:232: // full. We currently only
report this if the encoder has an internal source.
Put a comment here instead to explain why we don't tell this to non-internal
encoders.

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2450: RTC_DCHECK(encoder_config->streams.size()
== 1);
_EQ(1u,

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:533:
protection_bitrate_calculator_.GetEncoderTargetRate(), 0, 0);
What does it mean to put bogus values for fraction_loss and rtt here? Don't we
have OK values for them? Can we trigger this from the correct source instead
instead of fetching target rates?

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:855: return;  // The send stream is currently
not started.
Should the last fraction_loss and rtt be stored?

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:96:
video_sender_.SetChannelParameters(last_observed_bitrate_bps, 0, 0);
Can we skip 0, 0? Maybe by storing the last observed rtt/fraction_loss as well?

[perkj_webrtc, 2016-06-16 18:40:11]

Description was changed from

==========
Remove ViEEncoder::Pause / Start
Instead, we call ViEEncoer.SetRate with zero bitrate.
If the encoder has an internal source, this trickles down to
VideoEncoder::SetRate. 


BUG=webrtc:5687 b/28636240
==========

to

==========
Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a
BitrateObserver and VideoSendStream::Stop removes the stream as observer. 

That also means that start will trigger a VideoEncoder::SetRate call with the
most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.



BUG=webrtc:5687 b/28636240
==========

[perkj_webrtc, 2016-06-16 18:42:18]

ptal- I think this approach is cleaner.
And it will get even cleaner when we do all calls on a task queue instead of
everything in EncoderThreadProcess....

That will be the next cl.

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

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:60: uint32_t
ProtectionBitrateCalculator::SetTargetRates(
On 2016/06/16 13:16:28, pbos-webrtc wrote:
> This part of the code should be run synchronously (e.g. no concurrent
> SetTargetRates), should we add a thread-checker or similar?

I wish but not yet. If you have two VideoSendStreams they use two separate
threads to call into BitrateAllocator that will it turn call SetTargetRates....

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

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:232: // full. We currently only
report this if the encoder has an internal source.
On 2016/06/16 13:16:28, pbos-webrtc wrote:
> Put a comment here instead to explain why we don't tell this to non-internal
> encoders.

Done.

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2450: RTC_DCHECK(encoder_config->streams.size()
== 1);
On 2016/06/16 13:16:28, pbos-webrtc wrote:
> _EQ(1u, 

Done.

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:533:
protection_bitrate_calculator_.GetEncoderTargetRate(), 0, 0);
On 2016/06/16 13:16:28, pbos-webrtc wrote:
> What does it mean to put bogus values for fraction_loss and rtt here? Don't we
> have OK values for them? Can we trigger this from the correct source instead
> instead of fetching target rates?

There is no guarantee that the rtt and packet loss is valid when the transport
becomes writable and Start is called. 

Thinking about it, we should probably deregister from the BitrateAllocator.

We can store the last

https://codereview.webrtc.org/2070343002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:855: return;  // The send stream is currently
not started.
On 2016/06/16 13:16:28, pbos-webrtc wrote:
> Should the last fraction_loss and rtt be stored?

I dont' think they actually make sense. But we could.

[mflodman, 2016-06-17 11:28:17]

lgtm

pbos,
Any more comments?

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:131: return last_non_zero_bitrate_bps_ /
I think this is good enough for now and I'll open a discussion around skipping
the start bitrate in InitEncode call all together.

[pbos-webrtc, 2016-06-17 12:38:21]

lgtm w/ some nits

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:619: if (*pending_state_change == true &&
state_ == kStopped) {
no == true

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:629: } else if (*pending_state_change ==
false) {
!(*pending_state_change)

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:142: rtc::Optional<bool> pending_state_change_
GUARDED_BY(encoder_settings_crit_);
Can you make this Optional<State> and just not use kEncoderTimedOut? It's hard
to read because if (*pending_state_change_) reads too close to "is there a state
change", not "what's the next state change". I think:

if (*pending_state_change == kStarted) is a lot easier to read.

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:152: State state_;
Can you make this GUARDED_BY a thread checker for the encoder thread then?

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:41:
time_of_last_frame_activity_ms_(std::numeric_limits<int64_t>::max()),
Can you use -1 instead as unset? This type should never overflow so I'm uneasy
about keeping it close to the max.

[perkj_webrtc, 2016-06-17 13:06:32]

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:619: if (*pending_state_change == true &&
state_ == kStopped) {
On 2016/06/17 12:38:21, pbos-webrtc wrote:
> no == true

Done.

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:629: } else if (*pending_state_change ==
false) {
On 2016/06/17 12:38:21, pbos-webrtc wrote:
> !(*pending_state_change)

Done.

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:142: rtc::Optional<bool> pending_state_change_
GUARDED_BY(encoder_settings_crit_);
On 2016/06/17 12:38:21, pbos-webrtc wrote:
> Can you make this Optional<State> and just not use kEncoderTimedOut? It's hard
> to read because if (*pending_state_change_) reads too close to "is there a
state
> change", not "what's the next state change". I think:
> 
> if (*pending_state_change == kStarted) is a lot easier to read.

Done.

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:152: State state_;
On 2016/06/17 12:38:21, pbos-webrtc wrote:
> Can you make this GUARDED_BY a thread checker for the encoder thread then?

Done.

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2070343002/diff/80001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:41:
time_of_last_frame_activity_ms_(std::numeric_limits<int64_t>::max()),
On 2016/06/17 12:38:21, pbos-webrtc wrote:
> Can you use -1 instead as unset? This type should never overflow so I'm uneasy
> about keeping it close to the max.

It must be  large value at the moment. I will be fixed in the next cl where this
is removed...

[perkj_webrtc, 2016-06-17 13:08:12]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-06-17 13:08:13]

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

[commit-bot: I haz the power, 2016-06-17 13:08:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2070343002/100001

[commit-bot: I haz the power, 2016-06-17 14:27:21]

Description was changed from

==========
Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a
BitrateObserver and VideoSendStream::Stop removes the stream as observer. 

That also means that start will trigger a VideoEncoder::SetRate call with the
most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.



BUG=webrtc:5687 b/28636240
==========

to

==========
Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a
BitrateObserver and VideoSendStream::Stop removes the stream as observer. 

That also means that start will trigger a VideoEncoder::SetRate call with the
most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.



BUG=webrtc:5687 b/28636240
==========

[commit-bot: I haz the power, 2016-06-17 14:27:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-17 14:27:35]

Description was changed from

==========
Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a
BitrateObserver and VideoSendStream::Stop removes the stream as observer. 

That also means that start will trigger a VideoEncoder::SetRate call with the
most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.



BUG=webrtc:5687 b/28636240
==========

to

==========
Remove ViEEncoder::Pause / Start

This cl change so that VideoSendStream::Start adds the stream as a
BitrateObserver and VideoSendStream::Stop removes the stream as observer.

That also means that start will trigger a VideoEncoder::SetRate call with the
most recent bitrate estimate.
VideoSendStream::Stop will trigger a VideoEncoder::SetRate with bitrate  = 0.

BUG=webrtc:5687 b/28636240

Committed: https://crrev.com/57c21f9b44621b6415d81d353923eb3975276703
Cr-Commit-Position: refs/heads/master@{#13192}
==========

[commit-bot: I haz the power, 2016-06-17 14:27:36]

Patchset 6 (id:??) landed as
https://crrev.com/57c21f9b44621b6415d81d353923eb3975276703
Cr-Commit-Position: refs/heads/master@{#13192}