diff mbox series

[FFmpeg-devel] avformat/rtpdec: Fix prft wallclock time.

Message ID 20210325044636.360896-1-alokpr@gmail.com
State Accepted
Commit 62f486e7930c5f1c8e4bfe342eb6a2bbd8cfd177
Headers show
Series [FFmpeg-devel] avformat/rtpdec: Fix prft wallclock time. | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Alok Priyadarshi March 25, 2021, 4:46 a.m. UTC 
Timestamp difference is available in media timebase (1/90K) where as
rtcp time is in the default microseconds timebase. This patch fixes
the calculated prft wallclock time by rescaling the timestamp delta
to the microseconds timebase.
---
 libavformat/rtpdec.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Carl Eugen Hoyos March 25, 2021, 9:14 a.m. UTC | #1 
Am Do., 25. März 2021 um 05:47 Uhr schrieb Alok Priyadarshi <alokpr@gmail.com>:
>
> Timestamp difference is available in media timebase (1/90K) where as
> rtcp time is in the default microseconds timebase. This patch fixes
> the calculated prft wallclock time by rescaling the timestamp delta
> to the microseconds timebase.
> ---
>  libavformat/rtpdec.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index b935dba1b8..21c1d01785 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -585,14 +585,19 @@ 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;
> +
>      AVProducerReferenceTime *prft =
>          (AVProducerReferenceTime *) av_packet_new_side_data(
>              pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
>      if (!prft)
>          return AVERROR(ENOMEM);
>
> -    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) - NTP_OFFSET_US +

> -                      timestamp - s->last_rtcp_timestamp;

Wouldn't this patch get much more readable if you only replace this line?

> +    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_time = av_rescale_q(delta_timestamp, s->st->time_base, AV_TIME_BASE_Q);
> +
> +    prft->wallclock = rtcp_time + delta_time;

Carl Eugen
Alok Priyadarshi March 25, 2021, 1:40 p.m. UTC | #2 
In effect the patch does replace that one line. But it also adds the steps
to illustrate how the wallclock is calculated. Adding all the calculations
in a single line will make it too long and hard to read.

Note that delta_timestamp can be negative. It typically happens after rtcp
SR is received and last_rtcp_ntp_time/last_rtcp_timestamp are refreshed.
The packet timestamp can be less than last_rtcp_timestamp for a brief
period of time. So it is necessary to explicitly cast both - timestamp
and last_rtcp_timestamp - to int64 before calculating delta. This was
another bug in the old code in addition to missing timebase scaling.

On Thu, Mar 25, 2021 at 2:23 AM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Do., 25. März 2021 um 05:47 Uhr schrieb Alok Priyadarshi <
> alokpr@gmail.com>:
> >
> > Timestamp difference is available in media timebase (1/90K) where as
> > rtcp time is in the default microseconds timebase. This patch fixes
> > the calculated prft wallclock time by rescaling the timestamp delta
> > to the microseconds timebase.
> > ---
> >  libavformat/rtpdec.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> > index b935dba1b8..21c1d01785 100644
> > --- a/libavformat/rtpdec.c
> > +++ b/libavformat/rtpdec.c
> > @@ -585,14 +585,19 @@ 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;
> > +
> >      AVProducerReferenceTime *prft =
> >          (AVProducerReferenceTime *) av_packet_new_side_data(
> >              pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
> >      if (!prft)
> >          return AVERROR(ENOMEM);
> >
> > -    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) -
> NTP_OFFSET_US +
>
> > -                      timestamp - s->last_rtcp_timestamp;
>
> Wouldn't this patch get much more readable if you only replace this line?
>
> > +    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_time = av_rescale_q(delta_timestamp, s->st->time_base,
> AV_TIME_BASE_Q);
> > +
> > +    prft->wallclock = rtcp_time + delta_time;
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Alok Priyadarshi March 30, 2021, 2:49 p.m. UTC | #3 
Ping. Could someone please review this patch. Thanks.

On Thu, Mar 25, 2021, 6:40 AM Alok Priyadarshi <alokpr@gmail.com> wrote:

