Making FakeNetworkPipe demux audio and video packets.

Making FakeNetworkPipe demux audio and video packets.

BUG=None

Review-Url: https://codereview.webrtc.org/2794243002
Cr-Commit-Position: refs/heads/master@{#17629}
Committed: https://chromium.googlesource.com/external/webrtc/+/20c84ccd483df1e73a51f9852a9e25949f2a71cd

[minyue-webrtc, 2017-04-04 06:46:05]

Description was changed from

==========
fixing

BUG=
==========

to

==========
Fixing video loop back test.

The video loopback test fails due to https://codereview.webrtc.org/2686043006/.

This is a fix.

BUG=None
==========

[minyue-webrtc, 2017-04-04 06:46:05]

minyue@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[minyue-webrtc, 2017-04-04 06:47:59]

Karl,

Would you take a look at this? Please also my question inline.

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:1715: CodecInst{103, "OPUS", 48000, 960, 2,
64000};
Karl, 120 fails, which is weird to me. Please advice.

BTW, 120 works with the origin version of the relanded CL.

[minyue-webrtc, 2017-04-04 06:57:45]

On 2017/04/04 06:47:59, minyue-webrtc wrote:
> Karl,
> 
> Would you take a look at this? Please also my question inline.
> 
>
https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
> File webrtc/video/video_quality_test.cc (right):
> 
>
https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
> webrtc/video/video_quality_test.cc:1715: CodecInst{103, "OPUS", 48000, 960, 2,
> 64000};
> Karl, 120 fails, which is weird to me. Please advice.
> 
> BTW, 120 works with the origin version of the relanded CL.

Wait a second, I rebased the work and found it failed again. Let me try myself
first.

[minyue-webrtc, 2017-04-04 09:35:04]

Description was changed from

==========
Fixing video loop back test.

The video loopback test fails due to https://codereview.webrtc.org/2686043006/.

This is a fix.

BUG=None
==========

to

==========
Fixing video loop back test.

The video loopback test fails due to 
https://codereview.webrtc.org/2686043006/ and
https://codereview.webrtc.org/2774463003

This is a fix.

BUG=None
==========

[minyue-webrtc, 2017-04-04 09:35:04]

minyue@webrtc.org changed reviewers:
+ nisse@webrtc.org

[minyue-webrtc, 2017-04-04 09:35:44]

I found another cause of failure. Now this should be fixed totally. PTAL.

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:1715: CodecInst{103, "OPUS", 48000, 960, 2,
64000};
On 2017/04/04 06:47:59, minyue-webrtc wrote:
> Karl, 120 fails, which is weird to me. Please advice.
> 
> BTW, 120 works with the origin version of the relanded CL.

Continuing bisecting reflected another cause of failure, besides line 1727.

But 120 worked after rebasing. I am not sure what could cause it to work again,
and I am too lazy to find error in the relanding CL, which should have been
fixed anyway.

[minyue-webrtc, 2017-04-04 09:38:42]

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:1715: CodecInst{103, "OPUS", 48000, 960, 2,
64000};
On 2017/04/04 09:35:44, minyue-webrtc wrote:
> On 2017/04/04 06:47:59, minyue-webrtc wrote:
> > Karl, 120 fails, which is weird to me. Please advice.
> > 
> > BTW, 120 works with the origin version of the relanded CL.
> 
> Continuing bisecting reflected another cause of failure, besides line 1727.
> 
> But 120 worked after rebasing. I am not sure what could cause it to work
again,
> and I am too lazy to find error in https://codereview.webrtc.org/2774833003,
which should have been
> fixed anyway.

[nisse-webrtc, 2017-04-04 09:57:28]

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1767: MediaTypePacketReceiver
audio_receiver(call->Receiver(), MediaType::AUDIO);
If it's ok to use two separate transports in this test, I don't think you need
MediaTypePacketReceiver. Instead, make sure each transport is configured to
generate packets with the correct media type.

To use the same transport for both audio and video, you'd need a workaround
similar to CallTest::PayloadDemuxer.

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.h:24: // Helper class to ensure we deliver
correct media_type to the receiving call.
I wasn't aware of this class.

I think we can delete this one, and instead make sure that the underlying
FakeNetworkPipe is configured with the right media type.

[stefan-webrtc, 2017-04-05 08:52:16]

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

[stefan-webrtc, 2017-04-05 08:52:17]

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:1767: MediaTypePacketReceiver
audio_receiver(call->Receiver(), MediaType::AUDIO);
On 2017/04/04 09:57:27, nisse-webrtc wrote:
> If it's ok to use two separate transports in this test, I don't think you need
> MediaTypePacketReceiver. Instead, make sure each transport is configured to
> generate packets with the correct media type.
> 
> To use the same transport for both audio and video, you'd need a workaround
> similar to CallTest::PayloadDemuxer.

Let's not introduce an additional transport. We want to be able to test audio
and video over the same bottleneck.

[minyue-webrtc, 2017-04-05 08:55:10]

On 2017/04/05 08:52:17, stefan-webrtc wrote:
>
https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
> File webrtc/video/video_quality_test.cc (right):
> 
>
https://codereview.webrtc.org/2794243002/diff/20001/webrtc/video/video_qualit...
> webrtc/video/video_quality_test.cc:1767: MediaTypePacketReceiver
> audio_receiver(call->Receiver(), MediaType::AUDIO);
> On 2017/04/04 09:57:27, nisse-webrtc wrote:
> > If it's ok to use two separate transports in this test, I don't think you
need
> > MediaTypePacketReceiver. Instead, make sure each transport is configured to
> > generate packets with the correct media type.
> > 
> > To use the same transport for both audio and video, you'd need a workaround
> > similar to CallTest::PayloadDemuxer.
> 
> Let's not introduce an additional transport. We want to be able to test audio
> and video over the same bottleneck.

Yes, of course. I made wrong. Fixing coming.

[minyue-webrtc, 2017-04-05 21:21:05]

Patchset #3 (id:40001) has been deleted

[minyue-webrtc, 2017-04-05 21:35:11]

Patchset #3 (id:60001) has been deleted

[minyue-webrtc, 2017-04-05 21:45:42]

Patchset #3 (id:80001) has been deleted

[minyue-webrtc, 2017-04-05 21:55:31]

Patchset #3 (id:100001) has been deleted

[minyue-webrtc, 2017-04-06 08:40:13]

Patchset #3 (id:120001) has been deleted

[minyue-webrtc, 2017-04-06 08:43:06]

Description was changed from

==========
Fixing video loop back test.

The video loopback test fails due to 
https://codereview.webrtc.org/2686043006/ and
https://codereview.webrtc.org/2774463003

This is a fix.

BUG=None
==========

to

==========
Making FakeNetworkPipe demux audio and video packets.

BUG=None
==========

[minyue-webrtc, 2017-04-06 08:59:13]

Patchset #3 (id:140001) has been deleted

[minyue-webrtc, 2017-04-06 11:17:17]

Patchset #3 (id:160001) has been deleted

[minyue-webrtc, 2017-04-06 11:33:38]

I solved the problem from a lower level. Basically, per nisse@'s suggestion, I
moved the demuxing into fake network pipe. I like the solution so far. WDYT,
PTAL.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
File webrtc/audio/test/low_bandwidth_audio_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
webrtc/audio/test/low_bandwidth_audio_test.cc:92:
test::CallTest::kAudioSendPayloadType, "OPUS", 48000, 960, 2, 64000};
use the payload defined in CallTest so that
CreateReceiveTransport/CreateSendTransport can use
test::CallTest::payload_type_map_

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:217: RTC_CHECK(it != payload_type_map_.end())
I made it very strict here. All payloads have to be registered. I think it is
good to disallow any unexpected payload.

