Don't hardcode MediaType::ANY in FakeNetworkPipe.

Don't hardcode MediaType::ANY in FakeNetworkPipe.

Instead let each test set the appropriate media type. This simplifies
demuxing in Call and later in RtpTransportController.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2774463003
Cr-Commit-Position: refs/heads/master@{#17418}
Committed: https://chromium.googlesource.com/external/webrtc/+/9c47b00e24da2941eb095df5a4459c6d98a8a88d

[nisse-webrtc, 2017-03-23 13:47:20]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, solenberg@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-03-23 13:47:21]

Hi,

this change is intended to make it simpler to demux incoming RTP packets, and in
addition make the tests behave more similar to the production code. PTAL.

[the sun, 2017-03-23 14:32:42]

https://codereview.webrtc.org/2774463003/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2774463003/diff/1/webrtc/call/call.cc#newcode1175
webrtc/call/call.cc:1175: RTC_DCHECK(media_type == MediaType::AUDIO ||
media_type == MediaType::VIDEO);
With this change, it looks like there are other places that use ::ANY that need
changing also. E.g. webrtc/video/replay.cc and
webrtc/media/engine/fakewebrtccall.cc.

[nisse-webrtc, 2017-03-23 15:27:29]

https://codereview.webrtc.org/2774463003/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2774463003/diff/1/webrtc/call/call.cc#newcode1175
webrtc/call/call.cc:1175: RTC_DCHECK(media_type == MediaType::AUDIO ||
media_type == MediaType::VIDEO);
On 2017/03/23 14:32:41, the sun wrote:
> With this change, it looks like there are other places that use ::ANY that
need
> changing also. E.g. webrtc/video/replay.cc and

Not sure about replay.cc. If it's video-only, we could just replace
MediaType::ANY with MediaType::Video there.

> webrtc/media/engine/fakewebrtccall.cc.

Looks like this accepts MediaType::ANY, but shouldn't be broken if we stop using
it.

And then there's some use related to rtc_event_log, but that looks independent
of the demuxing logic.

[nisse-webrtc, 2017-03-24 12:49:54]

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

[nisse-webrtc, 2017-03-24 12:49:55]

+pbos: Do you know what to do with replay.cc? 

I could try just replacing MediaType::ANY by MediaType::VIDEO, but are there any
tests for it so I can know if it's broken?

[pbos-webrtc, 2017-03-24 15:44:23]

On 2017/03/24 12:49:55, nisse-webrtc wrote:
> +pbos: Do you know what to do with replay.cc? 
> 
> I could try just replacing MediaType::ANY by MediaType::VIDEO, but are there
any
> tests for it so I can know if it's broken?

Don't remember, if I recall correctly it's only video so it sounds like it
should be OK. No tests cover replay.cc, you'd have to run rtp recordings through
it I think.

[nisse-webrtc, 2017-03-27 10:52:02]

On 2017/03/24 15:44:23, pbos-webrtc wrote:
> On 2017/03/24 12:49:55, nisse-webrtc wrote:
> > +pbos: Do you know what to do with replay.cc? 
> > 
> > I could try just replacing MediaType::ANY by MediaType::VIDEO, but are there
> any
> > tests for it so I can know if it's broken?
> 
> Don't remember, if I recall correctly it's only video so it sounds like it
> should be OK. No tests cover replay.cc, you'd have to run rtp recordings
through
> it I think.

Updated replay.cc. For testing, it would be good with some "rtp recording" under
resources/, and instructions on how to use it with replay.cc.

[nisse-webrtc, 2017-03-27 10:53:54]

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

[commit-bot: I haz the power, 2017-03-27 10:53:55]

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-27 10:58:25]

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

[commit-bot: I haz the power, 2017-03-27 10:58:26]

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

[nisse-webrtc, 2017-03-27 11:56:10]

Description was changed from

==========
Don't hardcode MediaType::ANY in FakeNetworkPipe.

Instead let each test set the appropriate media type. This simplifies
demuxing in Call and later in RtpTransportController.

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

