Reuse allocated encoders in SimulcastEncoderAdapter.

Reuse allocated encoders in SimulcastEncoderAdapter.

Prior to this change, the SimulcastEncoderAdapter would destroy and create
encoders whenever it is being reinitialized. After this change, the
SimulcastEncoderAdapter will cache the already allocated encoders, and reuse
them after reinitialization.

This change will help in reducing the number of PictureID "jumps" that have
been seen around encoder reinitialization.

TESTED=AppRTCMobile, Chrome desktop, and internal app, with forced encoder reinits every 30 frames and https://codereview.webrtc.org/2833493003/ applied.
BUG=webrtc:7475
Review-Url: https://codereview.webrtc.org/2830793005
Cr-Commit-Position: refs/heads/master@{#18215}
Committed: https://chromium.googlesource.com/external/webrtc/+/0b8bfb9d98b7a2011d80bcdf3f430bc040370dad

[brandtr, 2017-04-20 09:05:25]

Patchset #1 (id:1) has been deleted

[brandtr, 2017-04-20 09:09:50]

Patchset #2 (id:40001) has been deleted

[brandtr, 2017-04-20 11:56:45]

Description was changed from

==========
Reuse allocated encoders in SimulcastEncoderAdapter.

Prior to this change, the SimulcastEncoderAdapter would destroy and create
encoders whenever it is being reinitialized. After this change, the
SimulcastEncoderAdapter will cache the already allocated encoders, and reuse
them after reinitialization.

This change will help in reducing the number of PictureID "jumps" that have
been seen around encoder reinitialization.

BUG=webrtc:7475
==========

to

==========
Reuse allocated encoders in SimulcastEncoderAdapter.

Prior to this change, the SimulcastEncoderAdapter would destroy and create
encoders whenever it is being reinitialized. After this change, the
SimulcastEncoderAdapter will cache the already allocated encoders, and reuse
them after reinitialization.

This change will help in reducing the number of PictureID "jumps" that have
been seen around encoder reinitialization.

TESTED=AppRTCMobile, Chrome desktop, and internal app, with forced encoder
reinits every 30 frames and https://codereview.webrtc.org/2833493003/ applied.
BUG=webrtc:7475
==========

[brandtr, 2017-04-20 11:58:48]

brandtr@webrtc.org changed reviewers:
+ glaznev@webrtc.org, noahric@chromium.org, stefan@webrtc.org

[brandtr, 2017-04-20 11:58:49]

Please take a look.

holmer: general comments + OWNER

noahric: You seem familiar with earlier problems in this class, would you mind
taking a look?

glaznev: Are you aware of any assumptions in MediaCodecVideoEncoder, relying on
it being recreated whenever reinited?

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:229: encoder
= stored_encoders_.back();
I may want to rethink the order here, to ensure that the higher streams get to
reuse encoders before the lower streams get to do that.

This is based on an intuition that HW codec factories generally will output HW
codecs first, and when unable to do so, will output internal SW fallback codecs.
(Is this true in general?) We probably want to use the HW codecs for the highest
resolutions, i.e., the highest streams.

[AlexG, 2017-04-20 19:05:13]

> 
> glaznev: Are you aware of any assumptions in MediaCodecVideoEncoder, relying
on
> it being recreated whenever reinited?

I don't think there are any assumption

[noahric, 2017-04-24 17:27:01]

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:229: encoder
= stored_encoders_.back();
On 2017/04/20 11:58:49, brandtr wrote:
> I may want to rethink the order here, to ensure that the higher streams get to
> reuse encoders before the lower streams get to do that.
> 
> This is based on an intuition that HW codec factories generally will output HW
> codecs first, and when unable to do so, will output internal SW fallback
codecs.
> (Is this true in general?) We probably want to use the HW codecs for the
highest
> resolutions, i.e., the highest streams.

Two things:
1) You should try to use an equivalently sized codec, given that many encoders
can't dynamically be resized (though they can dynamically handle bitrate
changes)