[minyue-webrtc, 2017-04-06 11:35:01]

minyue@webrtc.org changed reviewers:
+ oprypin@webrtc.org

[minyue-webrtc, 2017-04-06 11:35:01]

+oleh for the change in low bit rate audio test.

[oprypin_webrtc, 2017-04-06 11:45:32]

low_bandwidth_audio_test works fine after the change, just one question^

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
File webrtc/audio/test/low_bandwidth_audio_test.cc (left):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
webrtc/audio/test/low_bandwidth_audio_test.cc:25: 
Why does the constant need to be moved?

[stefan-webrtc, 2017-04-06 11:54:51]

Looks like a nice solution.

I wonder if it would be good to isolate demuxing from the fake network pipe, so
that the fake network pipe only knows about packets, but doesn't know about rtp?
Something like this:

Video/AudioSender -> webrtc::Transport -> FakeNetworkPipe -> DemuxingReceiver ->
Video/AudioReceiver

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:214: const uint8_t pt = packet_data[1] & 0x7f;
payload_type

[minyue-webrtc, 2017-04-06 11:56:33]

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
File webrtc/audio/test/low_bandwidth_audio_test.cc (left):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/audio/test/low_ba...
webrtc/audio/test/low_bandwidth_audio_test.cc:25: 
On 2017/04/06 11:45:32, oprypin_webrtc wrote:
> Why does the constant need to be moved?

