Fix race in WebRtcVideoEngine2Tests, improve coverage.

Fix race in WebRtcVideoEngine2Tests, improve coverage.

WebRtcVideoEngine2Test.ReconfiguresEncoderOnContentTypeChange was flaky,
so fixed that and added a new test case that is similar but that uses
fake streams instead to better cover actual encoder creation. I think
it makes sense to keep them both.

Tested with gtest_parallel using many threads and iterations.

BUG=webrtc:7275, webrtc:4172
This CL is deprecated and replaced by https://webrtc-review.googlesource.com/c/src/+/4386

[sprang_webrtc, 2017-03-01 13:52:51]

sprang@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2017-03-02 04:53:08]

Sorry for my confusion; I'm not the most familiar with this code, though I'd
like to learn more.

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:913: // fake streams and
verifies that new encoder instances are actually
I think I'm confused about the various encoder objects... This test calls
WaitForCreatedVideoEncoders, so what encoder instances is it *not* verifying are
created? The simulcast encoder wrappers? And why does using fake streams make a
difference?

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1030: // Encoder will get
configured twice, once one recreating, and again when
Did you mean to say "once when"? Also, does the encoder really *need* to be
configured twice, or should there be a TODO to optimize this?

[sprang_webrtc, 2017-03-02 14:45:08]

> I'm not the most familiar with this code

You and me both :)
So my testing is perhaps also a little confused.

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:913: // fake streams and
verifies that new encoder instances are actually
On 2017/03/02 04:53:07, Taylor Brandstetter wrote:
> I think I'm confused about the various encoder objects... This test calls
> WaitForCreatedVideoEncoders, so what encoder instances is it *not* verifying
are
> created? The simulcast encoder wrappers? And why does using fake streams make
a
> difference?

Yes, this does not test the simulcast encoder adapter being created, which is
what I want. I really, what I wanted to test is which encoder is being
instantiated in WebRtcVideoSendStream::CreateVideoEncoder(), but that doesn't
seem to be directly testable. Instead we must go via an external encoder
factory, which is always wrapped in a simulcast encoder adapter.

So I'm leaving this as an integration test just to make sure that we configure
encoders of the right kind and don't trigger any dchecks in webrtc.

With the fake encoders below I set content type VP9, which doesn't have
simulcast and so no wrapper is created. That makes it possible to check for
direct allocations, and since we can also check how many send streams have been
created and how many times the have been reconfigured, we can check the exact
behavior we expect with regards to reconfigure vs recreate.

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1030: // Encoder will get
configured twice, once one recreating, and again when
On 2017/03/02 04:53:07, Taylor Brandstetter wrote:
> Did you mean to say "once when"? Also, does the encoder really *need* to be
> configured twice, or should there be a TODO to optimize this?

"once when", yes.

It doesn't technically need to. It's the case we discussed in the last cl
regarding if ReconfigureEncoder() is guaranteed to be a subset of SetCodec() or
not.
I can add a todo at the call site of that extraneous reconfiguration.

[Taylor Brandstetter, 2017-03-02 19:36:19]

lgtm; thanks for the explanation.

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/2721333003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:1030: // Encoder will get
configured twice, once one recreating, and again when
On 2017/03/02 14:45:07, språng wrote:
> I can add a todo at the call site of that extraneous reconfiguration.

That sounds good.

[sprang_webrtc, 2017-03-03 08:51:52]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2017-03-03 08:51:55]

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-03-03 08:55:44]

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

[commit-bot: I haz the power, 2017-03-03 08:55:45]

Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/21871)

[sprang_webrtc, 2017-03-03 09:38:23]

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

[commit-bot: I haz the power, 2017-03-03 09:38:29]

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-03-03 09:58:40]

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

[commit-bot: I haz the power, 2017-03-03 09:58:40]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-03-03 10:06:55]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-03-03 10:06:55]

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2721333003/#ps40001 (title:
"s/EXPECT_EQ(false, /EXPECT_FALSE(")

[commit-bot: I haz the power, 2017-03-03 10:06:57]

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-03-03 10:11:07]

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

[commit-bot: I haz the power, 2017-03-03 10:11:08]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14422)

[sprang_webrtc, 2017-03-03 10:16:12]

pthatcher1@, ptal as owner

[sprang_webrtc, 2017-09-28 16:12:27]

Description was changed from

==========
Fix race in WebRtcVideoEngine2Tests, improve coverage.

WebRtcVideoEngine2Test.ReconfiguresEncoderOnContentTypeChange was flaky,
so fixed that and added a new test case that is similar but that uses
fake streams instead to better cover actual encoder creation. I think
it makes sense to keep them both.

Tested with gtest_parallel using many threads and iterations.

BUG=webrtc:7275, webrtc:4172
==========

to

==========
Fix race in WebRtcVideoEngine2Tests, improve coverage.

WebRtcVideoEngine2Test.ReconfiguresEncoderOnContentTypeChange was flaky,
so fixed that and added a new test case that is similar but that uses
fake streams instead to better cover actual encoder creation. I think
it makes sense to keep them both.

Tested with gtest_parallel using many threads and iterations.

BUG=webrtc:7275, webrtc:4172

This CL is deprecated and replaced by
https://webrtc-review.googlesource.com/c/src/+/4386
==========