Message ID | 20210325044636.360896-1-alokpr@gmail.com |
---|---|
State | Accepted |
Commit | 62f486e7930c5f1c8e4bfe342eb6a2bbd8cfd177 |
Headers | show |
Series | [FFmpeg-devel] avformat/rtpdec: Fix prft wallclock time. | expand |
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 |
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
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".
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". > >
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.
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 --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; }