Because I do not want to use "120" any longer. (But yes, you may argue that we
can move line 92 in PS3 here. I see only one line of using the const, so I think
inline is better.)

I have a comment in PS3. The basic idea is that for tests derived from CallTest,
we try not to introduce new payload types than those defined in CallTest, so
that when one creates a Transport, he/she does not need to consider what
payload_type_map to give. Arbitrary payload types may also lead to conflicts.

[nisse-webrtc, 2017-04-06 12:23:55]

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

[nisse-webrtc, 2017-04-06 12:23:56]

Thanks for doing this!

Adding perkj@, who implemented a related workaround in an internal application.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:206: });
Why do you need to split payload_type_map into audio_pt_map and video_pt_map?

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:433: {CallTest::kRedPayloadType, MediaType::VIDEO},
Looks like kRedPayloadType is 118, but here's a different value:

webrtc/test/fuzzers/ulpfec_generator_fuzzer.cc:constexpr uint8_t kRedPayloadType
= 97;

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:436: {CallTest::kFlexfecPayloadType, MediaType::VIDEO},
kFlexfecPayloadType is 120 here, but 123 in fuzzer tests and flexfec unit tests.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:437: {CallTest::kAudioSendPayloadType,
MediaType::AUDIO}};
And kAudioSendPayloadType is 103.

At least before this cl, 103 is used together with MediaType::VIDEO in
webrtc_tests and video_engine_tests. So either a collision, or a bug in how my
earlier cl configured the MediaType.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:57: const std::map<uint8_t,
webrtc::MediaType> additional_pt_map = {
What do you think about defining *all* payload types used by tests, and the
payload type map, in a single place? Should make it easier to avoid collisions.

[perkj_webrtc, 2017-04-06 13:49:54]

I like the approach with a map at creation time. It will work for our needs as
well. But you must allow backwards compatibility. ie, unknown packets should be
of type Video or ANY.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:217: RTC_CHECK(it != payload_type_map_.end())
On 2017/04/06 11:33:38, minyue-webrtc wrote:
> I made it very strict here. All payloads have to be registered. I think it is
> good to disallow any unexpected payload.

but it will break upstream since that is not the current behaviour. 
You will have to allow the old behaviour or use type ANY until this has landed
and upstream projects changed.

[minyue-webrtc, 2017-04-06 18:45:10]

Per stefan's suggestion, I separated demuxer out of FakeNetworkPipe.

To avoid extensive changes, FakeNetworkPipe creates and owns demuxer.

WDYT, Stefan?

I replied to some other comments. Further changes will follow when general
questions are addressed.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/call/call_perf_te...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/call/call_perf_te...
webrtc/call/call_perf_tests.cc:206: });
On 2017/04/06 12:23:56, nisse-webrtc wrote:
> Why do you need to split payload_type_map into audio_pt_map and video_pt_map?

