Removed SSRC knowledge from ViEEncoder.

Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug recording is moved to VideoSendStream.
I think that make sence since that allows debug recording with external encoder implementations one day.

BUG=webrtc:5687
Committed: https://crrev.com/600246e63f9da0a050e58ef069d3860fef08b09a
Cr-Commit-Position: refs/heads/master@{#12632}

[perkj_webrtc, 2016-04-29 09:58:04]

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

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

Please.

[perkj_webrtc, 2016-04-29 10:01:12]

Description was changed from

==========
Removed SSRC knowledge from ViEEncoder.

BUG=webrtc:5687
==========

to

==========
Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug
recording is moved to VideoSendStream. 
I think that make sence since that allows debug recording with external encoder
implementations one day.

BUG=webrtc:5687
==========

[pbos-webrtc, 2016-05-02 01:05:27]

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
File webrtc/video/encoder_state_feedback.h (right):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback.h:41: int GetStreamIndex(uint32_t ssrc)
EXCLUSIVE_LOCKS_REQUIRED(crit_);
this should be a size_t

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:37:
MOCK_METHOD1(OnReceivedIntraFrameRequest, void(int));
size_t

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (left):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:437: if (time_last_intra_request_ms_[i] +
kMinKeyFrameRequestIntervalMs >
Is this logic handled somewhere else as well or is the limit intentionally
removed?

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

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:389: return;
remove

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.h:116: virtual void OnReceivedIntraFrameRequest(int
stream_index);
size_t

[pbos-webrtc, 2016-05-02 01:06:11]

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

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:616: encoded_image, codec_specific_info,
fragmentation);
== 0

[pbos-webrtc, 2016-05-02 01:06:46]

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

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:615: bool return_value =
payload_router_.Encoded(
eh, no, this should be int32_t, ignore comment below

[perkj_webrtc, 2016-05-03 10:22:40]

PTAL

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
File webrtc/video/encoder_state_feedback.h (right):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback.h:41: int GetStreamIndex(uint32_t ssrc)
EXCLUSIVE_LOCKS_REQUIRED(crit_);
On 2016/05/02 01:05:27, pbos-webrtc wrote:
> this should be a size_t

Done.

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

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:615: bool return_value =
payload_router_.Encoded(
On 2016/05/02 01:06:46, pbos-webrtc wrote:
> eh, no, this should be int32_t, ignore comment below

Done.

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (left):

https://codereview.webrtc.org/1936503002/diff/20001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:437: if (time_last_intra_request_ms_[i] +
kMinKeyFrameRequestIntervalMs >
On 2016/05/02 01:05:27, pbos-webrtc wrote:
> Is this logic handled somewhere else as well or is the limit intentionally
> removed?

no- unintentinonally actually. I misread what this does and thought it was not
used. I will move it to the feedback observer so that it will be independent of
encoder implementation.

[pbos-webrtc, 2016-05-03 18:19:59]

lgtm with my nits fixed

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:29:
time_last_intra_request_ms_.resize(ssrcs.size(), 0);
I think -1 is the usual no-timestamp timestamp.

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:63:
vie_encoder_->OnReceivedIntraFrameRequest(index);
I think you can drop crit_ during this callback.

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback_unittest.cc:86: 
Try one that's in the future but too soon (e.g advance time ~10ms?)

[perkj_webrtc, 2016-05-04 08:28:14]

Patchset #10 (id:180001) has been deleted

[perkj_webrtc, 2016-05-04 08:31:03]

PTAL- I removed EncoderStateFeedback::Init

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:29:
time_last_intra_request_ms_.resize(ssrcs.size(), 0);
On 2016/05/03 18:19:59, pbos-webrtc wrote:
> I think -1 is the usual no-timestamp timestamp.

Done.

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:63:
vie_encoder_->OnReceivedIntraFrameRequest(index);
On 2016/05/03 18:19:59, pbos-webrtc wrote:
> I think you can drop crit_ during this callback.

Done.

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1936503002/diff/120001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback_unittest.cc:86: 
On 2016/05/03 18:19:59, pbos-webrtc wrote:
> Try one that's in the future but too soon (e.g advance time ~10ms?)

Done.

[pbos-webrtc, 2016-05-04 14:10:36]

lgtm, nice!

https://codereview.webrtc.org/1936503002/diff/200001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1936503002/diff/200001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:68: RTC_DCHECK(vie_encoder_);
Remove all of these DCHECKs, if you want them keep them add one to the ctor.

[perkj_webrtc, 2016-05-04 15:25:39]

https://codereview.webrtc.org/1936503002/diff/200001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1936503002/diff/200001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback.cc:68: RTC_DCHECK(vie_encoder_);
On 2016/05/04 14:10:36, pbos-webrtc wrote:
> Remove all of these DCHECKs, if you want them keep them add one to the ctor.

Done.

[perkj_webrtc, 2016-05-04 18:25:06]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-05-04 18:25:06]

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/1936503002/#ps240001 (title:
"Rebased")

[commit-bot: I haz the power, 2016-05-04 18:25:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1936503002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1936503002/240001

[commit-bot: I haz the power, 2016-05-04 18:26:55]

Description was changed from

==========
Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug
recording is moved to VideoSendStream. 
I think that make sence since that allows debug recording with external encoder
implementations one day.

BUG=webrtc:5687
==========

to

==========
Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug
recording is moved to VideoSendStream. 
I think that make sence since that allows debug recording with external encoder
implementations one day.

BUG=webrtc:5687
==========

[commit-bot: I haz the power, 2016-05-04 18:26:56]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-05-04 18:27:02]

Description was changed from

==========
Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug
recording is moved to VideoSendStream. 
I think that make sence since that allows debug recording with external encoder
implementations one day.

BUG=webrtc:5687
==========

to

==========
Removed SSRC knowledge from ViEEncoder.

SSRC knowledge is contained withing VideoSendStream. That also means that debug
recording is moved to VideoSendStream.
I think that make sence since that allows debug recording with external encoder
implementations one day.

BUG=webrtc:5687

Committed: https://crrev.com/600246e63f9da0a050e58ef069d3860fef08b09a
Cr-Commit-Position: refs/heads/master@{#12632}
==========

[commit-bot: I haz the power, 2016-05-04 18:27:03]

Patchset 12 (id:??) landed as
https://crrev.com/600246e63f9da0a050e58ef069d3860fef08b09a
Cr-Commit-Position: refs/heads/master@{#12632}