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

[Andreas Rheinhardt]

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.

- Andreas