Fixed problems in neteq when RTP and decoder timestamps increment with

Fixed problems in neteq when RTP and decoder timestamps increment with
different sample rate frequency.

BUG=webrtc:7327
Problems before the fix:
1. NetEqImpl::timestamp_ is inconsistent. Initially it is set to
the original RTP timestamp, but later gets updated with the
scaled timestamp.
2. NetEqImpl::InsertPacketInternal::main_timestamp is set with
the original RTP timestamp, but later gets compared with the
NetEqImpl::timestamp_ which may or may not be with the same
sample rate frequency and this results in major problems.
3. IncreaseEndTimestamp(main_timestamp - timestamp_) will be
incorrect when SSRC is changed and not the first packet.
4. delay_manager_->Update() may not be always invoked, since
the (main_timestamp - timestamp_) >= 0 will not be true when
the previous scaled timestamp_ is bigger than the main_timestamp
(current RTP timestamp) even if the current RTP timestamp is
bigger than the previous RTP timestamp.
5. delay_manager_->Update() parameters are main_timestamp
which increments with the RTP sample rate frequency and the
fs_hz_ which is the decoder sample rate frequency. When these
two frequencies are different as is the case with g.722, the
DelayManager::Update() will misfire and calculate incorrect
packet_len_ms and inter-arrival time (IAT) as a result. This
in effect will cause neteq to enter kPreemptiveExpand operation
and will keep expanding the jitter buffer even if the RTP packets
arrive with no jitter at all.

The fix corrects all these problems by making sure the
main_timestamp and the timestamp_ are always set with the scaled
timestamp and increment with the decoder sample rate frequency.

Review-Url: https://codereview.webrtc.org/2743063005
Cr-Commit-Position: refs/heads/master@{#17232}
Committed: https://chromium.googlesource.com/external/webrtc/+/e851a9a763dc2c0fca76a489e44eb2393dcabb55

[Dani Kirov, 2017-03-11 00:33:22]

The CQ bit was checked by dkirovbroadsoft@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 00:33: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-03-11 00:33:32]

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

[commit-bot: I haz the power, 2017-03-11 00:33:33]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[ossu, 2017-03-13 15:03:16]

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

[ossu, 2017-03-13 15:03:17]

This looks reasonable - always using internal timestamps for these calculations.
I'll try to activate QC and see if it holds up. :)
Also, please see comment.

https://codereview.webrtc.org/2743063005/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2743063005/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:597: if ((rtp_header.header.ssrc
!= ssrc_) || first_packet_) {
It seems to me that update_sample_rate_and_channels captures first_packet or
ssrc change, so perhaps refactor as such:

  bool update_sample_rate_and_channels = first_packet_ ||
(rtp_header.header.ssrc != ssrc_);
  if (update_sample_rate_and_channels) {
    timestamp_scaler_->Reset();
  }

[...] (ToInternal, copy values, etc.)

  if (update_sample_rate_and_channels) {
    rtcp_.Init(...);
  }

[ossu, 2017-03-13 15:03:23]

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

[commit-bot: I haz the power, 2017-03-13 15:03:34]

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-13 15:17:06]

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

[commit-bot: I haz the power, 2017-03-13 15:17:07]

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

[Dani Kirov, 2017-03-13 18:01:02]

On 2017/03/13 15:03:17, ossu wrote:
> This looks reasonable - always using internal timestamps for these
calculations.
> I'll try to activate QC and see if it holds up. :)
> Also, please see comment.
> 
>
https://codereview.webrtc.org/2743063005/diff/1/webrtc/modules/audio_coding/n...
> File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):
> 
>
https://codereview.webrtc.org/2743063005/diff/1/webrtc/modules/audio_coding/n...
> webrtc/modules/audio_coding/neteq/neteq_impl.cc:597: if
((rtp_header.header.ssrc
> != ssrc_) || first_packet_) {
> It seems to me that update_sample_rate_and_channels captures first_packet or
> ssrc change, so perhaps refactor as such:
> 
>   bool update_sample_rate_and_channels = first_packet_ ||
> (rtp_header.header.ssrc != ssrc_);
>   if (update_sample_rate_and_channels) {
>     timestamp_scaler_->Reset();
>   }
> 
> [...] (ToInternal, copy values, etc.)
> 
>   if (update_sample_rate_and_channels) {
>     rtcp_.Init(...);
>   }

Patch set 2 with code refactored as per review comment.

[ossu, 2017-03-14 10:17:17]

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

[commit-bot: I haz the power, 2017-03-14 10:17:22]

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-14 10:37:51]

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

