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...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