2) I'm guessing they won't be re-used anyways in the current model if they've
been released. It will, at least, in the code I maintain. Though it's generally
a hard problem; that code also uses internal source encoders, so *not* calling
release will have it continue to produce frames.

I'm guessing the answer is that you want encoder resources destroyed not during
release but during destroy. My slight worry there is that inactive channels are
also released (I think) and not destroyed, so that may not work.

The answer may be to try to rework this so instead of doing a release + init, it
happens in a single pass, and equivalently configured encoders are just skipped.

[brandtr, 2017-04-26 11:41:23]

Rebase.

[brandtr, 2017-04-26 14:59:45]

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:229: encoder
= stored_encoders_.back();
> Two things:
> 1) You should try to use an equivalently sized codec, given that many encoders
> can't dynamically be resized (though they can dynamically handle bitrate
> changes)

Yes, that would be a nice optimization. Not sure it's worth the complexity
however, because my feeling is that this class is mainly used in two ways:

a) "Simulcast mode": Wrapping multiple encoders, with the QualityScaler
disabled. SimulcastEncoderAdapter::InitEncode() will be called at the beginning
of the call, and ::Release() will be called at the end.

b) "Single-stream mode": Wrapping a single encoder, with the QualityScaler
enabled. SimulcastEncoderAdapter::InitEncode() will be called at the beginning
of the call, and ::Release() will be called at the end. Mid-call, there might be
reinits due to the QualityScaler changing the resolution.

The difference that this CL makes is that in scenario b), the wrapped single
encoder C++ object will not be destroyed, and thus, the contained RTP state will
not be re-randomized mid call, thus reducing the number of mid-call picture_id
jumps. In addition to breaking the spec, these mid-call picture_id jumps have
been shown to confuse the jitter buffer, possibly leading to video distortions
on the receiving end.

Long-term, we should move the RTP state out of the encoder wrappers. This CL is
just intended to improve the situation in the interim.

> 2) I'm guessing they won't be re-used anyways in the current model if they've
> been released. It will, at least, in the code I maintain. Though it's
generally
> a hard problem; that code also uses internal source encoders, so *not* calling
> release will have it continue to produce frames.
> 
> I'm guessing the answer is that you want encoder resources destroyed not
during
> release but during destroy. My slight worry there is that inactive channels
are
> also released (I think) and not destroyed, so that may not work.
> 
> The answer may be to try to rework this so instead of doing a release + init,
it
> happens in a single pass, and equivalently configured encoders are just
skipped.

Although it would be nice to reuse the actual codec, in this CL I'm mainly
concerned with the wrapper object which contains the RTP state. So although your
proposed optimization sounds nice, I'd prefer not to change the semantics of
::InitEncode() and ::Release() right now.

