Question about timestamp calculation in NetEQ's GetAudio

[lichuan lin]

Hi WebRTC Team,

I have a question regarding the timestamp calculation in NetEQImpl::GetAudio() method in neteq_impl.cc(https://webrtc.googlesource.com/src/+/refs/heads/main/modules/audio_coding/neteq/neteq_impl.cc). 

Code Location:
Line 681-685 in webrtc/modules/audio_coding/neteq/neteq_impl.cc

Current Code:

audio_frame->timestamp_ = first_packet_ ? 0 : timestamp_scaler_->ToExternal(playout_timestamp_) - static_cast<uint32_t>(audio_frame->samples_per_channel_);

My Question:
I noticed that ToExternal() converts the internal timestamp to external/RTP timestamp domain (which may use a different clock rate, e.g., G.722 uses 8000Hz RTP clock but 16000Hz sample rate). However, samples_per_channel represents the actual number of samples at the internal sample rate (e.g., 160 samples for 10ms @ 16kHz).

For codecs where RTP clock rate ≠ sample rate (like G.722), this calculation seems to mix two different units:

• ToExternal(playout_timestamp_) returns RTP timestamp units (e.g., @ 8000Hz)
• samples_per_channel is in physical sample count (e.g., @ 16000Hz)

Example for G.722:

• Internal playout_timestamp_ = 960 (@ 16000Hz)
• ToExternal(960) = 480 (@ 8000Hz RTP clock)
• samples_per_channel = 160 (@ 16000Hz physical samples)
• Result: 480 - 160 = 320

However, the correct RTP timestamp for the previous frame should be:

• Expected: 480 - 80 = 400 (where 80 = 10ms @ 8000Hz)

My Suggestion:
Would it be more accurate to compute the difference before conversion?

audio_frame->timestamp_ = timestamp_scaler_->ToExternal( playout_timestamp_ - audio_frame->samples_per_channel_);

Or is there a design consideration that I'm missing? The current implementation works correctly for Opus (where RTP clock = sample rate = 48000Hz), but may have a 2x error for G.722.

Looking forward to your clarification. Thank you!

Best regards,
Justeyllc