Refactor neteq_rtpplay

Refactor neteq_rtpplay

This change is a major refactoring of the neteq_rtpplay tool. It
consists of the following parts:

- NetEqTest class: Breaks out the main simulation loop from
  neteq_rtpplay into a separate class with well defined inputs and
  outputs.
- NetEqInput: Interface class for the input to NetEqTest.
- NetEqPacketSourceInput: Implementation of NetEqInput that provides a
  PacketSource objects with a NetEqInput interface. This has two
  subclasses; one for RtpFileSource and one for RtcEventLogSource.
- NetEqReplacementInput: An object that modifies the packets provided by
  another NetEqInput object, and replaces the packet payloads with meta
  data readable by a FakeDecodeFromFile decoder.
- FakeDecodeFromFile: An AudioDecoder implementation that produces
  "decoded" data by reading from an audio file.

BUG=webrtc:2692, webrtc:5447
Committed: https://crrev.com/e8a77e330942dd98ee86c09ac22850e0d4225944
Cr-Commit-Position: refs/heads/master@{#13252}

[hlundin-webrtc, 2016-06-01 16:14:39]

henrik.lundin@webrtc.org changed reviewers:
+ ivoc@webrtc.org

[hlundin-webrtc, 2016-06-01 16:14:40]

Ivo,

Please, review this when you get a chance.

Thanks!

[hlundin-webrtc, 2016-06-09 08:57:36]

Change how SSRC filtering works

[hlundin-webrtc, 2016-06-09 08:59:44]

Change how SSRC filtering works

[hlundin-webrtc, 2016-06-09 09:00:02]

Patchset #2 (id:20001) has been deleted

[ivoc, 2016-06-14 16:39:57]

Very nice update, code looks way better this way. See some (mostly minor)
comments below.

https://codereview.webrtc.org/2020363003/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.h (right):

https://codereview.webrtc.org/2020363003/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.h:48: // that the
FakeDecpdeFromFile decoder will understand. |encoded| must be at
Looks like a typo here.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.cc:32:
timestamp_to_decode != *next_timestamp_from_input_) {
Why would this happen?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_input.h (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:58: virtual
std::unique_ptr<PacketData> GetPacket() = 0;
Does this advance to the next packet? It's not very clear from the comments and
function name.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.cc
(right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.cc:42: return
std::unique_ptr<NetEqInput::PacketData>();
I think the "NetEqInput::" part can be removed (since you use it without it on
line 44).

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h
(right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h:40: class
RtpFileSource;
Isn't it normal to put the forward declarations at the top of the header?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h:59: class
RtcEventLogSource;
Same here.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:238: return
source_->NextPacketTime();
Can't the next source packet be invalid based on SSRC? In that case this value
would not be correct would it?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:249:
packet_to_return->header.header.ssrc == ssrc_);
I don't understand this DCHECK, can you explain why this holds?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:338: codecs[FLAGS_pcmu]
= std::make_pair(NetEqDecoder::kDecoderPCMu, "pcmu");
Since DecoderMap is a std::map, it should be possible to use an initializer
list, i.e. codecs = {{FLAGS_pcmu, std::make_pair(NetEqDecoder::kDecoderPCMu,
"pcmu")}, ...};

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:370:
ext_codecs.find(replacement_pt) != ext_codecs.end()) {
To me this format looks easier to understand, but it could be a matter of taste:
while (!(a == b && c == d))

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:375:
cn_types.insert(FLAGS_cn_nb);
Initializer list would be nice here, i.e. cn_types = {FLAGS_cn_nb, ...};

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:379: std::set<uint8_t>
forbidden_types;
And here.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:69: while
(input_->NextPacketTime() &&
I think this can be an if instead of a while now, right?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:83: while
(input_->NextOutputEventTime() &&
Same here, I think.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:103:
input_->AdvanceOutputEvent();
Perhaps there should be a corresponding function for packets?

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.h (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:15: #include <map>
Wrong order.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:28: virtual
~NetEqTestErrorCallback() = default;
Is this needed?

[hlundin-webrtc, 2016-06-17 10:27:42]

Review comments

[hlundin-webrtc, 2016-06-17 10:30:09]

Thanks for reviewing. PTAL again.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/fake_decode_from_file.cc:32:
timestamp_to_decode != *next_timestamp_from_input_) {
On 2016/06/14 16:39:56, ivoc wrote:
> Why would this happen?

This happens when packets are lost (through simulating packet losses). If the
code here detects a gap in the timestamps, it will skip the corresponding number
of samples from the file. Added a comment to explain.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_input.h (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:58: virtual
std::unique_ptr<PacketData> GetPacket() = 0;
On 2016/06/14 16:39:56, ivoc wrote:
> Does this advance to the next packet? It's not very clear from the comments
and
> function name.

It does advance to the next packet. I changed to PopPacket to align with common
nomenclature of STL containers. Also updated the comment.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.cc
(right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.cc:42: return
std::unique_ptr<NetEqInput::PacketData>();
On 2016/06/14 16:39:56, ivoc wrote:
> I think the "NetEqInput::" part can be removed (since you use it without it on
> line 44).

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h
(right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h:40: class
RtpFileSource;
On 2016/06/14 16:39:56, ivoc wrote:
> Isn't it normal to put the forward declarations at the top of the header?

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_packet_source_input.h:59: class
RtcEventLogSource;
On 2016/06/14 16:39:56, ivoc wrote:
> Same here.

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:238: return
source_->NextPacketTime();
On 2016/06/14 16:39:57, ivoc wrote:
> Can't the next source packet be invalid based on SSRC? In that case this value
> would not be correct would it?

No. The FindNextWithCorrectSsrc() method will always leave |source_| in a state
where a valid SSRC is first in line (or empty if no more packets are available).
I wrote a description about that in the PopPacket() method.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:249:
packet_to_return->header.header.ssrc == ssrc_);
On 2016/06/14 16:39:57, ivoc wrote:
> I don't understand this DCHECK, can you explain why this holds?

This holds because (1) only this method updates the packet we get from source_,
and (2) when this method returns, the source_ will always be in a state of
either having a valid SSRC first in line or no packet at all (end of source).

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:338: codecs[FLAGS_pcmu]
= std::make_pair(NetEqDecoder::kDecoderPCMu, "pcmu");
On 2016/06/14 16:39:57, ivoc wrote:
> Since DecoderMap is a std::map, it should be possible to use an initializer
> list, i.e. codecs = {{FLAGS_pcmu, std::make_pair(NetEqDecoder::kDecoderPCMu,
> "pcmu")}, ...};

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:370:
ext_codecs.find(replacement_pt) != ext_codecs.end()) {
On 2016/06/14 16:39:57, ivoc wrote:
> To me this format looks easier to understand, but it could be a matter of
taste:
> while (!(a == b && c == d))

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:375:
cn_types.insert(FLAGS_cn_nb);
On 2016/06/14 16:39:56, ivoc wrote:
> Initializer list would be nice here, i.e. cn_types = {FLAGS_cn_nb, ...};

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:379: std::set<uint8_t>
forbidden_types;
On 2016/06/14 16:39:56, ivoc wrote:
> And here.

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:69: while
(input_->NextPacketTime() &&
On 2016/06/14 16:39:57, ivoc wrote:
> I think this can be an if instead of a while now, right?

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:83: while
(input_->NextOutputEventTime() &&
On 2016/06/14 16:39:57, ivoc wrote:
> Same here, I think.

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:103:
input_->AdvanceOutputEvent();
On 2016/06/14 16:39:57, ivoc wrote:
> Perhaps there should be a corresponding function for packets?

No, I don't think so. The problem is that PopPacket (previously GetPacket) moves
the ownership of the packet to the caller. So, if PopPacket did not also do the
Advance... step, the NetEqInput object would be hanging in a bad state for a
while. That is:

t = input_->NextPacketTime();   // valid
p = input_->PopPacket();  // transfer ownership to p, but won't pre-fetch the
next packet
t = input_->NextPacketTime();  // INVALID
input_->AdvancePacket();
t = input_->NextPacketTime();  // valid again

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.h (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:15: #include <map>
On 2016/06/14 16:39:57, ivoc wrote:
> Wrong order.

Done.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:28: virtual
~NetEqTestErrorCallback() = default;
On 2016/06/14 16:39:57, ivoc wrote:
> Is this needed?

Compiler says yes. If I omit the explicit dtor, the compiler will generate a
non-virtual default dtor, but that is not allowed when we have virtual methods.

[ivoc, 2016-06-21 07:59:59]

LGTM, see some small points below.

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/2020363003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:238: return
source_->NextPacketTime();
On 2016/06/17 10:30:08, hlundin-webrtc wrote:
> On 2016/06/14 16:39:57, ivoc wrote:
> > Can't the next source packet be invalid based on SSRC? In that case this
value
> > would not be correct would it?
> 
> No. The FindNextWithCorrectSsrc() method will always leave |source_| in a
state
> where a valid SSRC is first in line (or empty if no more packets are
available).
> I wrote a description about that in the PopPacket() method.

Ah right, good point.

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_input.h (right):

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:57: // Returns the next
packet to be inserted into NetEq. The next packet is
In the first sentence when you say "next packet" you mean the one that is
returned, but in the second sentence when you say "next packet", I think you
mean the one after that. It makes the comment a little hard to read.

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:72: // delivered next by
GetPacket().
GetPacket() -> PopPacket().

[hlundin-webrtc, 2016-06-21 09:11:42]

Patchset #4 (id:80001) has been deleted

[hlundin-webrtc, 2016-06-21 09:12:20]

Thanks. I will now rebase, fix an issues, and commit.

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/neteq_input.h (right):

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:57: // Returns the next
packet to be inserted into NetEq. The next packet is
On 2016/06/21 07:59:58, ivoc wrote:
> In the first sentence when you say "next packet" you mean the one that is
> returned, but in the second sentence when you say "next packet", I think you
> mean the one after that. It makes the comment a little hard to read.

Good point. I re-wrote this.

https://codereview.webrtc.org/2020363003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/neteq_input.h:72: // delivered next by
GetPacket().
On 2016/06/21 07:59:58, ivoc wrote:
> GetPacket() -> PopPacket().

Done.

[hlundin-webrtc, 2016-06-21 13:39:19]

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

[commit-bot: I haz the power, 2016-06-21 13:39:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020363003/120001

[commit-bot: I haz the power, 2016-06-21 13:41:43]

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

[commit-bot: I haz the power, 2016-06-21 13:41:44]

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

[hlundin-webrtc, 2016-06-21 13:48:01]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-06-21 13:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2020363003/#ps140001 (title:
"Fixing 64-bit printf")

[hlundin-webrtc, 2016-06-21 13:48:02]

The CQ bit was unchecked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2016-06-21 13:48:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020363003/140001

[hlundin-webrtc, 2016-06-21 13:48:23]

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

[commit-bot: I haz the power, 2016-06-21 13:48:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020363003/140001

[commit-bot: I haz the power, 2016-06-21 13:50:54]

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

[commit-bot: I haz the power, 2016-06-21 13:50:57]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
  win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/1...)

[hlundin-webrtc, 2016-06-22 12:28:04]

Fixing win compilation and gyp dependencies

[hlundin-webrtc, 2016-06-22 12:52:06]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-06-22 12:52:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2020363003/#ps160001 (title:
"Fixing win compilation and gyp dependencies")

[commit-bot: I haz the power, 2016-06-22 12:52:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2020363003/160001

[commit-bot: I haz the power, 2016-06-22 13:34:08]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2016-06-22 13:34:12]

Description was changed from

==========
Refactor neteq_rtpplay

This change is a major refactoring of the neteq_rtpplay tool. It
consists of the following parts:

- NetEqTest class: Breaks out the main simulation loop from
  neteq_rtpplay into a separate class with well defined inputs and
  outputs.
- NetEqInput: Interface class for the input to NetEqTest.
- NetEqPacketSourceInput: Implementation of NetEqInput that provides a
  PacketSource objects with a NetEqInput interface. This has two
  subclasses; one for RtpFileSource and one for RtcEventLogSource.
- NetEqReplacementInput: An object that modifies the packets provided by
  another NetEqInput object, and replaces the packet payloads with meta
  data readable by a FakeDecodeFromFile decoder.
- FakeDecodeFromFile: An AudioDecoder implementation that produces
  "decoded" data by reading from an audio file.

BUG=webrtc:2692, webrtc:5447
==========

to

==========
Refactor neteq_rtpplay

This change is a major refactoring of the neteq_rtpplay tool. It
consists of the following parts:

- NetEqTest class: Breaks out the main simulation loop from
  neteq_rtpplay into a separate class with well defined inputs and
  outputs.
- NetEqInput: Interface class for the input to NetEqTest.
- NetEqPacketSourceInput: Implementation of NetEqInput that provides a
  PacketSource objects with a NetEqInput interface. This has two
  subclasses; one for RtpFileSource and one for RtcEventLogSource.
- NetEqReplacementInput: An object that modifies the packets provided by
  another NetEqInput object, and replaces the packet payloads with meta
  data readable by a FakeDecodeFromFile decoder.
- FakeDecodeFromFile: An AudioDecoder implementation that produces
  "decoded" data by reading from an audio file.

BUG=webrtc:2692, webrtc:5447

Committed: https://crrev.com/e8a77e330942dd98ee86c09ac22850e0d4225944
Cr-Commit-Position: refs/heads/master@{#13252}
==========

[commit-bot: I haz the power, 2016-06-22 13:34:13]

Patchset 7 (id:??) landed as
https://crrev.com/e8a77e330942dd98ee86c09ac22850e0d4225944
Cr-Commit-Position: refs/heads/master@{#13252}