[noahric, 2017-04-26 21:38:51]

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:229: encoder
= stored_encoders_.back();
On 2017/04/26 14:59:44, brandtr wrote:
> > Two things:
> > 1) You should try to use an equivalently sized codec, given that many
encoders
> > can't dynamically be resized (though they can dynamically handle bitrate
> > changes)
> 
> Yes, that would be a nice optimization. Not sure it's worth the complexity
> however, because my feeling is that this class is mainly used in two ways:
> 
> a) "Simulcast mode": Wrapping multiple encoders, with the QualityScaler
> disabled. SimulcastEncoderAdapter::InitEncode() will be called at the
beginning
> of the call, and ::Release() will be called at the end.
> 
> b) "Single-stream mode": Wrapping a single encoder, with the QualityScaler
> enabled. SimulcastEncoderAdapter::InitEncode() will be called at the beginning
> of the call, and ::Release() will be called at the end. Mid-call, there might
be
> reinits due to the QualityScaler changing the resolution.
> 
> The difference that this CL makes is that in scenario b), the wrapped single
> encoder C++ object will not be destroyed, and thus, the contained RTP state
will
> not be re-randomized mid call, thus reducing the number of mid-call picture_id
> jumps. In addition to breaking the spec, these mid-call picture_id jumps have
> been shown to confuse the jitter buffer, possibly leading to video distortions
> on the receiving end.
> 
> Long-term, we should move the RTP state out of the encoder wrappers. This CL
is
> just intended to improve the situation in the interim.
> 
> > 2) I'm guessing they won't be re-used anyways in the current model if
they've
> > been released. It will, at least, in the code I maintain. Though it's
> generally
> > a hard problem; that code also uses internal source encoders, so *not*
calling
> > release will have it continue to produce frames.
> > 
> > I'm guessing the answer is that you want encoder resources destroyed not
> during
> > release but during destroy. My slight worry there is that inactive channels
> are
> > also released (I think) and not destroyed, so that may not work.
> > 
> > The answer may be to try to rework this so instead of doing a release +
init,
> it
> > happens in a single pass, and equivalently configured encoders are just
> skipped.
> 
> Although it would be nice to reuse the actual codec, in this CL I'm mainly
> concerned with the wrapper object which contains the RTP state.

Ah, ok. Then I guess the question is just do I expect this to break anything in
the codebase I maintain, and the answer is "I don't think so".

I'll say that I think it is valuable to improve the simulcast case. In the code
I maintain, we basically do what I explained for Android: our MediaCodec
instances are loosely tied to the webrtc encoder instances and periodically
synchronize. When a configuration change happens, the manager of the (Java)
MediaCodec instances diffs the old/new state and keeps around MediaCodec
instances that haven't changed. But it's complex, and it'd be nicer to just have
this work at the SimulcastEncoderAdapter layer instead of trying to figure it
out after the fact.

[brandtr, 2017-04-27 10:43:31]

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:229: encoder
= stored_encoders_.back();
On 2017/04/26 21:38:51, noahric wrote:
> On 2017/04/26 14:59:44, brandtr wrote:
> > > Two things:
> > > 1) You should try to use an equivalently sized codec, given that many
> encoders
> > > can't dynamically be resized (though they can dynamically handle bitrate
> > > changes)
> > 
> > Yes, that would be a nice optimization. Not sure it's worth the complexity
> > however, because my feeling is that this class is mainly used in two ways:
> > 
> > a) "Simulcast mode": Wrapping multiple encoders, with the QualityScaler
> > disabled. SimulcastEncoderAdapter::InitEncode() will be called at the
> beginning
> > of the call, and ::Release() will be called at the end.
> > 
> > b) "Single-stream mode": Wrapping a single encoder, with the QualityScaler
> > enabled. SimulcastEncoderAdapter::InitEncode() will be called at the
beginning
> > of the call, and ::Release() will be called at the end. Mid-call, there
might
> be
> > reinits due to the QualityScaler changing the resolution.
> > 
> > The difference that this CL makes is that in scenario b), the wrapped single
> > encoder C++ object will not be destroyed, and thus, the contained RTP state
> will
> > not be re-randomized mid call, thus reducing the number of mid-call
picture_id
> > jumps. In addition to breaking the spec, these mid-call picture_id jumps
have
> > been shown to confuse the jitter buffer, possibly leading to video
distortions
> > on the receiving end.
> > 
> > Long-term, we should move the RTP state out of the encoder wrappers. This CL
> is
> > just intended to improve the situation in the interim.
> > 
> > > 2) I'm guessing they won't be re-used anyways in the current model if
> they've
> > > been released. It will, at least, in the code I maintain. Though it's
> > generally
> > > a hard problem; that code also uses internal source encoders, so *not*
> calling
> > > release will have it continue to produce frames.
> > > 
> > > I'm guessing the answer is that you want encoder resources destroyed not
> > during
> > > release but during destroy. My slight worry there is that inactive
channels
> > are
> > > also released (I think) and not destroyed, so that may not work.
> > > 
> > > The answer may be to try to rework this so instead of doing a release +
> init,
> > it
> > > happens in a single pass, and equivalently configured encoders are just
> > skipped.
> > 
> > Although it would be nice to reuse the actual codec, in this CL I'm mainly
> > concerned with the wrapper object which contains the RTP state.
> 
> Ah, ok. Then I guess the question is just do I expect this to break anything
in
> the codebase I maintain, and the answer is "I don't think so".

