[FFmpeg-devel] [PATCH 1/3] lavc/pgs_frame_merge_bsf: add bsf to merge PGS segments

[Andreas Rheinhardt]

John Stebbins:
> Required to remux m2ts to mkv
> ---
>  libavcodec/Makefile              |   1 +
>  libavcodec/bitstream_filters.c   |   1 +
>  libavcodec/pgs_frame_merge_bsf.c | 152 +++++++++++++++++++++++++++++++

Missing version bump.

>  3 files changed, 154 insertions(+)
>  create mode 100644 libavcodec/pgs_frame_merge_bsf.c
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index c1c9a44f2b..13909faabf 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -1119,6 +1119,7 @@ OBJS-$(CONFIG_VP9_METADATA_BSF)           += vp9_metadata_bsf.o
>  OBJS-$(CONFIG_VP9_RAW_REORDER_BSF)        += vp9_raw_reorder_bsf.o
>  OBJS-$(CONFIG_VP9_SUPERFRAME_BSF)         += vp9_superframe_bsf.o
>  OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF)   += vp9_superframe_split_bsf.o
> +OBJS-$(CONFIG_PGS_FRAME_MERGE_BSF)        += pgs_frame_merge_bsf.o

Not sorted alphabetically.
>  
>  # thread libraries
>  OBJS-$(HAVE_LIBC_MSVCRT)               += file_open.o
> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
> index 6b5ffe4d70..138f6dd7ad 100644
> --- a/libavcodec/bitstream_filters.c
> +++ b/libavcodec/bitstream_filters.c
> @@ -58,6 +58,7 @@ extern const AVBitStreamFilter ff_vp9_metadata_bsf;
>  extern const AVBitStreamFilter ff_vp9_raw_reorder_bsf;
>  extern const AVBitStreamFilter ff_vp9_superframe_bsf;
>  extern const AVBitStreamFilter ff_vp9_superframe_split_bsf;
> +extern const AVBitStreamFilter ff_pgs_frame_merge_bsf;

Not sorted alphabetically.

