diff mbox series

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

Message ID 20200418142547.21299-2-jstebbins@jetheaddev.com
State Superseded
Headers show
Series [FFmpeg-devel,1/4] lavc/pgs_frame_merge_bsf: add bsf to merge PGS segments | expand

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

John Stebbins April 18, 2020, 2:25 p.m. UTC
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

Comments

Andreas Rheinhardt May 2, 2020, 8:30 p.m. UTC | #1
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.
>  
> +@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@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?

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

> +            break;
> +        if (segment_type == PRESENTATION_SEGMENT && i + 10 < in->size &&
> +            !ctx->presentation_found) {
> +            uint8_t state;
> +            ctx->presentation_found = 1;
> +            ret = av_packet_copy_props(pkt, in);

This will copy the properties even if it eventually turns out that one
can just pass through the packet. Even worse: Imagine the input packet
having side-data. Then pkt contains side-data, yet the code thinks that
this packet is blank. This side data will then leak when the properties
of the next presentation segment are copied.

Furthermore, I would actually have expected the pts of the display
segment to be used. Or are they guaranteed to be identical?

> +            if (ret < 0)
> +                goto fail;
> +            state = in->data[i + 10] & 0xc0;

This amounts to the composition_state of the composition_descriptor(),
doesn't it?

> +            if (state)
> +                pkt->flags |= AV_PKT_FLAG_KEY;

The keyframe flag will be ignored on passthrough. Also, should the flag
be cleared if state == 0?

> +        }
> +        i += segment_len;
> +        if (segment_type == DISPLAY_SEGMENT) {
> +            size = display = i;
> +            break;
> +        }
> +    }
> +    if (display && pkt->size == 0 && size == in->size) { // passthrough
> +        ctx->presentation_found = 0;
> +        av_packet_move_ref(out, in);
> +        return 0;
> +    }
> +    if ((!display && i != in->size) || size > in->size) {

I don't see how size > in->size can ever be true.

> +        av_log(bsf, AV_LOG_WARNING, "Failed to parse PGS segments.\n");
> +        // force output what we have
> +        display = size = in->size;

If you output a corrupt packet, you should set the AV_PKT_FLAG_CORRUPT flag.

> +    }
> +
> +    pos = pkt->size;
> +    ret = av_grow_packet(pkt, size);
> +    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) {
> +        ctx->presentation_found = 0;

I don't see anything in the code that actually enforces that a
presentation segment has been found at this point; the properties of the
output packet would then still be blank. Is this intended? If it is
spec-incompliant, then one should add the AV_PKT_FLAG_CORRUPT flag.

> +        av_packet_move_ref(out, pkt);
> +        return 0;
> +    }
> +    return AVERROR(EAGAIN);
> +
> +fail:
> +    ctx->presentation_found = 0;
> +    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,
> +};
>
Andreas Rheinhardt May 2, 2020, 8:33 p.m. UTC | #2
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.
>  
> +@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@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);
> +}
> +
> +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
> +            break;
> +        if (segment_type == PRESENTATION_SEGMENT && i + 10 < in->size &&
> +            !ctx->presentation_found) {
> +            uint8_t state;
> +            ctx->presentation_found = 1;
> +            ret = av_packet_copy_props(pkt, in);
> +            if (ret < 0)
> +                goto fail;
> +            state = in->data[i + 10] & 0xc0;
> +            if (state)
> +                pkt->flags |= AV_PKT_FLAG_KEY;
> +        }
> +        i += segment_len;
> +        if (segment_type == DISPLAY_SEGMENT) {
> +            size = display = i;
> +            break;
> +        }
> +    }
> +    if (display && pkt->size == 0 && size == in->size) { // passthrough
> +        ctx->presentation_found = 0;
> +        av_packet_move_ref(out, in);
> +        return 0;
> +    }
> +    if ((!display && i != in->size) || size > in->size) {
> +        av_log(bsf, AV_LOG_WARNING, "Failed to parse PGS segments.\n");

You should include libavutil/log.h because of this.

> +        // force output what we have
> +        display = size = in->size;
> +    }
> +
> +    pos = pkt->size;
> +    ret = av_grow_packet(pkt, size);
> +    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) {
> +        ctx->presentation_found = 0;
> +        av_packet_move_ref(out, pkt);
> +        return 0;
> +    }
> +    return AVERROR(EAGAIN);
> +
> +fail:
> +    ctx->presentation_found = 0;
> +    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,
> +};
>
John Stebbins May 3, 2020, 3:45 p.m. UTC | #3
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.
> >  
> > +@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@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?