> In effect the patch does replace that one line. But it also adds the steps
> to illustrate how the wallclock is calculated. Adding all the calculations
> in a single line will make it too long and hard to read.
>
> Note that delta_timestamp can be negative. It typically happens after rtcp
> SR is received and last_rtcp_ntp_time/last_rtcp_timestamp are refreshed.
> The packet timestamp can be less than last_rtcp_timestamp for a brief
> period of time. So it is necessary to explicitly cast both - timestamp
> and last_rtcp_timestamp - to int64 before calculating delta. This was
> another bug in the old code in addition to missing timebase scaling.
>
> On Thu, Mar 25, 2021 at 2:23 AM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>
>> Am Do., 25. März 2021 um 05:47 Uhr schrieb Alok Priyadarshi <
>> alokpr@gmail.com>:
>> >
>> > Timestamp difference is available in media timebase (1/90K) where as
>> > rtcp time is in the default microseconds timebase. This patch fixes
>> > the calculated prft wallclock time by rescaling the timestamp delta
>> > to the microseconds timebase.
>> > ---
>> >  libavformat/rtpdec.c | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
>> > index b935dba1b8..21c1d01785 100644
>> > --- a/libavformat/rtpdec.c
>> > +++ b/libavformat/rtpdec.c
>> > @@ -585,14 +585,19 @@ 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;
>> > +
>> >      AVProducerReferenceTime *prft =
>> >          (AVProducerReferenceTime *) av_packet_new_side_data(
>> >              pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
>> >      if (!prft)
>> >          return AVERROR(ENOMEM);
>> >
>> > -    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) -
>> NTP_OFFSET_US +
>>
>> > -                      timestamp - s->last_rtcp_timestamp;
>>
>> Wouldn't this patch get much more readable if you only replace this line?
>>
>> > +    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_time = av_rescale_q(delta_timestamp, s->st->time_base,
>> AV_TIME_BASE_Q);
>> > +
>> > +    prft->wallclock = rtcp_time + delta_time;
>>
>> Carl Eugen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>
James Almer March 30, 2021, 3:01 p.m. UTC | #4 
On 3/25/2021 1:46 AM, Alok Priyadarshi wrote:
> Timestamp difference is available in media timebase (1/90K) where as
> rtcp time is in the default microseconds timebase. This patch fixes
> the calculated prft wallclock time by rescaling the timestamp delta
> to the microseconds timebase.
> ---
>   libavformat/rtpdec.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index b935dba1b8..21c1d01785 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -585,14 +585,19 @@ 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;
> +
>       AVProducerReferenceTime *prft =
>           (AVProducerReferenceTime *) av_packet_new_side_data(
>               pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
>       if (!prft)
>           return AVERROR(ENOMEM);
>   
> -    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) - NTP_OFFSET_US +
> -                      timestamp - s->last_rtcp_timestamp;
> +    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_time = av_rescale_q(delta_timestamp, s->st->time_base, AV_TIME_BASE_Q);
> +
> +    prft->wallclock = rtcp_time + delta_time;
>       prft->flags = 24;
>       return 0;
>   }

Applied, thanks.
Carl Eugen Hoyos March 30, 2021, 9:14 p.m. UTC | #5 
Am Di., 30. März 2021 um 17:02 Uhr schrieb James Almer <jamrial@gmail.com>:
>
> On 3/25/2021 1:46 AM, Alok Priyadarshi wrote:
> > Timestamp difference is available in media timebase (1/90K) where as
> > rtcp time is in the default microseconds timebase. This patch fixes
> > the calculated prft wallclock time by rescaling the timestamp delta
> > to the microseconds timebase.
> > ---
> >   libavformat/rtpdec.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> > index b935dba1b8..21c1d01785 100644
> > --- a/libavformat/rtpdec.c
> > +++ b/libavformat/rtpdec.c
> > @@ -585,14 +585,19 @@ 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;
> > +
> >       AVProducerReferenceTime *prft =
> >           (AVProducerReferenceTime *) av_packet_new_side_data(
> >               pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
> >       if (!prft)
> >           return AVERROR(ENOMEM);
> >
> > -    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) - NTP_OFFSET_US +
> > -                      timestamp - s->last_rtcp_timestamp;
> > +    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_time = av_rescale_q(delta_timestamp, s->st->time_base, AV_TIME_BASE_Q);
> > +
> > +    prft->wallclock = rtcp_time + delta_time;
> >       prft->flags = 24;
> >       return 0;
> >   }
>
> Applied, thanks.

I still believe that such patches should not get applied.

Carl Eugen
diff mbox series

Patch

diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
index b935dba1b8..21c1d01785 100644
--- a/libavformat/rtpdec.c
+++ b/libavformat/rtpdec.c
@@ -585,14 +585,19 @@  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;
+
     AVProducerReferenceTime *prft =
         (AVProducerReferenceTime *) av_packet_new_side_data(
             pkt, AV_PKT_DATA_PRFT, sizeof(AVProducerReferenceTime));
     if (!prft)
         return AVERROR(ENOMEM);
 
-    prft->wallclock = ff_parse_ntp_time(s->last_rtcp_ntp_time) - NTP_OFFSET_US +
-                      timestamp - s->last_rtcp_timestamp;
+    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_time = av_rescale_q(delta_timestamp, s->st->time_base, AV_TIME_BASE_Q);
+
+    prft->wallclock = rtcp_time + delta_time;
     prft->flags = 24;
     return 0;
 }