>  
>  #include "libavcodec/bsf_list.c"
>  
> diff --git a/libavcodec/pgs_frame_merge_bsf.c b/libavcodec/pgs_frame_merge_bsf.c
> new file mode 100644
> index 0000000000..4b8061d4e1
> --- /dev/null
> +++ b/libavcodec/pgs_frame_merge_bsf.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright (c) 2020 John Stebbins <jstebbins.hb at gmail.com>
> + *
> + * 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
> + */
> +
> +/**
> + * @file
> + * This bitstream filter merges PGS subtitle packets containing incomplete
> + * set of segments into a single packet
> + *
> + * Packets already containing a complete set of segments will be passed through
> + * unchanged.
> + */
> +
> +#include "avcodec.h"
> +#include "bsf.h"
> +#include "libavutil/intreadwrite.h"
> +
> +enum PGSSegmentType {
> +    PALETTE_SEGMENT      = 0x14,
> +    OBJECT_SEGMENT       = 0x15,
> +    PRESENTATION_SEGMENT = 0x16,
> +    WINDOW_SEGMENT       = 0x17,
> +    DISPLAY_SEGMENT      = 0x80,
> +};
> +
> +typedef struct PGSMergeContext {
> +    AVPacket *buffer_pkt, *in;
> +} PGSMergeContext;
> +
> +static void frame_merge_flush(AVBSFContext *bsf)
> +{
> +    PGSMergeContext *ctx = bsf->priv_data;
> +
> +    av_packet_unref(ctx->in);
> +    av_packet_unref(ctx->buffer_pkt);
> +}
> +
> +static int frame_merge_filter(AVBSFContext *bsf, AVPacket *out)
> +{
> +    PGSMergeContext *ctx = bsf->priv_data;
> +    AVPacket *in = ctx->in, *pkt = ctx->buffer_pkt;
> +    int ret, i, size, pos, display = 0;
> +    uint8_t segment_type;
> +    uint16_t segment_len;
> +
> +    if (!in->data) {
> +        ret = ff_bsf_get_packet_ref(bsf, in);
> +        if (ret < 0)
> +            return ret;
> +    }
> +    if (!in->size) {
> +        av_packet_unref(in);
> +        return AVERROR(EAGAIN);
> +    }
> +
> +    // Validate packet data and find display_end segment
> +    size = in->size;
> +    i = 0;
> +    while (i < in->size) {
> +        segment_type = in->data[i];
> +        segment_len  = AV_RB16(in->data + i + 1) + 3;

1. This code only requires the input packet to be padded as reading
segment_len is not guaranteed to be part of your packet. I have no
problem with that, yet you should add a comment about it.
2. Both segment_type and segment_len could be made local variable of
this loop.
3. The type of segment_len is wrong. There might be an uint16_t overflow
in the calculation. (Or more precisely: the calculation is done after
the usual integer promotions have been applied (i.e. the addition is
performed after promoting to int) and the conversion from int ->
uint16_t might not be lossless.)

> +        if (segment_type == DISPLAY_SEGMENT) {
> +            size = display = i + segment_len;

4. You could increment i before this check (and omit the incrementing
below). It will increase the value of i after this loop in case a
display segment is found, yet it doesn't matter because you also set
display here.
(Moving the check for segments extending beyond the packet end to before
this probably advantageous, too.)

> +            break;
> +        }
> +        if (segment_type == PRESENTATION_SEGMENT) {
> +            av_packet_copy_props(pkt, in);

5. This needs to be checked (there are allocations involved if the input
packet has side data (the mpegts demuxer adds side data of type
AV_PKT_DATA_MPEGTS_STREAM_ID)).
6. I don't see anything that guarantees that pkt is vanilla at this
point. A possibly invalid input packet might contain multiple
presentation segments in which case you would leak side data already
contained in pkt here; or there might multiple presentation segments
before the next display segment comes (don't know if this would be
invalid data, but the way

> +            pkt->pts = in->pts;
> +            pkt->dts = in->dts;

7. Completely unnecessary as av_packet_copy_props() already copies these
fields.

> +        }
> +        i += segment_len;

8. There is a potential for overflow here.

> +    }
> +    if ((!display && i != in->size) || size > in->size) {
> +        av_log(ctx, AV_LOG_WARNING, "Failed to parse PGS segments.\n");
> +        // force output what we have
> +        display = size = in->size;;

9. Double ";".

> +    }
> +
> +    pos = pkt->size;
> +    ret = av_grow_packet(pkt, size);

10. This bitstream filter is supposed to be used as an automatically
inserted bitstream filter upon remuxing to Matroska, so that there is a
good chance that it will get input that already conforms to what
Matroska expects. In other words: It needs a fast passthrough-mode.

> +    if (ret < 0)
> +        goto fail;
> +    memcpy(pkt->data + pos, in->data, size);
> +
> +    if (size == in->size)
> +        av_packet_unref(in);
> +    else {
> +        in->data += size;
> +        in->size -= size;
> +    }
> +
> +    if (display) {
> +        av_packet_move_ref(out, pkt);
> +        av_packet_copy_props(pkt, out);

11. Why are you copying the properties back (again without check)? Can
there be more than one display segment per presentation segment?
(You will probably have noticed that the internals of PGS subtitles
aren't my forte. If I am not mistaken, then not every PGS subtitle
packet is a keyframe. How much parsing would actually be needed to
determine whether the current output packet is a keyframe?)

> +        return 0;
> +    }
> +    return AVERROR(EAGAIN);
> +
> +fail:
> +    frame_merge_flush(bsf);
> +    return ret;
> +}
> +
> +static int frame_merge_init(AVBSFContext *bsf)
> +{
> +    PGSMergeContext *ctx = bsf->priv_data;
> +
> +    ctx->in  = av_packet_alloc();
> +    ctx->buffer_pkt = av_packet_alloc();
> +    if (!ctx->in || !ctx->buffer_pkt)
> +        return AVERROR(ENOMEM);
> +
> +    return 0;
> +}
> +
> +static void frame_merge_close(AVBSFContext *bsf)
> +{
> +    PGSMergeContext *ctx = bsf->priv_data;
> +
> +    av_packet_free(&ctx->in);
> +    av_packet_free(&ctx->buffer_pkt);
> +}
> +
> +static const enum AVCodecID frame_merge_codec_ids[] = {
> +    AV_CODEC_ID_HDMV_PGS_SUBTITLE, AV_CODEC_ID_NONE,
> +};
> +
> +const AVBitStreamFilter ff_pgs_frame_merge_bsf = {
> +    .name           = "pgs_frame_merge",
> +    .priv_data_size = sizeof(PGSMergeContext),
> +    .init           = frame_merge_init,
> +    .flush          = frame_merge_flush,
> +    .close          = frame_merge_close,
> +    .filter         = frame_merge_filter,
> +    .codec_ids      = frame_merge_codec_ids,
> +};
>