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

John Stebbins jstebbins at jetheaddev.com
Mon May 4 15:26:08 EEST 2020


On Mon, 2020-05-04 at 08:29 +0200, Andreas Rheinhardt wrote:
> John Stebbins:
> > On Sat, 2020-05-02 at 22:30 +0200, Andreas Rheinhardt wrote:
> > > John Stebbins:
> > > > Required to remux m2ts to mkv
> > > > ---
> > > >  Changelog                        |   1 +
> > > >  doc/bitstream_filters.texi       |   8 ++
> > > >  libavcodec/Makefile              |   1 +
> > > >  libavcodec/bitstream_filters.c   |   1 +
> > > >  libavcodec/pgs_frame_merge_bsf.c | 168
> > > > +++++++++++++++++++++++++++++++
> > > >  5 files changed, 179 insertions(+)
> > > >  create mode 100644 libavcodec/pgs_frame_merge_bsf.c
> > > > 
> > > > diff --git a/Changelog b/Changelog
> > > > index d9fcd8bb0a..fec4867488 100644
> > > > --- a/Changelog
> > > > +++ b/Changelog
> > > > @@ -59,6 +59,7 @@ version <next>:
> > > >  - mv30 decoder
> > > >  - Expanded styling support for 3GPP Timed Text Subtitles
> > > > (movtext)
> > > >  - WebP parser
> > > > +- PGS subtitle frame merge bitstream filter
> > > >  
> > > >  
> > > >  version 4.2:
> > > > diff --git a/doc/bitstream_filters.texi
> > > > b/doc/bitstream_filters.texi
> > > > index 8fe5b3ad75..21ed09986c 100644
> > > > --- a/doc/bitstream_filters.texi
> > > > +++ b/doc/bitstream_filters.texi
> > > > @@ -548,6 +548,14 @@ ffmpeg -i INPUT -c copy -bsf noise[=1]
> > > > output.mkv
> > > >  @section null
> > > >  This bitstream filter passes the packets through unchanged.
> > > >  
> > > > + at section pgs_frame_merge
> > > > +
> > > > +Merge a sequence of PGS Subtitle segments ending with an "end
> > > > of
> > > > display set"
> > > > +segment into a single packet.
> > > > +
> > > > +This is required by some containers that support PGS subtitles
> > > > +(muxer @code{matroska}).
> > > > +
> > > >  @section prores_metadata
> > > >  
> > > >  Modify color property metadata embedded in prores stream.
> > > > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > > > index 88944d9a3a..b630de21bc 100644
> > > > --- a/libavcodec/Makefile
> > > > +++ b/libavcodec/Makefile
> > > > @@ -1115,6 +1115,7 @@ OBJS-
> > > > $(CONFIG_MP3_HEADER_DECOMPRESS_BSF)  +=
> > > > mp3_header_decompress_bsf.o \
> > > >  OBJS-$(CONFIG_MPEG2_METADATA_BSF)         +=
> > > > mpeg2_metadata_bsf.o
> > > >  OBJS-$(CONFIG_NOISE_BSF)                  += noise_bsf.o
> > > >  OBJS-$(CONFIG_NULL_BSF)                   += null_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
> > > >  OBJS-$(CONFIG_TEXT2MOVSUB_BSF)            += movsub_bsf.o
> > > > diff --git a/libavcodec/bitstream_filters.c
> > > > b/libavcodec/bitstream_filters.c
> > > > index 6b5ffe4d70..92619225f0 100644
> > > > --- a/libavcodec/bitstream_filters.c
> > > > +++ b/libavcodec/bitstream_filters.c
> > > > @@ -49,6 +49,7 @@ extern const AVBitStreamFilter
> > > > ff_mpeg4_unpack_bframes_bsf;
> > > >  extern const AVBitStreamFilter ff_mov2textsub_bsf;
> > > >  extern const AVBitStreamFilter ff_noise_bsf;
> > > >  extern const AVBitStreamFilter ff_null_bsf;
> > > > +extern const AVBitStreamFilter ff_pgs_frame_merge_bsf;
> > > >  extern const AVBitStreamFilter ff_prores_metadata_bsf;
> > > >  extern const AVBitStreamFilter ff_remove_extradata_bsf;
> > > >  extern const AVBitStreamFilter ff_text2movsub_bsf;
> > > > diff --git a/libavcodec/pgs_frame_merge_bsf.c
> > > > b/libavcodec/pgs_frame_merge_bsf.c
> > > > new file mode 100644
> > > > index 0000000000..cae5c75655
> > > > --- /dev/null
> > > > +++ b/libavcodec/pgs_frame_merge_bsf.c
> > > > @@ -0,0 +1,168 @@
> > > > +/*
> > > > + * 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;
> > > > +    int presentation_found;
> > > > +} PGSMergeContext;
> > > > +
> > > > +static void frame_merge_flush(AVBSFContext *bsf)
> > > > +{
> > > > +    PGSMergeContext *ctx = bsf->priv_data;
> > > > +
> > > > +    av_packet_unref(ctx->in);
> > > > +    av_packet_unref(ctx->buffer_pkt);
> > > 
> > > flush should reset ctx->presentation_found, shouldn't it?
> > 
> > Ok
> > 
> > > > +}
> > > > +
> > > > +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;
> > > > +
> > > > +    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 + 3 <= in->size) {
> > > > +        uint8_t segment_type;
> > > > +        int segment_len;
> > > > +
> > > > +        segment_type = in->data[i];
> > > > +        segment_len  = AV_RB16(in->data + i + 1) + 3;
> > > > +        if (i + segment_len > in->size) // Invalid data
> > > 
> > > Possible overflow. Use an unsigned as segment_len to avoid it.
> > 
> > Not trying to be pithy, just making sure I understand exactly what
> > you
> > mean.
> > 
> > If I'm understanding right, you wish to protect against the case
> > that
> > in->size is > 2^31 - 2^16 and there are more than 32768 segments
> > found
> > and the last segment length is > 2^31 - in->size? 
> > 
> > Those are the only circumstances I see an overflow happening, so am
> > I
> > understanding correctly?
> > 
> Yes, this can happen if in->size is > INT_MAX - (2^16 + 3).
> The loop condition could also be changed to i <= in->size - 3.
> 

Thanks.  I would also have to similarly modify the test inside the loop
if (i + segment_len > in->size) => if (i > in->size - segment_len)

I think the code reads better if I just make i unsigned.  And this
works as well for pgs_frame_split_bsf.


More information about the ffmpeg-devel mailing list