Thanks, that's good input to have!
 
> I'll say that I think it is valuable to improve the simulcast case. In the
code
> I maintain, we basically do what I explained for Android: our MediaCodec
> instances are loosely tied to the webrtc encoder instances and periodically
> synchronize. When a configuration change happens, the manager of the (Java)
> MediaCodec instances diffs the old/new state and keeps around MediaCodec
> instances that haven't changed. But it's complex, and it'd be nicer to just
have
> this work at the SimulcastEncoderAdapter layer instead of trying to figure it
> out after the fact.

I agree that it would make sense to improve this case -- trying to reduce the
number of effective reinits that are happening.

We are currently doing some work to refactor and clean up the video_coding
module. The insights gained from that work could hopefully inform any future
steps we want to take, for example, to try to actually reuse the underlying HW
codecs and not just the wrappers.

[brandtr, 2017-04-27 10:44:15]

ping holmer.

Also added some more unit tests, verifying that the outgoing frames belong to
the same simulcast index as they did before the reinit.

[brandtr, 2017-05-01 09:43:34]

Noah: does this LG?

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (left):

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:137:
Release();
This change is needed for the thread checks to pass.

Before this dtor is called, VCMGenericEncoder will have called Release(),
meaning that the underlying encoders have already been Released()'d. (This
implicit contract should probably be clarified in
api/video_codecs/video_encoder.h).
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...

Some of the wrapped encoders might call their Release() in their dtors. This
seems to be the case for our VP8/VP9 wrappers, but not the case for all our HW
codec wrappers. See e.g.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...

