diff mbox series

[FFmpeg-devel,v2,4/5] mpegtsenc: Don't periodically announce PCR on SCTE-35 streams

Message ID 1688408109-14381-5-git-send-email-dheitmueller@ltnglobal.com
State New
Headers show
Series Add passthrough support for SCTE-35 | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to apply patch

Commit Message

Devin Heitmueller July 3, 2023, 6:15 p.m. UTC
Changes were made between in the last two years to periodically
send PCR-only packets on all PIDs, but for cases where the stream
may send packets very infrequently (like SCTE-35), this results in
extra TR101290 errors because it fails the PCR interval test.

I am not quite sure what the "right" fix should be for this, but
for now just disable all periodic sending of PCR-only packets on
SCTE-35 streams.

Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
---
 libavformat/mpegtsenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marton Balint July 3, 2023, 7:17 p.m. UTC | #1
On Mon, 3 Jul 2023, Devin Heitmueller wrote:

> Changes were made between in the last two years to periodically
> send PCR-only packets on all PIDs, but for cases where the stream
> may send packets very infrequently (like SCTE-35), this results in
> extra TR101290 errors because it fails the PCR interval test.
>
> I am not quite sure what the "right" fix should be for this, but
> for now just disable all periodic sending of PCR-only packets on
> SCTE-35 streams.

Hmm, only one PID per service should generate PCR, and if there is a video 
stream in the service, then that is preferred for PCR generation. An SCTE 
stream should only get selected for PCR if there are no video streams in a 
service. Or are you seeing something else?

And it is also strange that not sending PCR for any stream improves the 
PCR interval test, it should make it worse, because less PCRs are 
provided, no?

Thanks,
Marton

>
> Signed-off-by: Devin Heitmueller <dheitmueller@ltnglobal.com>
> ---
> libavformat/mpegtsenc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index c6cd1fd..ba60582 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -1579,7 +1579,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
>                     int st2_index = i < st->index ? i : (i + 1 == s->nb_streams ? st->index : i + 1);
>                     AVStream *st2 = s->streams[st2_index];
>                     MpegTSWriteStream *ts_st2 = st2->priv_data;
> -                    if (ts_st2->pcr_period) {
> +                    if (ts_st2->pcr_period && st2->codecpar->codec_id != AV_CODEC_ID_SCTE_35) {
>                         if (pcr - ts_st2->last_pcr >= ts_st2->pcr_period) {
>                             ts_st2->last_pcr = FFMAX(pcr - ts_st2->pcr_period, ts_st2->last_pcr + ts_st2->pcr_period);
>                             if (st2 != st) {
> -- 
> 1.8.3.1
>
> _______________________________________________
> 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".
>
Devin Heitmueller July 5, 2023, 3:29 p.m. UTC | #2
Hi Marton,

On Mon, Jul 3, 2023 at 3:20 PM Marton Balint <cus@passwd.hu> wrote:
> Hmm, only one PID per service should generate PCR, and if there is a video
> stream in the service, then that is preferred for PCR generation. An SCTE
> stream should only get selected for PCR if there are no video streams in a
> service. Or are you seeing something else?
>
> And it is also strange that not sending PCR for any stream improves the
> PCR interval test, it should make it worse, because less PCRs are
> provided, no?

You make valid points.  Admittedly this is a patch I wrote a couple of
years ago that has been ported to master, and it's entirely possible
that the description is incorrect.  I definitely made the patch in
response to seeing that I was getting recurring PCR packets on the
SCTE-35 stream (which is contrary to SCTE-35's typical behavior where
you typically only get packets on the PID when there is an actual
trigger).  Also, you generally don't have PCRs delivered on streams
that are composed only of sections (i.e. not containing PES packets),
although I haven't looked to see if that's technically illegal.

I will back out the patch and better document the behavior I am seeing
and update the patch description so it's more clear why it is needed.

Regards,

Devin
diff mbox series

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index c6cd1fd..ba60582 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -1579,7 +1579,7 @@  static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
                     int st2_index = i < st->index ? i : (i + 1 == s->nb_streams ? st->index : i + 1);
                     AVStream *st2 = s->streams[st2_index];
                     MpegTSWriteStream *ts_st2 = st2->priv_data;
-                    if (ts_st2->pcr_period) {
+                    if (ts_st2->pcr_period && st2->codecpar->codec_id != AV_CODEC_ID_SCTE_35) {
                         if (pcr - ts_st2->last_pcr >= ts_st2->pcr_period) {
                             ts_st2->last_pcr = FFMAX(pcr - ts_st2->pcr_period, ts_st2->last_pcr + ts_st2->pcr_period);
                             if (st2 != st) {