[FFmpeg-devel] [PATCH 3/4] lavc/pgs_frame_split_bsf: add bsf to split PGS segments
John Stebbins
jstebbins at jetheaddev.com
Sun May 3 19:33:06 EEST 2020
On Sat, 2020-05-02 at 23:05 +0200, Andreas Rheinhardt wrote:
> John Stebbins:
> > Requried to remux mkv to m2ts
> > ---
> > Changelog | 1 +
> > doc/bitstream_filters.texi | 8 ++
> > libavcodec/Makefile | 1 +
> > libavcodec/bitstream_filters.c | 1 +
> > libavcodec/pgs_frame_split_bsf.c | 176
> > +++++++++++++++++++++++++++++++
> > 5 files changed, 187 insertions(+)
> > create mode 100644 libavcodec/pgs_frame_split_bsf.c
> >
> > diff --git a/Changelog b/Changelog
> > index fec4867488..372bb4694c 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -60,6 +60,7 @@ version <next>:
> > - Expanded styling support for 3GPP Timed Text Subtitles (movtext)
> > - WebP parser
> > - PGS subtitle frame merge bitstream filter
> > +- PGS subtitle frame split bitstream filter
> >
> >
> > version 4.2:
> > diff --git a/doc/bitstream_filters.texi
> > b/doc/bitstream_filters.texi
> > index 21ed09986c..643c419076 100644
> > --- a/doc/bitstream_filters.texi
> > +++ b/doc/bitstream_filters.texi
> > @@ -556,6 +556,14 @@ segment into a single packet.
> > This is required by some containers that support PGS subtitles
> > (muxer @code{matroska}).
> >
> > + at section pgs_frame_split
> > +
> > +Split a packet containing a sequence of PGS Subtitle segments into
> > separate
> > +packets each containing a single segment.
> > +
> > +This is required by some streaming formats, typically the MPEG-2
> > +transport stream format (muxer @code{mpegts}).
>
> Is there a reason you don't add this bsf automatically for ts?
Yes, it is dependent on:
http://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/261598.html
I have a patch ready for when the above gets approved and committed
>
> > +
> > @section prores_metadata
> >
> > Modify color property metadata embedded in prores stream.
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index b630de21bc..0e5f2db192 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -1116,6 +1116,7 @@ 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_PGS_FRAME_SPLIT_BSF) += pgs_frame_split_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 92619225f0..02e72c9ca5 100644
> > --- a/libavcodec/bitstream_filters.c
> > +++ b/libavcodec/bitstream_filters.c
> > @@ -50,6 +50,7 @@ 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_pgs_frame_split_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_split_bsf.c
> > b/libavcodec/pgs_frame_split_bsf.c
> > new file mode 100644
> > index 0000000000..c983f6acf5
> > --- /dev/null
> > +++ b/libavcodec/pgs_frame_split_bsf.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * 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 splits PGS subtitle packets into packets
> > containing
> > + * just one segment.
> > + *
> > + * Packets already containing only one segment 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 PGSSplitContext {
> > + AVPacket *in;
> > +} PGSSplitContext;
> > +
> > +static int frame_split_filter(AVBSFContext *bsf, AVPacket *out)
> > +{
> > + PGSSplitContext *ctx = bsf->priv_data;
> > + AVPacket *in = ctx->in;
> > + int i, ret;
> > + uint8_t segment_type;
> > + uint16_t segment_len;
> > + int split = !!in->data;
> > +
> > + 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
> > + i = 0;
> > + while (i < in->size) {
> > + segment_type = in->data[i];
> > + segment_len = AV_RB16(in->data + i + 1) + 3;
>
> uint16_t is not enough for segment_len. Also you are possibly
> overreading here.
>
Ugh, originally I didn't add the "+ 3" and then forgot about the type
Will fix
> > + i += segment_len;
>
> Possible overflow.
>
Similar to my comment in the pgs merge review. The conditions of the
overflow are pretty extreme. I.e. A very large input packet and more
than 32768 segments in the packet and the last segment size > 2^31 -
in->size.
I could make i unsigned similar to your suggestion for merge. Just
making sure I'm not missing something else. There you suggested
makeing segment_len unisigned, but making i unsigned works just as well
in merge.
> > + }
> > + if (i != in->size) {
> > + av_log(bsf, AV_LOG_WARNING, "Failed to parse PGS
> > segments.\n");
>
> If you output invalid data, you need to set the AV_PKT_FLAG_CORRUPT
> flag.
>
> > + av_packet_move_ref(out, in);
> > + return 0;
> > + }
> > +
> > + segment_type = in->data[0];
> > + segment_len = AV_RB16(in->data + 1) + 3;
> > + if (split || segment_len < in->size) { // Split
> > + int64_t ts_delta, dts;
> > +
> > + ret = av_packet_ref(out, in);
> > + if (ret < 0)
> > + goto fail;
> > + out->size = segment_len;
> > + in->data += segment_len;
> > + in->size -= segment_len;
> > +
> > + // Update PTS/DTS, Stagger timestamps by 3ms
>
> Where does this number come from?
There is no spec beyond A must be > B which is > C type wording and
some segments are specified to have no DTS. I looked at some real BD
data to determine a good nominal value.
>
> > + ts_delta = 270; // Assume 90khz tick if no time_base_in
> > + if (bsf->time_base_in.num && bsf->time_base_in.den)
> > + ts_delta = bsf->time_base_in.den / bsf-
> > >time_base_in.num / 333;
> > + if (ts_delta == 0)
> > + ts_delta = 1;
> > +
> > + // Compute DTS, it's the same for all packets that carry
> > it
> > + // Assuming input PTS/DTS come from original
> > presentation_segment
> > + if (in->dts <= in->pts - 2 * ts_delta &&
> > + in->dts != AV_NOPTS_VALUE)
> > + dts = in->dts;
> > + else if (in->pts != AV_NOPTS_VALUE)
> > + dts = in->pts - 2 * ts_delta;
> > +
> > + switch (segment_type) {
> > + case DISPLAY_SEGMENT:
> > + if (in->pts != AV_NOPTS_VALUE)
> > + out->pts = in->pts - ts_delta;
> > + out->dts = AV_NOPTS_VALUE;
> > + break;
> > + case PALETTE_SEGMENT:
> > + if (in->pts != AV_NOPTS_VALUE)
> > + out->pts = in->pts - 2 * ts_delta;
> > + out->dts = AV_NOPTS_VALUE;
> > + break;
> > + case PRESENTATION_SEGMENT:
> > + out->pts = in->pts;
> > + out->dts = dts;
> > + break;
> > + case WINDOW_SEGMENT:
> > + case OBJECT_SEGMENT:
> > + default:
> > + if (in->pts != AV_NOPTS_VALUE)
> > + out->pts = in->pts - ts_delta;
> > + out->dts = dts;
> > + break;
> > + }
> > + } else
> > + av_packet_move_ref(out, in);
> > +
> > + return 0;
> > +
> > +fail:
> > + av_packet_unref(in);
> > +
> > + return ret;
> > +}
> > +
> > +static int frame_split_init(AVBSFContext *bsf)
> > +{
> > + PGSSplitContext *ctx = bsf->priv_data;
> > +
> > + ctx->in = av_packet_alloc();
> > + if (!ctx->in)
> > + return AVERROR(ENOMEM);
> > +
> > + return 0;
> > +}
> > +
> > +static void frame_split_flush(AVBSFContext *bsf)
> > +{
> > + PGSSplitContext *ctx = bsf->priv_data;
> > +
> > + av_packet_unref(ctx->in);
> > +}
> > +
> > +static void frame_split_close(AVBSFContext *bsf)
> > +{
> > + PGSSplitContext *ctx = bsf->priv_data;
> > +
> > + av_packet_free(&ctx->in);
> > +}
> > +
> > +static const enum AVCodecID frame_split_codec_ids[] = {
> > + AV_CODEC_ID_HDMV_PGS_SUBTITLE, AV_CODEC_ID_NONE,
> > +};
> > +
> > +const AVBitStreamFilter ff_pgs_frame_split_bsf = {
> > + .name = "pgs_frame_split",
> > + .priv_data_size = sizeof(PGSSplitContext),
> > + .init = frame_split_init,
> > + .flush = frame_split_flush,
> > + .close = frame_split_close,
> > + .filter = frame_split_filter,
> > + .codec_ids = frame_split_codec_ids,
> > +};
> >
>
More information about the ffmpeg-devel
mailing list