Actually, it is always safe to use the superset. But I hope to check that no
accidental video payloads run into audio_send_transport and vice versa.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:433: {CallTest::kRedPayloadType, MediaType::VIDEO},
On 2017/04/06 12:23:56, nisse-webrtc wrote:
> Looks like kRedPayloadType is 118, but here's a different value:
> 
> webrtc/test/fuzzers/ulpfec_generator_fuzzer.cc:constexpr uint8_t
kRedPayloadType
> = 97;

ok. I will take a look at ulpfec_generator_fuzzer.cc, and make sure it runs
well.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:437: {CallTest::kAudioSendPayloadType,
MediaType::AUDIO}};
On 2017/04/06 12:23:56, nisse-webrtc wrote:
> And kAudioSendPayloadType is 103.
> 
> At least before this cl, 103 is used together with MediaType::VIDEO in
> webrtc_tests and video_engine_tests. So either a collision, or a bug in how my
> earlier cl configured the MediaType.

I should have solved them, since tests all passed now. But it would be good to
double check.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:217: RTC_CHECK(it != payload_type_map_.end())
On 2017/04/06 13:49:53, perkj_webrtc wrote:
> On 2017/04/06 11:33:38, minyue-webrtc wrote:
> > I made it very strict here. All payloads have to be registered. I think it
is
> > good to disallow any unexpected payload.
> 
> but it will break upstream since that is not the current behaviour. 
> You will have to allow the old behaviour or use type ANY until this has landed
> and upstream projects changed.

Ok. and so, is the change of ctor definition also needs to consider backward
compatibility?

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/video/video_quali...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/video/video_quali...
webrtc/video/video_quality_test.cc:57: const std::map<uint8_t,
webrtc::MediaType> additional_pt_map = {
On 2017/04/06 12:23:56, nisse-webrtc wrote:
> What do you think about defining *all* payload types used by tests, and the
> payload type map, in a single place? Should make it easier to avoid
collisions.

sure. will try

[perkj_webrtc, 2017-04-06 19:13:15]

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:217: RTC_CHECK(it != payload_type_map_.end())
On 2017/04/06 18:45:10, minyue-webrtc wrote:
> On 2017/04/06 13:49:53, perkj_webrtc wrote:
> > On 2017/04/06 11:33:38, minyue-webrtc wrote:
> > > I made it very strict here. All payloads have to be registered. I think it
> is
> > > good to disallow any unexpected payload.
> > 
> > but it will break upstream since that is not the current behaviour. 
> > You will have to allow the old behaviour or use type ANY until this has
landed
> > and upstream projects changed.
> 
> Ok. and so, is the change of ctor definition also needs to consider backward
> compatibility?

no, the "deprecated" version is used.

[minyue-webrtc, 2017-04-07 08:15:00]

Patchset #5 (id:220001) has been deleted

[minyue-webrtc, 2017-04-07 08:26:14]

Patchset #5 (id:240001) has been deleted

[minyue-webrtc, 2017-04-07 08:30:04]

Hej Per,

PS5 handles backward compatibility. PTAL.

[minyue-webrtc, 2017-04-07 09:16:28]

Hi all,

PS6 addresses other (small) comments. PTAL.

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:433: {CallTest::kRedPayloadType, MediaType::VIDEO},
On 2017/04/06 18:45:10, minyue-webrtc wrote:
> On 2017/04/06 12:23:56, nisse-webrtc wrote:
> > Looks like kRedPayloadType is 118, but here's a different value:
> > 
> > webrtc/test/fuzzers/ulpfec_generator_fuzzer.cc:constexpr uint8_t
> kRedPayloadType
> > = 97;
> 
> ok. I will take a look at ulpfec_generator_fuzzer.cc, and make sure it runs
> well.

I have read ulpfec_generator_fuzzer now, it does not use fake network pipe. So
it is not a problem. And I don't think we need to force  kRedPayloadType or
other payload type throughout webrtc, wdyt?

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:436: {CallTest::kFlexfecPayloadType, MediaType::VIDEO},
On 2017/04/06 12:23:56, nisse-webrtc wrote:
> kFlexfecPayloadType is 120 here, but 123 in fuzzer tests and flexfec unit
tests.

same here

https://codereview.webrtc.org/2794243002/diff/180001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:437: {CallTest::kAudioSendPayloadType,
MediaType::AUDIO}};
On 2017/04/06 18:45:10, minyue-webrtc wrote:
> On 2017/04/06 12:23:56, nisse-webrtc wrote:
> > And kAudioSendPayloadType is 103.
> > 
> > At least before this cl, 103 is used together with MediaType::VIDEO in
> > webrtc_tests and video_engine_tests. So either a collision, or a bug in how
my
> > earlier cl configured the MediaType.
> 
> I should have solved them, since tests all passed now. But it would be good to
> double check.