[commit-bot: I haz the power, 2017-03-14 10:37:51]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2017-03-14 15:54:13]

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

[hlundin-webrtc, 2017-03-14 15:54:14]

LGTM

Thanks for fixing this!

[ossu, 2017-03-14 16:57:14]

lgtm

https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:605: if
(!decoder_database_->IsRed(rtp_header.header.payloadType)) {
Hmm... I'm not sure this _should_ be necessary. I'll have a look at that
separately.

[ossu, 2017-03-14 16:58:22]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2017-03-14 16:58:31]

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-14 17:00:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489510702687720,
"parent_rev": "184732b7c1baee9d6918b2f90c3a5b56846ae137", "commit_rev":
"e851a9a763dc2c0fca76a489e44eb2393dcabb55"}

[commit-bot: I haz the power, 2017-03-14 17:00:31]

Description was changed from

==========
Fixed problems in neteq when RTP and decoder timestamps increment with
different sample rate frequency.

BUG=webrtc:7327

Problems before the fix:
1. NetEqImpl::timestamp_ is inconsistent. Initially it is set to
the original RTP timestamp, but later gets updated with the
scaled timestamp.
2. NetEqImpl::InsertPacketInternal::main_timestamp is set with
the original RTP timestamp, but later gets compared with the
NetEqImpl::timestamp_ which may or may not be with the same
sample rate frequency and this results in major problems.
3. IncreaseEndTimestamp(main_timestamp - timestamp_) will be
incorrect when SSRC is changed and not the first packet.
4. delay_manager_->Update() may not be always invoked, since
the (main_timestamp - timestamp_) >= 0 will not be true when
the previous scaled timestamp_ is bigger than the main_timestamp
(current RTP timestamp) even if the current RTP timestamp is
bigger than the previous RTP timestamp.
5. delay_manager_->Update() parameters are main_timestamp
which increments with the RTP sample rate frequency and the
fs_hz_ which is the decoder sample rate frequency. When these
two frequencies are different as is the case with g.722, the
DelayManager::Update() will misfire and calculate incorrect
packet_len_ms and inter-arrival time (IAT) as a result. This
in effect will cause neteq to enter kPreemptiveExpand operation
and will keep expanding the jitter buffer even if the RTP packets
arrive with no jitter at all.

The fix corrects all these problems by making sure the
main_timestamp and the timestamp_ are always set with the scaled
timestamp and increment with the decoder sample rate frequency.
==========

to

==========
Fixed problems in neteq when RTP and decoder timestamps increment with
different sample rate frequency.

BUG=webrtc:7327

Problems before the fix:
1. NetEqImpl::timestamp_ is inconsistent. Initially it is set to
the original RTP timestamp, but later gets updated with the
scaled timestamp.
2. NetEqImpl::InsertPacketInternal::main_timestamp is set with
the original RTP timestamp, but later gets compared with the
NetEqImpl::timestamp_ which may or may not be with the same
sample rate frequency and this results in major problems.
3. IncreaseEndTimestamp(main_timestamp - timestamp_) will be
incorrect when SSRC is changed and not the first packet.
4. delay_manager_->Update() may not be always invoked, since
the (main_timestamp - timestamp_) >= 0 will not be true when
the previous scaled timestamp_ is bigger than the main_timestamp
(current RTP timestamp) even if the current RTP timestamp is
bigger than the previous RTP timestamp.
5. delay_manager_->Update() parameters are main_timestamp
which increments with the RTP sample rate frequency and the
fs_hz_ which is the decoder sample rate frequency. When these
two frequencies are different as is the case with g.722, the
DelayManager::Update() will misfire and calculate incorrect
packet_len_ms and inter-arrival time (IAT) as a result. This
in effect will cause neteq to enter kPreemptiveExpand operation
and will keep expanding the jitter buffer even if the RTP packets
arrive with no jitter at all.

