Simplify SSRC usage inside ViEEncoder.

Simplify SSRC usage inside ViEEncoder.

Since SSRCs can no longer change on the fly, SSRC code can be made a lot
simpler (and faster). Resulting code has less and shorter locking.

BUG=webrtc:5494
R=danilchap@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/0013dcc0c1b541acc51394b91c559a26db3bf4bc

[pbos-webrtc, 2016-02-19 18:08:01]

PTAL

[danilchap, 2016-02-19 19:25:56]

looks good, but can be a bit better

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:507: if (time_last_intra_request_ms_[i] +
kMinKeyFrameRequestIntervalMs >
no check if time_last_intra_request_ms_[i] is valid (!= -1)
Silently relying that now_ms >> kMinIntervalMs?

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (left):

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.h#ol...
webrtc/video/vie_encoder.h:46: class ViEEncoder : public RtcpIntraFrameObserver,
if you remove inheritence from RtcpIntraFrameObserver, then it it will be easier
to argue that OnReceivedIntraFrameRequest is called from EncoderStateFeedback
only that filters ssrcs.
Or even OnReceivedIntraFrameRequest can then take channel instead of ssrc,
eliminating ssrcs_ in VieEncoder_
also, ViEEncoder::OnLocalSsrcChanged could then be removed instead of DCHECKED.

[pbos-webrtc, 2016-02-19 19:30:07]

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:507: if (time_last_intra_request_ms_[i] +
kMinKeyFrameRequestIntervalMs >
On 2016/02/19 19:25:56, danilchap wrote:
> no check if time_last_intra_request_ms_[i] is valid (!= -1)
> Silently relying that now_ms >> kMinIntervalMs?

We're relying on now_ms >= 0 (pretty much everywhere), and at the -1 case is off
by one, so it doesn't really matter that we limit this to 300 or 301 ms. Either
I update the default to 0 or ignore this, I don't wanna have != -1 && ...

WDYT?

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (left):

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.h#ol...
webrtc/video/vie_encoder.h:46: class ViEEncoder : public RtcpIntraFrameObserver,
On 2016/02/19 19:25:56, danilchap wrote:
> if you remove inheritence from RtcpIntraFrameObserver, then it it will be
easier
> to argue that OnReceivedIntraFrameRequest is called from EncoderStateFeedback
> only that filters ssrcs.
> Or even OnReceivedIntraFrameRequest can then take channel instead of ssrc,
> eliminating ssrcs_ in VieEncoder_
> also, ViEEncoder::OnLocalSsrcChanged could then be removed instead of
DCHECKED.

Done.

[pbos-webrtc, 2016-02-19 19:30:20]

remove inheritance

[pbos-webrtc, 2016-02-19 19:33:22]

Remove OnLocalSsrcChange from test and suppressions

[danilchap, 2016-02-19 19:39:36]

lgtm

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:507: if (time_last_intra_request_ms_[i] +
kMinKeyFrameRequestIntervalMs >
On 2016/02/19 19:30:07, pbos-webrtc wrote:
> On 2016/02/19 19:25:56, danilchap wrote:
> > no check if time_last_intra_request_ms_[i] is valid (!= -1)
> > Silently relying that now_ms >> kMinIntervalMs?
> 
> We're relying on now_ms >= 0 (pretty much everywhere), and at the -1 case is
off
> by one, so it doesn't really matter that we limit this to 300 or 301 ms.
Either
> I update the default to 0 or ignore this, I don't wanna have != -1 && ...
> 
> WDYT?

Just want to be sure being implicit here is intended, not overlooked.

[pbos-webrtc, 2016-02-19 19:40:56]

On 2016/02/19 19:39:36, danilchap wrote:
> lgtm
> 
> https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc
> File webrtc/video/vie_encoder.cc (right):
> 
>
https://codereview.webrtc.org/1713683003/diff/1/webrtc/video/vie_encoder.cc#n...
> webrtc/video/vie_encoder.cc:507: if (time_last_intra_request_ms_[i] +
> kMinKeyFrameRequestIntervalMs >
> On 2016/02/19 19:30:07, pbos-webrtc wrote:
> > On 2016/02/19 19:25:56, danilchap wrote:
> > > no check if time_last_intra_request_ms_[i] is valid (!= -1)
> > > Silently relying that now_ms >> kMinIntervalMs?
> > 
> > We're relying on now_ms >= 0 (pretty much everywhere), and at the -1 case is
> off
> > by one, so it doesn't really matter that we limit this to 300 or 301 ms.
> Either
> > I update the default to 0 or ignore this, I don't wanna have != -1 && ...
> > 
> > WDYT?
> 
> Just want to be sure being implicit here is intended, not overlooked.

Yep, intended. Thanks!

[commit-bot: I haz the power, 2016-02-19 19:42:37]

Description was changed from

==========
Simplify SSRC usage inside ViEEncoder.

Since SSRCs can no longer change on the fly, SSRC code can be made a lot
simpler (and faster). Resulting code has less and shorter locking.

BUG=webrtc:5494
R=danilchap@webrtc.org
==========

to

==========
Simplify SSRC usage inside ViEEncoder.

Since SSRCs can no longer change on the fly, SSRC code can be made a lot
simpler (and faster). Resulting code has less and shorter locking.

BUG=webrtc:5494
R=danilchap@webrtc.org

Committed: https://crrev.com/0013dcc0c1b541acc51394b91c559a26db3bf4bc
Cr-Commit-Position: refs/heads/master@{#11691}
==========

[pbos-webrtc, 2016-02-19 19:42:37]

Description was changed from

==========
Simplify SSRC usage inside ViEEncoder.

Since SSRCs can no longer change on the fly, SSRC code can be made a lot
simpler (and faster). Resulting code has less and shorter locking.

BUG=webrtc:5494
R=danilchap@webrtc.org
==========

to

==========
Simplify SSRC usage inside ViEEncoder.

Since SSRCs can no longer change on the fly, SSRC code can be made a lot
simpler (and faster). Resulting code has less and shorter locking.

BUG=webrtc:5494
R=danilchap@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0013dcc0c1b541acc51394b91...
==========

[commit-bot: I haz the power, 2016-02-19 19:42:38]

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

[pbos-webrtc, 2016-02-19 19:42:39]

Committed patchset #3 (id:40001) manually as
0013dcc0c1b541acc51394b91c559a26db3bf4bc (presubmit successful).