I could not find such conflict. Would you point it out?

[nisse-webrtc, 2017-04-07 10:04:26]

On 2017/04/07 09:16:28, minyue-webrtc wrote:
> > On 2017/04/06 12:23:56, nisse-webrtc wrote:
> > > And kAudioSendPayloadType is 103.
> > > 
> > > At least before this cl, 103 is used together with MediaType::VIDEO in
> > > webrtc_tests and video_engine_tests. So either a collision, or a bug in
how
> my
> > > earlier cl configured the MediaType.
> > 
> > I should have solved them, since tests all passed now. But it would be good
to
> > double check.
> 
> I could not find such conflict. Would you point it out?

What I did was I added logging like

    LOG(LS_INFO) << "XXX media_type " << static_cast<int>(media_type_)
                 << " pt " << static_cast<int>(packet->data()[1] & 0x7f)
                 << " pt-byte " << static_cast<int>(packet->data()[1]);

just before FakeNetworkPipe calls packet_receiver_->DeliverPacket. And then I
got

(fake_network_pipe.cc:206): XXX media_type 2 pt 103 pt-byte 103

from running webrtc_tests and video_engine_tests (2 is MediaType::Video), and

(fake_network_pipe.cc:206): XXX media_type 1 pt 103 pt-byte 103

from running webrtc_perf_tests.

I just greped for this message, not sure which the individual test cases were.
If all tests (including perf tests) work with your changes, I don't think we
need to worry about it.

[minyue-webrtc, 2017-04-07 13:17:31]

On 2017/04/07 10:04:26, nisse-webrtc wrote:
> On 2017/04/07 09:16:28, minyue-webrtc wrote:
> > > On 2017/04/06 12:23:56, nisse-webrtc wrote:
> > > > And kAudioSendPayloadType is 103.
> > > > 
> > > > At least before this cl, 103 is used together with MediaType::VIDEO in
> > > > webrtc_tests and video_engine_tests. So either a collision, or a bug in
> how
> > my
> > > > earlier cl configured the MediaType.
> > > 
> > > I should have solved them, since tests all passed now. But it would be
good
> to
> > > double check.
> > 
> > I could not find such conflict. Would you point it out?
> 
> What I did was I added logging like
> 
>     LOG(LS_INFO) << "XXX media_type " << static_cast<int>(media_type_)
>                  << " pt " << static_cast<int>(packet->data()[1] & 0x7f)
>                  << " pt-byte " << static_cast<int>(packet->data()[1]);
> 
> just before FakeNetworkPipe calls packet_receiver_->DeliverPacket. And then I
> got
> 
> (fake_network_pipe.cc:206): XXX media_type 2 pt 103 pt-byte 103
> 
> from running webrtc_tests and video_engine_tests (2 is MediaType::Video), and
> 
> (fake_network_pipe.cc:206): XXX media_type 1 pt 103 pt-byte 103
> 
> from running webrtc_perf_tests.
> 
> I just greped for this message, not sure which the individual test cases were.
> If all tests (including perf tests) work with your changes, I don't think we
> need to worry about it.

This must have been fixed by your earlier commit. I checked ToT, it does not
happen any longer.

[perkj_webrtc, 2017-04-07 14:24:15]