> 
> > +            break;
> > +        if (segment_type == PRESENTATION_SEGMENT && i + 10 < in-
> > >size &&
> > +            !ctx->presentation_found) {
> > +            uint8_t state;
> > +            ctx->presentation_found = 1;
> > +            ret = av_packet_copy_props(pkt, in);
> 
> This will copy the properties even if it eventually turns out that
> one
> can just pass through the packet. Even worse: Imagine the input
> packet
> having side-data. Then pkt contains side-data, yet the code thinks
> that
> this packet is blank. This side data will then leak when the
> properties
> of the next presentation segment are copied.
> 

Right, thanks.

> Furthermore, I would actually have expected the pts of the display
> segment to be used. Or are they guaranteed to be identical?

pgssubdec gave the "end display set segment" a poor name (i'm guessing
for the sake of brevity) and I've gone an propagated it :(  I'll rename
that enum to make it more clear.

Display time is actually defined by the presentation segment's pts. 

> 
> > +            if (ret < 0)
> > +                goto fail;
> > +            state = in->data[i + 10] & 0xc0;
> 
> This amounts to the composition_state of the
> composition_descriptor(),
> doesn't it?

Yes, it turns out any non-zero value for the composition state says
this in the spec:

"The Display Set contains all elements needed to display the next 
composition." 

Different non-zero states define how the "Epoch" is handled which
basically defines when things come into and go out of scope.

> 
> > +            if (state)
> > +                pkt->flags |= AV_PKT_FLAG_KEY;
> 
> The keyframe flag will be ignored on passthrough. 

Will fix

> Also, should the flag
> be cleared if state == 0?
> 

Yes. Will do.

> > +        }
> > +        i += segment_len;
> > +        if (segment_type == DISPLAY_SEGMENT) {
> > +            size = display = i;
> > +            break;
> > +        }
> > +    }
> > +    if (display && pkt->size == 0 && size == in->size) { //
> > passthrough
> > +        ctx->presentation_found = 0;
> > +        av_packet_move_ref(out, in);
> > +        return 0;
> > +    }
> > +    if ((!display && i != in->size) || size > in->size) {
> 
> I don't see how size > in->size can ever be true.
> 

It could in an earlier iteration.  Will fix.

> > +        av_log(bsf, AV_LOG_WARNING, "Failed to parse PGS
> > segments.\n");
> > +        // force output what we have
> > +        display = size = in->size;
> 
> If you output a corrupt packet, you should set the
> AV_PKT_FLAG_CORRUPT flag.
> 

OK

> > +    }
> > +
> > +    pos = pkt->size;
> > +    ret = av_grow_packet(pkt, size);
> > +    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) {
> > +        ctx->presentation_found = 0;
> 
> I don't see anything in the code that actually enforces that a
> presentation segment has been found at this point; the properties of
> the
> output packet would then still be blank. Is this intended? If it is
> spec-incompliant, then one should add the AV_PKT_FLAG_CORRUPT flag.
> 

Good point.  Will do.

> > +        av_packet_move_ref(out, pkt);
> > +        return 0;
> > +    }
> > +    return AVERROR(EAGAIN);
> > +
> > +fail:
> > +    ctx->presentation_found = 0;
> > +    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,
> > +};
> > 
> 
> 


Also, will add av_log header you comment on in your next email.
Andreas Rheinhardt May 4, 2020, 6:29 a.m. UTC | #4
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.
>>>  
>>> +@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@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
John Stebbins May 4, 2020, 12:26 p.m. UTC | #5
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.
> > > >  
> > > > +@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@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.
diff mbox series

Patch

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.
 
+@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@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);
+}
+
+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
+            break;
+        if (segment_type == PRESENTATION_SEGMENT && i + 10 < in->size &&
+            !ctx->presentation_found) {
+            uint8_t state;
+            ctx->presentation_found = 1;
+            ret = av_packet_copy_props(pkt, in);
+            if (ret < 0)
+                goto fail;
+            state = in->data[i + 10] & 0xc0;
+            if (state)
+                pkt->flags |= AV_PKT_FLAG_KEY;
+        }
+        i += segment_len;
+        if (segment_type == DISPLAY_SEGMENT) {
+            size = display = i;
+            break;
+        }
+    }
+    if (display && pkt->size == 0 && size == in->size) { // passthrough
+        ctx->presentation_found = 0;
+        av_packet_move_ref(out, in);
+        return 0;
+    }
+    if ((!display && i != in->size) || size > in->size) {
+        av_log(bsf, AV_LOG_WARNING, "Failed to parse PGS segments.\n");
+        // force output what we have
+        display = size = in->size;
+    }
+
+    pos = pkt->size;
+    ret = av_grow_packet(pkt, size);
+    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) {
+        ctx->presentation_found = 0;
+        av_packet_move_ref(out, pkt);
+        return 0;
+    }
+    return AVERROR(EAGAIN);
+
+fail:
+    ctx->presentation_found = 0;
+    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,
+};