[stefan-webrtc, 2017-05-02 08:08:34]

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:503: for
(const auto& encoder : stored_encoders_) {
const VideoEncoder&

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:123:
std::vector<VideoEncoder*> stored_encoders_;
std::stack instead?

[brandtr, 2017-05-02 09:17:02]

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:503: for
(const auto& encoder : stored_encoders_) {
On 2017/05/02 08:08:34, stefan-webrtc wrote:
> const VideoEncoder&

Done.

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/2830793005/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:123:
std::vector<VideoEncoder*> stored_encoders_;
On 2017/05/02 08:08:34, stefan-webrtc wrote:
> std::stack instead?

Sure. Done.

[sprang_webrtc, 2017-05-11 11:53:35]

sprang@webrtc.org changed reviewers:
+ sprang@webrtc.org

[sprang_webrtc, 2017-05-11 11:53:36]

Code looks good, a few questions though.

The code path from the external API surfaces to the encoder instance is long and
windy and full of assumptions.
Is there any situation apart from reconfiguring the encoder or destroying the
send stream which could result in a Release() call on the encoder? If so, may we
end up holding onto an instance that the external user thinks has gone out of
scope?

Maybe a call-level integration test of some sort would be a good idea?

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:112: #endif
Maybe you can use an atomic instead?

[brandtr, 2017-05-12 08:13:10]

Rebase.

[brandtr, 2017-05-15 10:08:26]

sprang comments 1a: E2E test.

[brandtr, 2017-05-15 10:10:23]

Added E2E test that reinits encoder and recreates VideoSendStream.

By reverting the changes to SimulcastEncoderAdapter, the test fails:

~/s/w/src ❯❯❯ ninja -C out/Debug && out/Debug/video_engine_tests
--gtest_filter="*PictureId*"                          ⏎ TEST-Fail ⬆ ⬇ ✭
ninja: Entering directory `out/Debug'
[1/1] Regenerating ninja files
[39/39] STAMP obj/default.stamp
Note: Google Test filter = *PictureId*
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from EndToEndTest
[ RUN      ] EndToEndTest.RestartingVp8SendStreamPreservesPictureIdState
[       OK ] EndToEndTest.RestartingVp8SendStreamPreservesPictureIdState (3495
ms)
[ RUN      ]
EndToEndTest.RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState
[       OK ]
EndToEndTest.RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState
(3571 ms)
[----------] 2 tests from EndToEndTest (7066 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (7066 ms total)
[  PASSED  ] 2 tests.

~/s/w/src ❯❯❯ git revert 99a002a23048c42b542ee4c3966393bc3a7ddfc9               
                                        TEST-Fail ⬆ ⬇ ✭
[TEST-Fail 0e3284e23] Revert "Reuse allocated encoders in
SimulcastEncoderAdapter."
 4 files changed, 69 insertions(+), 363 deletions(-)

~/s/w/src ❯❯❯ ninja -C out/Debug && out/Debug/video_engine_tests
--gtest_filter="*PictureId*"                            TEST-Fail ⬆ ⬇ ✭
ninja: Entering directory `out/Debug'
[1/1] Regenerating ninja files
[39/39] STAMP obj/default.stamp
Note: Google Test filter = *PictureId*
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from EndToEndTest
[ RUN      ] EndToEndTest.RestartingVp8SendStreamPreservesPictureIdState
[       OK ] EndToEndTest.RestartingVp8SendStreamPreservesPictureIdState (3535
ms)
[ RUN      ]
EndToEndTest.RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 3079
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 3572
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 5565
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 3087
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 7478
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 5574
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 6328
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 7486
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 8417
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 6338
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 4133
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 2752
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 7011
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 26037
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 31672
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 8428
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 2936
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 4144
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 19508
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 7019
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 8095
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 31682
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 20118
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 8102
../../webrtc/video/end_to_end_tests.cc:4008: Failure
Value of: picture_id
  Actual: 28408
Expected: (last_observed_picture_id_[ssrc] + 1) % (1 << 15)
Which is: 20127
[  FAILED  ]
EndToEndTest.RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState
(3420 ms)
[----------] 2 tests from EndToEndTest (6955 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (6955 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ]
EndToEndTest.RestartingSimulcastEncoderAdapterSendStreamPreservesPictureIdState

 1 FAILED TEST

[brandtr, 2017-05-15 10:12:01]

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:112: #endif
On 2017/05/11 11:53:36, sprang_webrtc wrote:
> Maybe you can use an atomic instead?

Would be nice to not have this lock here at all. I'm not super familiar with
atomics, however. Could I just use rtc::AtomicOps::AcquireLoad and
rtc::AtomicOps::ReleaseStore on a volatile int, or do I need to use compare and
swap?

[brandtr, 2017-05-15 10:19:20]

Forgot to upload change that detaches sequenced task checker. Also deregister
callbacks when Release()'ing.

[brandtr, 2017-05-15 10:20:21]

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:156:
encoder->RegisterEncodeCompleteCallback(nullptr);
As discussed offline with sprang@.

[brandtr, 2017-05-15 10:49:06]

On 2017/05/11 11:53:36, sprang_webrtc wrote:
> Code looks good, a few questions though.
> 
> The code path from the external API surfaces to the encoder instance is long
and
> windy and full of assumptions.
> Is there any situation apart from reconfiguring the encoder or destroying the
> send stream which could result in a Release() call on the encoder? If so, may
we
> end up holding onto an instance that the external user thinks has gone out of
> scope?

Yes, it would be so much easier to reason about these types of changes if that
code path was simpler.

I did some code searching, and it seems that webrtc::VideoEncoder::Release() is
mainly called by VCMGenericEncoder::Release() and by the SW fallback adapter.
- The former happens when the VCMGenericEncoder is deleted, due to a encoder
reinit or call hangup. This is the scenario that you talk about in your
question.
- The latter happens during state transitions in the fallback adapter. The one
that is relevant to the SimulcastEncoderAdapter is when the underlying HW
encoder reports a failure, and the SW fallback encoder is initialized. Then the
(failing) SimulcastEncoderAdapter will be Release()'d, but not deleted. I think
this should be fine.

As discussed offline, I explicitly set the callback to null, to try to minimize
the risk that we are getting callbacks from an encoder that has been Release()'d
but not deleted. (This seems very unlikely to happen, but I don't think there
are any guarantees.)

For the future, I think it would make sense to change the order of the
SimulcastEncoderAdapter and the SW fallback. Right now, a single SW fallback
wraps a SimulcastEncoderAdapter, that might wrap multiple underlying HW
encoders...

> 
> Maybe a call-level integration test of some sort would be a good idea?
> 
>
https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
> File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h
(right):
> 
>
https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
> webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:112: #endif
> Maybe you can use an atomic instead?

[sprang_webrtc, 2017-05-15 12:03:39]

Thanks for the e2e test!
lgtm with nits

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/2830793005/diff/140001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:112: #endif
On 2017/05/15 10:12:01, brandtr wrote:
> On 2017/05/11 11:53:36, sprang_webrtc wrote:
> > Maybe you can use an atomic instead?
> 
> Would be nice to not have this lock here at all. I'm not super familiar with
> atomics, however. Could I just use rtc::AtomicOps::AcquireLoad and
> rtc::AtomicOps::ReleaseStore on a volatile int, or do I need to use compare
and
> swap?

rtc::AtomicOps::AcquireLoad and rtc::AtomicOps::ReleaseStore should be fine,
since the only thing you're really interested in is the memory barriers; you
don't need any atomic conditional changes.

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:156:
encoder->RegisterEncodeCompleteCallback(nullptr);
On 2017/05/15 10:20:20, brandtr wrote:
> As discussed offline with sprang@.

Acknowledged.

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

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:4067: SleepMs(100);
This seems like it could cause flakiness. Any way to avoid it without major
effort?

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:4137: // Ownership of
|video_encoder_factory_adapter| is transferred.
nit: Doesn't look like we need this temporary then?

[brandtr, 2017-05-15 13:27:23]

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

[commit-bot: I haz the power, 2017-05-15 13:27:31]

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, 2017-05-15 13:37:08]

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

[commit-bot: I haz the power, 2017-05-15 13:37:09]

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

[brandtr, 2017-05-15 13:59:21]

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

[commit-bot: I haz the power, 2017-05-15 13:59:28]

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, 2017-05-15 14:12:59]

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

[commit-bot: I haz the power, 2017-05-15 14:13:00]

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

[brandtr, 2017-05-15 15:52:18]

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

[commit-bot: I haz the power, 2017-05-15 15:52:25]

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, 2017-05-15 16:05:55]

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