to

==========
Don't hardcode MediaType::ANY in FakeNetworkPipe.

Instead let each test set the appropriate media type. This simplifies
demuxing in Call and later in RtpTransportController.

BUG=webrtc:7135
==========

[nisse-webrtc, 2017-03-27 11:57:25]

Ping?

Refactoring of receive side demux is blocked by this.

If more testing is needed for replay.cc, I'd need instruction son how to do a
manual test.

[the sun, 2017-03-27 14:54:47]

On 2017/03/27 11:57:25, nisse-webrtc wrote:
> Ping?
> 
> Refactoring of receive side demux is blocked by this.
> 
> If more testing is needed for replay.cc, I'd need instruction son how to do a
> manual test.

LGTM % one comment:

Remove MediaType::ANY from FakeCall::DeliverPacket() in
webrtc/media/engine/fakewebrtccall.cc. The fake call should to some extent
employ the same contract as the non fake.

[pbos-webrtc, 2017-03-27 16:02:44]

On 2017/03/27 10:52:02, nisse-webrtc wrote:
> On 2017/03/24 15:44:23, pbos-webrtc wrote:
> > On 2017/03/24 12:49:55, nisse-webrtc wrote:
> > > +pbos: Do you know what to do with replay.cc? 
> > > 
> > > I could try just replacing MediaType::ANY by MediaType::VIDEO, but are
there
> > any
> > > tests for it so I can know if it's broken?
> > 
> > Don't remember, if I recall correctly it's only video so it sounds like it
> > should be OK. No tests cover replay.cc, you'd have to run rtp recordings
> through
> > it I think.
> 
> Updated replay.cc. For testing, it would be good with some "rtp recording"
under
> resources/, and instructions on how to use it with replay.cc.

Given that there's no instantiation of anything audio in replay.cc I think this
won't break existing functionality (webrtc/video lgtm). Feel free to pick up
ownership of this binary if you want the latter part addressed. stefan@ might
have some RTP recording, or you could generate one and add it. :)

[nisse-webrtc, 2017-03-28 07:13:00]

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

[nisse-webrtc, 2017-03-28 07:13:00]

+sprang@. Can you have a look? OWNER's approval needed for video/.

[nisse-webrtc, 2017-03-28 07:17:43]

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

[commit-bot: I haz the power, 2017-03-28 07:17:51]

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-28 07:41:26]

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

[commit-bot: I haz the power, 2017-03-28 07:41:27]

Dry run: This issue passed the CQ dry run.

[sprang_webrtc, 2017-03-28 11:31:07]

lgtm

[nisse-webrtc, 2017-03-28 11:56:59]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-03-28 11:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2774463003/#ps60001 (title:
"Update FakeCall::DeliverPacket, for consistency with Call::DeliverRtp.")

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

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-28 11:59:42]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490702219163280,
"parent_rev": "0238ba83272cff3820b90890eb32f5be7ec6d1e7", "commit_rev":
"9c47b00e24da2941eb095df5a4459c6d98a8a88d"}

[commit-bot: I haz the power, 2017-03-28 11:59:44]

Description was changed from

==========
Don't hardcode MediaType::ANY in FakeNetworkPipe.

Instead let each test set the appropriate media type. This simplifies
demuxing in Call and later in RtpTransportController.

BUG=webrtc:7135
==========

to

==========
Don't hardcode MediaType::ANY in FakeNetworkPipe.

Instead let each test set the appropriate media type. This simplifies
demuxing in Call and later in RtpTransportController.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2774463003
Cr-Commit-Position: refs/heads/master@{#17418}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9c47b00e24da2941eb095df5a...
==========

[commit-bot: I haz the power, 2017-03-28 11:59:45]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/9c47b00e24da2941eb095df5a...

[lliuu, 2017-03-28 16:40:46]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/2784543002/ by lliuu@webrtc.org.

The reason for reverting is: Reverting since this seems to break multiple WebRTC
Perf buildbots.