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

Required to remux m2ts to mkv
---
 libavcodec/Makefile              |   1 +
 libavcodec/bitstream_filters.c   |   1 +
 libavcodec/pgs_frame_merge_bsf.c | 152 +++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 libavcodec/pgs_frame_merge_bsf.c

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@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,
> +};
>

On Thu, 2020-04-16 at 22:39 +0200, Andreas Rheinhardt wrote:
> 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.

Will do.  I didn't notice they were sorted.

> >  
> >  # 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.

ditto

> 
> >  
> >  #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@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.

Oops, I did not intend to read past the end of the packet.  I'll fix
that.

> 2. Both segment_type and segment_len could be made local variable of
> this loop.

Will do

> 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;
> 

Will fix.  I originaly did not add the "+ 3", did it as a separate
calculation later.  When consolidating, I forgot about the type.

> 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.)

Will do

> 
> > +            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
> 

It would be invalid for there to be a second presentation segment.  But
I'll make sure this only happens once.  This should survive invalid
data ;) Thanks.

> > +            pkt->pts = in->pts;
> > +            pkt->dts = in->dts;
> 
> 7. Completely unnecessary as av_packet_copy_props() already copies
> these
> fields.

I knew that, forgot to delete :(

> 
> > +        }
> > +        i += segment_len;
> 
> 8. There is a potential for overflow here.

Do you mean overflowing int "i"?  Not before "i" is > in->size I think.

> 
> > +    }
> > +    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.
> 

Again I "knew" that and forgot to handle.  Will do

> > +    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?)

The copy_props was a bad idea.  But it's there to make sure the next
output packet has some sort of timestamp attached to it if there
happens to not be a presentation segment in the next sequence.  But
*every* sequence of PGS segments should have a single presentation
segment.  So for a valid stream, it's not necessary.  

A sequence of segments either defines a subtitle to be displayed (and
therefore a keyframe) or it signals the end of the previous subtitle. 
The "end" signal is represented by the presentation segment having no
object references.  I could pretty easily look that up since the object
count is a fixed offset into the presentation segment.  So I'll go
ahead and add that

Thanks for the review :)

> 
> > +        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,
> > +};
> > 
>