Simplify EncoderStateFeedback.

Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

Committed: https://crrev.com/45c44f0b94c3be9f35e351dda4d5b4ab17b44bd1
Cr-Commit-Position: refs/heads/master@{#11687}

[pbos-webrtc, 2016-02-17 14:25:02]

Description was changed from

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and complexities to deliver feedback more directly.

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

to

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

[danilchap, 2016-02-17 16:14:01]

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.cc:14: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
included in webrtc/video/encoder_state_feedback.h already

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.cc:73: // Unused since all SSRCs are set in
Init and filtered outside.
Can't see how Init is related to ssrc change.
this function is called when ssrc should be reset. (e.g. ViEChannel::SetSSRC
would trigger it)
old_ssrc becomes obsolete, new_ssrc is a number never seen before.
Would expect this function behave like before: replace ssrc in registered_ssrc
and notify encoder.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.h (left):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:66:
RTC_DISALLOW_COPY_AND_ASSIGN(EncoderStateFeedback);
Doesn't look like EncoderStateFeedback need to be copiable - restore
disallow_copy_and_assign macro.
Or, if it needs to be copiable, declare copy constructor/assignment explicitly
and remove constructormagic.h include

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.h (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:17: #include <map>
needed?

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:22: #include "webrtc/base/scoped_ptr.h"
not needed too

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:37: // Removes the registered encoder.
Necessary while ViEChannel outlives
may be write is as todo if there are plans to tear down TearDown function when
ViEChannel will be gone or ViEChannel and ViEEncoder swapped.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:54: class VieKeyRequestTest :
public ::testing::Test {
Since only one test left, may be class could be removed and all SetUp moved to
one place from current three.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:72: TEST_F(VieKeyRequestTest,
CreateAndTriggerRequests) {
OnLocalSsrcChanged probably should be tested too.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:73: const int ssrc = 1234;
const uint32_t ssrc

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:213: encoder_feedback_.Init(config_.rtp.ssrcs,
&vie_encoder_);
encoder_feedback obviously depends on vie_encoder_.
may be initialize order should reflect that. (i.e. move it one line down)

[pbos-webrtc, 2016-02-18 16:00:25]

feedback

[pbos-webrtc, 2016-02-18 16:00:30]

Thanks, PTAL.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.cc:14: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
On 2016/02/17 16:14:00, danilchap wrote:
> included in webrtc/video/encoder_state_feedback.h already

Done.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.h (left):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:66:
RTC_DISALLOW_COPY_AND_ASSIGN(EncoderStateFeedback);
On 2016/02/17 16:14:00, danilchap wrote:
> Doesn't look like EncoderStateFeedback need to be copiable - restore
> disallow_copy_and_assign macro.
> Or, if it needs to be copiable, declare copy constructor/assignment explicitly
> and remove constructormagic.h include

Removed the include, I don't think it semantically makes sense to copy this
class so I'm not sure it makes that much sense to put this macro here.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.h (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:17: #include <map>
On 2016/02/17 16:14:00, danilchap wrote:
> needed?

Done.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:22: #include "webrtc/base/scoped_ptr.h"
On 2016/02/17 16:14:00, danilchap wrote:
> not needed too

Done.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:37: // Removes the registered encoder.
Necessary while ViEChannel outlives
On 2016/02/17 16:14:00, danilchap wrote:
> may be write is as todo if there are plans to tear down TearDown function when
> ViEChannel will be gone or ViEChannel and ViEEncoder swapped.

Rewritten with a TODO.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:54: class VieKeyRequestTest :
public ::testing::Test {
On 2016/02/17 16:14:00, danilchap wrote:
> Since only one test left, may be class could be removed and all SetUp moved to
> one place from current three.

Done.

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:72: TEST_F(VieKeyRequestTest,
CreateAndTriggerRequests) {
On 2016/02/17 16:14:00, danilchap wrote:
> OnLocalSsrcChanged probably should be tested too.

Added DCHECKs to verify in runtime that it's never called, so there's no
functionality to verify (and I don't think a death test for it is important).

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback_unittest.cc:73: const int ssrc = 1234;
On 2016/02/17 16:14:01, danilchap wrote:
> const uint32_t ssrc

Done

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:213: encoder_feedback_.Init(config_.rtp.ssrcs,
&vie_encoder_);
On 2016/02/17 16:14:01, danilchap wrote:
> encoder_feedback obviously depends on vie_encoder_.
> may be initialize order should reflect that. (i.e. move it one line down)

Done.

[danilchap, 2016-02-18 17:18:56]

few more unused headers, then lgtm

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
File webrtc/video/encoder_state_feedback.h (left):

https://codereview.webrtc.org/1706803002/diff/1/webrtc/video/encoder_state_fe...
webrtc/video/encoder_state_feedback.h:66:
RTC_DISALLOW_COPY_AND_ASSIGN(EncoderStateFeedback);
On 2016/02/18 16:00:29, pbos-webrtc wrote:
> On 2016/02/17 16:14:00, danilchap wrote:
> > Doesn't look like EncoderStateFeedback need to be copiable - restore
> > disallow_copy_and_assign macro.
> > Or, if it needs to be copiable, declare copy constructor/assignment
explicitly
> > and remove constructormagic.h include
> 
> Removed the include, I don't think it semantically makes sense to copy this
> class so I'm not sure it makes that much sense to put this macro here.

Acknowledged.

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

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:18: #include
"webrtc/base/scoped_ptr.h"
not used now

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:22: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
think this one is not used too, even before this CL

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:24: #include
"webrtc/video/payload_router.h"
and this one

[pbos-webrtc, 2016-02-19 16:35:29]

remove unused headers

[pbos-webrtc, 2016-02-19 16:35:36]

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

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:18: #include
"webrtc/base/scoped_ptr.h"
On 2016/02/18 17:18:56, danilchap wrote:
> not used now

Done.

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:22: #include
"webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
On 2016/02/18 17:18:56, danilchap wrote:
> think this one is not used too, even before this CL

Done.

https://codereview.webrtc.org/1706803002/diff/20001/webrtc/video/encoder_stat...
webrtc/video/encoder_state_feedback_unittest.cc:24: #include
"webrtc/video/payload_router.h"
On 2016/02/18 17:18:56, danilchap wrote:
> and this one

Done.

[pbos-webrtc, 2016-02-19 16:36:18]

Description was changed from

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

to

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/45c44f0b94c3be9f35e351dda...
==========

[commit-bot: I haz the power, 2016-02-19 16:36:19]

Description was changed from

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/45c44f0b94c3be9f35e351dda...
==========

to

==========
Simplify EncoderStateFeedback.

EncoderStateFeedback is now only connected to one encoder, so remove map
and other complexity to deliver feedback more directly.

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

Committed: https://crrev.com/45c44f0b94c3be9f35e351dda4d5b4ab17b44bd1
Cr-Commit-Position: refs/heads/master@{#11687}
==========

[pbos-webrtc, 2016-02-19 16:36:19]

Committed patchset #3 (id:40001) manually as
45c44f0b94c3be9f35e351dda4d5b4ab17b44bd1 (presubmit successful).

[commit-bot: I haz the power, 2016-02-19 16:36:19]

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