lgtm %

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:92: if (!demuxer_)
demuxer always exists right? It is moved into the ctor and the unique_ptr can be
made const and CHECK(demuxer) instead of if...

[stefan-webrtc, 2017-04-10 07:45:26]

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:472: CallTest::payload_type_map_,
Wouldn't it be cleaner to pass the payload_type_map into CreateSendTransport
than to rely on a static const?

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:60: static const std::map<uint8_t, MediaType>
payload_type_map_;
kPayloadTypeMap

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:77: // TODO(minyue): remove when the deprecate
ctors are removed.
deprecated

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:122: if (!demuxer_)
As Per pointed out above, this can also be removed.

[minyue-webrtc, 2017-04-10 07:58:46]

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:472: CallTest::payload_type_map_,
On 2017/04/10 07:45:26, stefan-webrtc wrote:
> Wouldn't it be cleaner to pass the payload_type_map into CreateSendTransport
> than to rely on a static const?

I also thought about it. I think it would be an improvement. Can I do it in a
follow up CL, to avoid possible inconvenience it may bring.

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:60: static const std::map<uint8_t, MediaType>
payload_type_map_;
On 2017/04/10 07:45:26, stefan-webrtc wrote:
> kPayloadTypeMap

Ok, I would like it but I learnt that only POD const can be kCamelCase.

[nisse-webrtc, 2017-04-10 09:33:57]

On 2017/04/07 14:24:15, perkj_webrtc wrote:
> lgtm %
> 
>
https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
> File webrtc/test/fake_network_pipe.cc (right):
> 
>
https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
> webrtc/test/fake_network_pipe.cc:92: if (!demuxer_)
> demuxer always exists right? It is moved into the ctor and the unique_ptr can
be
> made const and CHECK(demuxer) instead of if...

lgtm, % above. Further improvements can go in a followup cl.

[minyue-webrtc, 2017-04-10 09:56:10]

Hi,

comments are all addressed except for the kCamelCase on the none-POD const.

Stefan, do you still want me change to kPayloadTypeMap?

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:77: // TODO(minyue): remove when the deprecate
ctors are removed.
On 2017/04/10 07:45:26, stefan-webrtc wrote:
> deprecated

Done. and since there are several deprecated ctors, I clarified a bit more.

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:92: if (!demuxer_)
On 2017/04/07 14:24:15, perkj_webrtc wrote:
> demuxer always exists right? It is moved into the ctor and the unique_ptr can
be
> made const and CHECK(demuxer) instead of if...

Yes, that would be nice.

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:122: if (!demuxer_)
On 2017/04/10 07:45:26, stefan-webrtc wrote:
> As Per pointed out above, this can also be removed.

Done.

[minyue-webrtc, 2017-04-10 09:57:26]

https://codereview.webrtc.org/2794243002/diff/300001/webrtc/test/fake_network...
File webrtc/test/fake_network_pipe.cc (right):

https://codereview.webrtc.org/2794243002/diff/300001/webrtc/test/fake_network...
webrtc/test/fake_network_pipe.cc:122: // No demuxer means that this pipe will
terminate the flow of packets.
oh, I should have removed this comment. I meant to move it up to line 40

[stefan-webrtc, 2017-04-10 12:04:15]