[commit-bot: I haz the power, 2017-05-15 16:05:57]

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

[brandtr, 2017-05-15 16:11:16]

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

[commit-bot: I haz the power, 2017-05-15 16:11:25]

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, 2017-05-15 16:30:35]

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

[commit-bot: I haz the power, 2017-05-15 16:30:36]

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

[brandtr, 2017-05-15 17:14:04]

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

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:4067: SleepMs(100);
On 2017/05/15 12:03:39, sprang_webrtc wrote:
> This seems like it could cause flakiness. Any way to avoid it without major
> effort?

Yes, turns out this is super flaky :( I'll delete the VideoSendStream
recreations for now and leave a TODO. One robust solution would be to
synchronize the frame input with the packet output.

Long-term, I think we should move the picture_id state to the RTP module, where
the rest of the RTP state lives. Then we wouldn't have to worry about our
different encoder implementations doing the right thing w.r.t. picture_id, and
the synchronization wouldn't be needed here.

https://codereview.webrtc.org/2830793005/diff/200001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:4137: // Ownership of
|video_encoder_factory_adapter| is transferred.
On 2017/05/15 12:03:39, sprang_webrtc wrote:
> nit: Doesn't look like we need this temporary then?

Nope. Gone.

https://codereview.webrtc.org/2830793005/diff/220001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2830793005/diff/220001/webrtc/modules/video_cod...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:163:
rtc::AtomicOps::ReleaseStore(&inited_, 0);
sprang: would you mind checking that I'm using the atomics correctly here...?

[brandtr, 2017-05-15 17:16:13]

ping holmer for video/.

The tsan failure is a race in libvpx. I will check with marpan about that.

[stefan-webrtc, 2017-05-16 08:42:31]

lgtm for webrtc/video

[sprang_webrtc, 2017-05-16 11:40:56]

lgtm

[brandtr, 2017-05-17 07:12:27]

Rebase.

[brandtr, 2017-05-17 07:14:31]

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

[commit-bot: I haz the power, 2017-05-17 07:14:35]

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, 2017-05-17 07:44:37]

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

