On Sun, Jun 08, 2025 at 09:27:07AM +0200, Clément Péron wrote:
> Hi,
>
> On Sun, 8 Jun 2025 at 01:33, Michael Niedermayer <mich...@niedermayer.cc>
> wrote:
> >
> > Hi
> >
> > On Sat, Jun 07, 2025 at 10:06:19AM +0200, Clément Péron wrote:
> > > The rtp_set_prft() function incorrectly calculates the timestamp delta
> > > when RTP timestamps wrap around the 32-bit boundary. The current code:
> > >
> > > delta_timestamp = (int64_t)timestamp -
> > > (int64_t)s->last_rtcp_timestamp;
> > >
> > > treats both timestamps as large positive values, causing wraparound to
> > > produce a large negative delta instead of the correct small positive
> > > delta.
> > >
> > > For example, with a 90kHz video clock:
> > > - last_rtcp_timestamp = 0xFFFFFF00 (near wraparound)
> > > - timestamp = 0x00000100 (after wraparound)
> > > - Current result: delta ≈ -4.3 billion ticks ≈ -47,721 seconds
> > > - Expected result: delta ≈ +512 ticks ≈ +0.006 seconds
> > >
> > > This causes prft->wallclock to jump backward by approximately:
> > > - 90kHz video: ~47,721 seconds (~13.25 hours)
> > > - 48kHz audio: ~89,478 seconds (~24.9 hours)
> > > - 8kHz audio: ~536,871 seconds (~6.2 days)
> > >
> > > Fix by casting the subtraction result to int32_t, which correctly
> > > handles wraparound through modular arithmetic:
> > >
> > > delta_timestamp = (int32_t)(timestamp - s->last_rtcp_timestamp);
> > >
> > > This ensures the delta is always in the range [-2^31, 2^31-1], making
> > > wraparound produce the correct small positive values.
> > >
> > > Fixes timing jumps in applications that rely on Producer Reference Time
> > > for media synchronization.
> > >
> > > Signed-off-by: Clément Péron <peron.c...@gmail.com>
> > > ---
> > > libavformat/rtpdec.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> > > index 729bf83685..7664ab58bd 100644
> > > --- a/libavformat/rtpdec.c
> > > +++ b/libavformat/rtpdec.c
> > > @@ -633,7 +633,8 @@ void ff_rtp_parse_set_crypto(RTPDemuxContext *s,
> > > const char *suite,
> > > }
> > >
> > > static int rtp_set_prft(RTPDemuxContext *s, AVPacket *pkt, uint32_t
> > > timestamp) {
> > > - int64_t rtcp_time, delta_timestamp, delta_time;
> > > + int64_t rtcp_time, delta_time;
> > > + int32_t delta_timestamp;
> > >
> > > AVProducerReferenceTime *prft =
> > > (AVProducerReferenceTime *) av_packet_new_side_data(
> > > @@ -642,7 +643,7 @@ static int rtp_set_prft(RTPDemuxContext *s, AVPacket
> > > *pkt, uint32_t timestamp) {
> > > return AVERROR(ENOMEM);
> > >
> > > rtcp_time = ff_parse_ntp_time(s->last_rtcp_ntp_time) - NTP_OFFSET_US;
> > > - delta_timestamp = (int64_t)timestamp -
> > > (int64_t)s->last_rtcp_timestamp;
> > > + delta_timestamp = (int32_t)(timestamp - s->last_rtcp_timestamp);
> > > delta_time = av_rescale_q(delta_timestamp, s->st->time_base,
> > > AV_TIME_BASE_Q);
> > >
> > > prft->wallclock = rtcp_time + delta_time;
> >
> > will apply
>
> Thank you
>
> I found also another place where this rollover could be problematic
>
> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index 7664ab58bd..aa1ea46469 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -670,10 +670,10 @@ static void finalize_packet(RTPDemuxContext *s,
> AVPacket *pkt, uint32_t timestam
>
> if (s->last_rtcp_ntp_time != AV_NOPTS_VALUE && s->ic->nb_streams > 1) {
> int64_t addend;
> - int delta_timestamp;
> + int32_t delta_timestamp;
>
> /* compute pts from timestamp with received ntp_time */
> - delta_timestamp = timestamp - s->last_rtcp_timestamp;
> + delta_timestamp = (int32_t)(timestamp - s->last_rtcp_timestamp);
> /* convert to the PTS timebase */
> addend = av_rescale(s->last_rtcp_ntp_time - s->first_rtcp_ntp_time,
> s->st->time_base.den,
>
> But to be honest I don't have time to properly test it, but I could
> send a patch with an RFT tag if you want.better to have a patch than no patch, also this only affects 64bit int systems. I would tend to apply this fix even if noone tests it. Or am i missing something ? Its just more correct after the patch thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The difference between a dictatorship and a democracy is that every 4 years the population together is allowed to provide 1 bit of input to the government.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
