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

[John Stebbins]

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

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