[FFmpeg-devel] [PATCH 4/5] bsf: Add new bitstream filter to set pts_adjustment when reclocking

[Andreas Rheinhardt]

Devin Heitmueller:
> Because SCTE-35 messages are represented in TS streams as sections
> rather than PES packets, we cannot rely on ffmpeg's standard
> mechanisms to adjust PTS values if reclocking the stream.
> This filter will leverage the SCTE-35 pts_adjust field to
> compensate for any change in the PTS values in the stream.
> 
> See SCTE-35 2019 Sec 9.6 for information about the use of
> the pts_adjust field.
> 
> This filter also tweaks the mpegtsenc mux to automatically
> add it so the user doesn't have to include it manually.
> 
> Signed-off-by: Devin Heitmueller <dheitmueller at ltnglobal.com>
> ---
>  libavcodec/Makefile              |   1 +
>  libavcodec/bitstream_filters.c   |   1 +
>  libavcodec/scte35ptsadjust_bsf.c | 114 +++++++++++++++++++++++++++++++++++++++
>  libavformat/mpegtsenc.c          |   2 +
>  4 files changed, 118 insertions(+)
>  create mode 100644 libavcodec/scte35ptsadjust_bsf.c
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 0ce8fe5..6944c82 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -1250,6 +1250,7 @@ OBJS-$(CONFIG_PCM_RECHUNK_BSF)            += pcm_rechunk_bsf.o
>  OBJS-$(CONFIG_PGS_FRAME_MERGE_BSF)        += pgs_frame_merge_bsf.o
>  OBJS-$(CONFIG_PRORES_METADATA_BSF)        += prores_metadata_bsf.o
>  OBJS-$(CONFIG_REMOVE_EXTRADATA_BSF)       += remove_extradata_bsf.o av1_parse.o
> +OBJS-$(CONFIG_SCTE35PTSADJUST_BSF)        += scte35ptsadjust_bsf.o
>  OBJS-$(CONFIG_SETTS_BSF)                  += setts_bsf.o
>  OBJS-$(CONFIG_TEXT2MOVSUB_BSF)            += movsub_bsf.o
>  OBJS-$(CONFIG_TRACE_HEADERS_BSF)          += trace_headers_bsf.o
> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
> index 7512fcc..d30dfbd 100644
> --- a/libavcodec/bitstream_filters.c
> +++ b/libavcodec/bitstream_filters.c
> @@ -57,6 +57,7 @@ extern const FFBitStreamFilter ff_pcm_rechunk_bsf;
>  extern const FFBitStreamFilter ff_pgs_frame_merge_bsf;
>  extern const FFBitStreamFilter ff_prores_metadata_bsf;
>  extern const FFBitStreamFilter ff_remove_extradata_bsf;
> +extern const FFBitStreamFilter ff_scte35ptsadjust_bsf;
>  extern const FFBitStreamFilter ff_setts_bsf;
>  extern const FFBitStreamFilter ff_text2movsub_bsf;
>  extern const FFBitStreamFilter ff_trace_headers_bsf;
> diff --git a/libavcodec/scte35ptsadjust_bsf.c b/libavcodec/scte35ptsadjust_bsf.c
> new file mode 100644
> index 0000000..e6e9ec9
> --- /dev/null
> +++ b/libavcodec/scte35ptsadjust_bsf.c
> @@ -0,0 +1,114 @@
> +/*
> + * SCTE-35 PTS fixup bitstream filter
> + * Copyright (c) 2023 LTN Global Communications, Inc.
> + *
> + * Author: Devin Heitmueller <dheitmueller at ltnglobal.com>
> + *
> + * Because SCTE-35 messages are represented in TS streams as sections
> + * rather than PES packets, we cannot rely on ffmpeg's standard
> + * mechanisms to adjust PTS values if reclocking the stream.
> + * This filter will leverage the SCTE-35 pts_adjust field to
> + * compensate for any change in the PTS values in the stream.
> + *
> + * See SCTE-35 2019 Sec 9.6 for information about the use of
> + * the pts_adjust field.
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "avcodec.h"

What is that for? The BSF API is separate from the AVCodecContext stuff.

> +#include "bsf.h"
> +#include "bsf_internal.h"
> +
> +static int scte35ptsadjust_filter(AVBSFContext *ctx, AVPacket *out)
> +{
> +    AVPacket *in;
> +    size_t size;

av_packet_get_side_data() can work with NULL for the size pointer.

> +    const int64_t *orig_pts;
> +    int64_t cur_pts_adjust;
> +    int ret = 0;
> +
> +    ret = ff_bsf_get_packet(ctx, &in);
> +    if (ret < 0)
> +        return ret;
> +
> +    /* Retrieve the original PTS, which will be used to calculate the pts_adjust */
> +    orig_pts = (int64_t *) av_packet_get_side_data(in, AV_PKT_DATA_ORIG_PTS, &size);
> +    if (orig_pts == NULL) {
> +        /* No original PTS specified, so just pass the packet through */
> +        av_packet_move_ref(out, in);
> +        av_packet_free(&in);
> +        return 0;
> +    }
> +
> +    av_log(ctx, AV_LOG_DEBUG, "%s pts=%" PRId64 " orig_pts=%" PRId64 "\n", __func__,
> +           in->pts, *orig_pts);
> +
> +    /* The pts_adjust field is logically buf[4]-buf[8] of the payload */
> +    if (in->size < 8)