The fix corrects all these problems by making sure the
main_timestamp and the timestamp_ are always set with the scaled
timestamp and increment with the decoder sample rate frequency.

Review-Url: https://codereview.webrtc.org/2743063005
Cr-Commit-Position: refs/heads/master@{#17232}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e851a9a763dc2c0fca76a489e...
==========

[commit-bot: I haz the power, 2017-03-14 17:00:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/e851a9a763dc2c0fca76a489e...

[Dani Kirov, 2017-03-14 18:32:03]

On 2017/03/14 16:57:14, ossu wrote:
> lgtm
> 
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):
> 
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> webrtc/modules/audio_coding/neteq/neteq_impl.cc:605: if
> (!decoder_database_->IsRed(rtp_header.header.payloadType)) {
> Hmm... I'm not sure this _should_ be necessary. I'll have a look at that
> separately.

If we scale RED payload, the code will hit debug check
RTC_DCHECK_EQ(1, !!decoder + !!cng_decoder_);
in DecoderInfo::SampleRateHz(). 
That's why the scaling in patch set 3 is applied on non-RED payloads only.

[ossu, 2017-03-20 09:32:03]

On 2017/03/14 18:32:03, Dani Kirov wrote:
> On 2017/03/14 16:57:14, ossu wrote:
> > lgtm
> > 
> >
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> > webrtc/modules/audio_coding/neteq/neteq_impl.cc:605: if
> > (!decoder_database_->IsRed(rtp_header.header.payloadType)) {
> > Hmm... I'm not sure this _should_ be necessary. I'll have a look at that
> > separately.
> 
> If we scale RED payload, the code will hit debug check
> RTC_DCHECK_EQ(1, !!decoder + !!cng_decoder_);
> in DecoderInfo::SampleRateHz(). 
> That's why the scaling in patch set 3 is applied on non-RED payloads only.

You are absolutely correct. Since I'm (at least partially) responsible for that
code, I'm pondering if it's reasonable to have that happen just because of a
request to get sample rate. Seems a bit surprising.

[Dani Kirov, 2017-03-20 21:34:54]

On 2017/03/20 09:32:03, ossu wrote:
> On 2017/03/14 18:32:03, Dani Kirov wrote:
> > On 2017/03/14 16:57:14, ossu wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> > > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2743063005/diff/40001/webrtc/modules/audio_codi...
> > > webrtc/modules/audio_coding/neteq/neteq_impl.cc:605: if
> > > (!decoder_database_->IsRed(rtp_header.header.payloadType)) {
> > > Hmm... I'm not sure this _should_ be necessary. I'll have a look at that
> > > separately.
> > 
> > If we scale RED payload, the code will hit debug check
> > RTC_DCHECK_EQ(1, !!decoder + !!cng_decoder_);
> > in DecoderInfo::SampleRateHz(). 
> > That's why the scaling in patch set 3 is applied on non-RED payloads only.
> 
> You are absolutely correct. Since I'm (at least partially) responsible for
that
> code, I'm pondering if it's reasonable to have that happen just because of a
> request to get sample rate. Seems a bit surprising.

Yes it is indeed surprising, but that seems to be the approach taken with the
webrtc code in general. This could be very problematic when such debug check
happens in the field and the app crashed as a result of it. Even with the most
complete unittest coverage, there could be still cases not covered by the
unittests and such debug check can still happen. For a real time communication
system, it would be better if this is handled as error and have error recovery
in place instead of crashing.
For this particular case, even if there was no debug check, functionally the
code should still scale timestamps only if the packet is not RED, otherwise we
may get double timestamp scaling when we scale the new packets that appear as a
result of RED splitting, or we have to keep track of what packets we had before
and after RED splitting and the logic becomes more complicated.