lgtm

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc
File webrtc/test/call_test.cc (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc...
webrtc/test/call_test.cc:472: CallTest::payload_type_map_,
On 2017/04/10 07:58:46, minyue-webrtc wrote:
> On 2017/04/10 07:45:26, stefan-webrtc wrote:
> > Wouldn't it be cleaner to pass the payload_type_map into CreateSendTransport
> > than to rely on a static const?
> 
> I also thought about it. I think it would be an improvement. Can I do it in a
> follow up CL, to avoid possible inconvenience it may bring.

SG

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h
File webrtc/test/call_test.h (right):

https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h#...
webrtc/test/call_test.h:60: static const std::map<uint8_t, MediaType>
payload_type_map_;
On 2017/04/10 07:58:46, minyue-webrtc wrote:
> On 2017/04/10 07:45:26, stefan-webrtc wrote:
> > kPayloadTypeMap
> 
> Ok, I would like it but I learnt that only POD const can be kCamelCase.

Acknowledged.

[minyue-webrtc, 2017-04-10 12:18:52]

On 2017/04/10 12:04:15, stefan-webrtc wrote:
> lgtm
> 
> https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc
> File webrtc/test/call_test.cc (right):
> 
>
https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.cc...
> webrtc/test/call_test.cc:472: CallTest::payload_type_map_,
> On 2017/04/10 07:58:46, minyue-webrtc wrote:
> > On 2017/04/10 07:45:26, stefan-webrtc wrote:
> > > Wouldn't it be cleaner to pass the payload_type_map into
CreateSendTransport
> > > than to rely on a static const?
> > 
> > I also thought about it. I think it would be an improvement. Can I do it in
a
> > follow up CL, to avoid possible inconvenience it may bring.
> 
> SG
> 
> https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h
> File webrtc/test/call_test.h (right):
> 
>
https://codereview.webrtc.org/2794243002/diff/280001/webrtc/test/call_test.h#...
> webrtc/test/call_test.h:60: static const std::map<uint8_t, MediaType>
> payload_type_map_;
> On 2017/04/10 07:58:46, minyue-webrtc wrote:
> > On 2017/04/10 07:45:26, stefan-webrtc wrote:
> > > kPayloadTypeMap
> > 
> > Ok, I would like it but I learnt that only POD const can be kCamelCase.
> 
> Acknowledged.

Thanks!

Karl and Oleh,

any more comments? I am committing this if I don't hear from you.

[minyue-webrtc, 2017-04-10 13:56:09]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2017-04-10 13:56:10]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2794243002/#ps320001 (title:
"missing stuff")

[commit-bot: I haz the power, 2017-04-10 13:56:20]

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-04-10 13:57:50]

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

[commit-bot: I haz the power, 2017-04-10 13:57:52]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19406)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/19200)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24708)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[minyue-webrtc, 2017-04-10 18:19:29]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2017-04-10 18:19:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2794243002/#ps340001 (title:
"rebasing")

[commit-bot: I haz the power, 2017-04-10 18:19:40]

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-04-10 18:28:09]

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

[commit-bot: I haz the power, 2017-04-10 18:28:11]

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

[minyue-webrtc, 2017-04-10 20:52:39]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2017-04-10 20:52:39]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
stefan@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2794243002/#ps360001 (title:
"fixing android")

[commit-bot: I haz the power, 2017-04-10 20:52:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[minyue-webrtc, 2017-04-10 21:36:37]

https://codereview.webrtc.org/2794243002/diff/360001/webrtc/test/direct_trans...
File webrtc/test/direct_transport.h (right):

https://codereview.webrtc.org/2794243002/diff/360001/webrtc/test/direct_trans...
webrtc/test/direct_transport.h:44: : DirectTransport(
android bots do not allow a deprecated ctor calls another deprecated ctor,

[commit-bot: I haz the power, 2017-04-10 23:57:58]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1491857559178490,
"parent_rev": "d9ce76444f98b7abdfcacb0e5fbdb278163e8c07", "commit_rev":
"20c84ccd483df1e73a51f9852a9e25949f2a71cd"}

[commit-bot: I haz the power, 2017-04-10 23:58:02]

Description was changed from

==========
Making FakeNetworkPipe demux audio and video packets.

BUG=None
==========

to

==========
Making FakeNetworkPipe demux audio and video packets.

BUG=None

Review-Url: https://codereview.webrtc.org/2794243002
Cr-Commit-Position: refs/heads/master@{#17629}
Committed:
https://chromium.googlesource.com/external/webrtc/+/20c84ccd483df1e73a51f9852...
==========

[commit-bot: I haz the power, 2017-04-10 23:58:04]

Committed patchset #10 (id:360001) as
https://chromium.googlesource.com/external/webrtc/+/20c84ccd483df1e73a51f9852...