You are reading data[8], so size should be at least 9.

> +        goto fail;

This will return 0 instead of indicating an error.
(But this is irrelevant, see below.)

> +
> +    /* Extract the current pts_adjust value from the packet */
> +    cur_pts_adjust = ((int64_t) in->data[4] & (int64_t) 0x01 << 32) |
> +                     ((int64_t) in->data[5] << 24) |
> +                     ((int64_t) in->data[6] << 16) |
> +                     ((int64_t) in->data[7] << 8) |
> +                     ((int64_t) in->data[8] );
> +
> +    av_log(ctx, AV_LOG_DEBUG, "%s pts_adjust=%" PRId64 "\n", __func__,
> +           cur_pts_adjust);
> +
> +    /* Compute the new PTS adjust value */
> +    cur_pts_adjust -= *orig_pts;
> +    cur_pts_adjust += in->pts;
> +    cur_pts_adjust &= (int64_t) 0x1ffffffff;
> +
> +    av_log(ctx, AV_LOG_DEBUG, "%s new pts_adjust=%" PRId64 "\n", __func__,
> +           cur_pts_adjust);

We typically don't add __func__, because the actual information where
the log comes from is contained in the logcontext. Furthermore, you
could combine these two av_log().

> +
> +    /* Clone the incoming packet since we need to modify it */
> +    ret = av_new_packet(out, in->size);
> +    if (ret < 0)
> +        goto fail;
> +    ret = av_packet_copy_props(out, in);
> +    if (ret < 0)
> +        goto fail;
> +    memcpy(out->data, in->data, in->size);

Just use av_packet_make_writable() instead of this; use it in
conjunction with ff_bsf_get_packet_ref(), avoiding the second packet. It
also avoids allocations and copies in case the packet's data is already
writable and avoids copying side-data etc.

> +
> +    /* Insert the updated pts_adjust value */
> +    out->data[4] &= 0xfe; /* Preserve top 7 unrelated bits */
> +    out->data[4] |= cur_pts_adjust >> 32;
> +    out->data[5] = cur_pts_adjust >> 24;
> +    out->data[6] = cur_pts_adjust >> 16;
> +    out->data[7] = cur_pts_adjust >> 8;
> +    out->data[8] = cur_pts_adjust;

The last four could be simplified to AV_WB32(out_data[5],
(uint32_t)cur_pts_adjust);

> +
> +fail:
> +    av_packet_free(&in);
> +
> +    return ret;
> +}
> +
> +static const enum AVCodecID codec_ids[] = {
> +    AV_CODEC_ID_SCTE_35, AV_CODEC_ID_NONE,
> +};
> +
> +const FFBitStreamFilter ff_scte35ptsadjust_bsf = {
> +    .p.name         = "scte35ptsadjust",
> +    .p.codec_ids    = codec_ids,
> +    .filter         = scte35ptsadjust_filter,
> +};
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index c6cd1fd..48d7833 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -2337,6 +2337,8 @@ static int mpegts_check_bitstream(AVFormatContext *s, AVStream *st,
>                                (st->codecpar->extradata_size > 0 &&
>                                 st->codecpar->extradata[0] == 1)))
>              ret = ff_stream_add_bitstream_filter(st, "hevc_mp4toannexb", NULL);
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_SCTE_35) {
> +        ret = ff_stream_add_bitstream_filter(st, "scte35ptsadjust", NULL);
>      }
>  
>      return ret;