[commit-bot: I haz the power, 2017-05-17 07:44:38]

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

[brandtr, 2017-05-19 11:33:03]

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

[commit-bot: I haz the power, 2017-05-19 11:33:12]

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, 2017-05-19 11:53:43]

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

[commit-bot: I haz the power, 2017-05-19 11:53:44]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-19 13:49:01]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-05-19 13:49:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2830793005/#ps320001 (title:
"Disable new test on tsan due to pre-existing race in libvpx.")

[commit-bot: I haz the power, 2017-05-19 13:49:09]

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, 2017-05-19 13:51:43]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1495201741283700,
"parent_rev": "e87c87651f36650e76827671c4ec2a5c436ef5a9", "commit_rev":
"0b8bfb9d98b7a2011d80bcdf3f430bc040370dad"}

[commit-bot: I haz the power, 2017-05-19 13:51:47]

Description was changed from

==========
Reuse allocated encoders in SimulcastEncoderAdapter.

Prior to this change, the SimulcastEncoderAdapter would destroy and create
encoders whenever it is being reinitialized. After this change, the
SimulcastEncoderAdapter will cache the already allocated encoders, and reuse
them after reinitialization.

This change will help in reducing the number of PictureID "jumps" that have
been seen around encoder reinitialization.

TESTED=AppRTCMobile, Chrome desktop, and internal app, with forced encoder
reinits every 30 frames and https://codereview.webrtc.org/2833493003/ applied.
BUG=webrtc:7475
==========

to

==========
Reuse allocated encoders in SimulcastEncoderAdapter.

Prior to this change, the SimulcastEncoderAdapter would destroy and create
encoders whenever it is being reinitialized. After this change, the
SimulcastEncoderAdapter will cache the already allocated encoders, and reuse
them after reinitialization.

This change will help in reducing the number of PictureID "jumps" that have
been seen around encoder reinitialization.

TESTED=AppRTCMobile, Chrome desktop, and internal app, with forced encoder
reinits every 30 frames and https://codereview.webrtc.org/2833493003/ applied.
BUG=webrtc:7475

Review-Url: https://codereview.webrtc.org/2830793005
Cr-Commit-Position: refs/heads/master@{#18215}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0b8bfb9d98b7a2011d80bcdf3...
==========

[commit-bot: I haz the power, 2017-05-19 13:51:48]

Committed patchset #15 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/0b8bfb9d98b7a2011d80bcdf3...

[brandtr, 2017-05-19 17:32:54]

A revert of this CL (patchset #15 id:320001) has been created in
https://codereview.webrtc.org/2893003002/ by brandtr@webrtc.org.

The reason for reverting is: Breaks Chrome tests..

[brandtr, 2017-05-22 07:42:21]

Patchset #16 (id:340001) has been deleted