[FFmpeg-devel] [PATCH] lavc: Add filter_units bitstream filter

Eran Kornblau eran.kornblau at kaltura.com
Sun Mar 25 15:40:22 EEST 2018


> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Mark Thompson
> Sent: Sunday, March 11, 2018 8:38 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc: Add filter_units bitstream filter
> 
> On 08/03/18 04:01, James Almer wrote:
> > On 3/6/2018 3:49 PM, Mark Thompson wrote:
> >> This can remove units with types in or not in a given set from a stream.
> >> For example, it can be used to remove all non-VCL NAL units from an 
> >> H.264 or
> >> H.265 stream.
> >> ---
> >> On 06/03/18 17:27, Hendrik Leppkes wrote:
> >>> On Tue, Mar 6, 2018 at 3:51 PM, Eran Kornblau <eran.kornblau at kaltura.com> wrote:
> >>>> Hi all,
> >>>>
> >>>> The attached patch adds a parameter that enables the user to choose which AVC/HEVC NAL units to include in the output.
> >>>> The parameter is supplied as a bitmask in order to keep things simple.
> >>>>
> >>>> A short background on why we need it - in our transcoding process, 
> >>>> we partition the video in chunks, the chunks are transcoded in 
> >>>> parallel and packaged in MPEG-TS container. The transcoded TS 
> >>>> chunks are then concatenated and packaged in MP4. These MP4 files are later repackaged on-the-fly to various protocols (HLS/DASH etc.) using our JIT packager.
> >>>> For performance reasons (can get into more detail if anyone's 
> >>>> interested...), when packaging the MP4 to DASH/CENC, we configure the packager to assume that each AVC frame contains exactly one NAL unit.
> >>>> The problem is that the transition through MPEG-TS adds additional 
> >>>> NAL units (NAL AUD before each frame + SPS/PPS before each key frame), and this assumption fails.
> >>>> Using the attached patch we can pass '-nal_types_mask 0x3e' which will make ffmpeg output only VCL NALs in the stream.
> >>>>
> >>>
> >>> Having such logic in one single muxer is not something we really 
> >>> like around here. Next time someone needs something similar for 
> >>> another codec, you're stuck re-implementing it.
> >>>
> >>> To achieve the same effect, Mark Thompson quickly wipped up a 
> >>> Bitstream Filter using his CBS framework which achieves the same 
> >>> result. He'll be sending that patch to the mailing list in a while.
> >> The suggested use-case would be '-bsf:v filter_units=pass_types=1-5' for H.264 or '-bsf:v filter_units=pass_types=0-31' for H.265.
> >>
> >> (Also note that filters already exist for some individual parts of 
> >> this: h264_metadata can remove AUDs, extract_extradata can remove 
> >> parameter sets.)
> >>
> >> - Mark
> >>
> >>
> >>  libavcodec/Makefile            |   1 +
> >>  libavcodec/bitstream_filters.c |   1 +
> >>  libavcodec/filter_units_bsf.c  | 250 
> >> +++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 252 insertions(+)
> >>  create mode 100644 libavcodec/filter_units_bsf.c
> >>
> > 
> > Can you write some minimal documentation with the two above examples?
> 
> Done.
> 
> >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 
> >> b496f0d..b99bdce 100644
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -1037,6 +1037,7 @@ OBJS-$(CONFIG_DUMP_EXTRADATA_BSF)         += dump_extradata_bsf.o
> >>  OBJS-$(CONFIG_DCA_CORE_BSF)               += dca_core_bsf.o
> >>  OBJS-$(CONFIG_EXTRACT_EXTRADATA_BSF)      += extract_extradata_bsf.o    \
> >>                                               h2645_parse.o
> >> +OBJS-$(CONFIG_FILTER_UNITS_BSF)           += filter_units_bsf.o
> >>  OBJS-$(CONFIG_H264_METADATA_BSF)          += h264_metadata_bsf.o
> >>  OBJS-$(CONFIG_H264_MP4TOANNEXB_BSF)       += h264_mp4toannexb_bsf.o
> >>  OBJS-$(CONFIG_H264_REDUNDANT_PPS_BSF)     += h264_redundant_pps_bsf.o
> >> diff --git a/libavcodec/bitstream_filters.c 
> >> b/libavcodec/bitstream_filters.c index 338ef82..e1dc198 100644
> >> --- a/libavcodec/bitstream_filters.c
> >> +++ b/libavcodec/bitstream_filters.c
> >> @@ -29,6 +29,7 @@ extern const AVBitStreamFilter ff_chomp_bsf;  
> >> extern const AVBitStreamFilter ff_dump_extradata_bsf;  extern const 
> >> AVBitStreamFilter ff_dca_core_bsf;  extern const AVBitStreamFilter 
> >> ff_extract_extradata_bsf;
> >> +extern const AVBitStreamFilter ff_filter_units_bsf;
> >>  extern const AVBitStreamFilter ff_h264_metadata_bsf;  extern const 
> >> AVBitStreamFilter ff_h264_mp4toannexb_bsf;  extern const 
> >> AVBitStreamFilter ff_h264_redundant_pps_bsf; diff --git 
> >> a/libavcodec/filter_units_bsf.c b/libavcodec/filter_units_bsf.c new 
> >> file mode 100644 index 0000000..3126f17
> >> --- /dev/null
> >> +++ b/libavcodec/filter_units_bsf.c
> >> @@ -0,0 +1,250 @@
> >> +/*
> >> + * 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  */
> >> +
> >> +#include <stdlib.h>
> >> +
> >> +#include "libavutil/common.h"
> >> +#include "libavutil/opt.h"
> >> +
> >> +#include "bsf.h"
> >> +#include "cbs.h"
> >> +
> >> +
> >> +typedef struct FilterUnitsContext {
> >> +    const AVClass *class;
> >> +
> >> +    CodedBitstreamContext *cbc;
> >> +
> >> +    const char *pass_types;
> >> +    const char *remove_types;
> >> +
> >> +    int remove;
> >> +    CodedBitstreamUnitType *type_list;
> >> +    int nb_types;
> >> +} FilterUnitsContext;
> >> +
> >> +
> >> +static int filter_units_make_type_list(const char *list_string,
> >> +                                       CodedBitstreamUnitType **type_list,
> >> +                                       int *nb_types) {
> >> +    CodedBitstreamUnitType *list = NULL;
> >> +    int pass, count;
> >> +
> >> +    for (pass = 1; pass <= 2; pass++) {
> >> +        long value, range_start, range_end;
> >> +        const char *str;
> >> +        char *value_end;
> >> +
> >> +        count = 0;
> >> +        for (str = list_string; *str;) {
> >> +            value = strtol(str, &value_end, 0);
> >> +            if (str == value_end)
> >> +                goto invalid;
> >> +            str = (const char *)value_end;
> >> +            if (*str == '-') {
> >> +                ++str;
> >> +                range_start = value;
> >> +                range_end   = strtol(str, &value_end, 0);
> >> +                if (str == value_end)
> >> +                    goto invalid;
> >> +
> >> +                for (value = range_start; value < range_end; value++) {
> >> +                    if (pass == 2)
> >> +                        list[count] = value;
> >> +                    ++count;
> >> +                }
> >> +            } else {
> >> +                if (pass == 2)
> >> +                    list[count] = value;
> >> +                ++count;
> >> +            }
> >> +            if (*str == '|')
> >> +                ++str;
> >> +        }
> >> +        if (pass == 1) {
> >> +            list = av_malloc_array(count, sizeof(*list));
> >> +            if (!list)
> >> +                return AVERROR(ENOMEM);
> >> +        }
> >> +    }
> >> +
> >> +    *type_list = list;
> >> +    *nb_types  = count;
> >> +    return 0;
> >> +
> >> +invalid:
> >> +    av_freep(&list);
> >> +    return AVERROR(EINVAL);
> >> +}
> >> +
> >> +static int filter_units_filter(AVBSFContext *bsf, AVPacket *out) {
> >> +    FilterUnitsContext *ctx = bsf->priv_data;
> >> +    AVPacket *in = NULL;
> >> +    CodedBitstreamFragment au;
> >> +    int err, i, j;
> >> +
> >> +    while (1) {
> >> +        err = ff_bsf_get_packet(bsf, &in);
> >> +        if (err < 0)
> >> +            return err;
> >> +
> >> +        err = ff_cbs_read_packet(ctx->cbc, &au, in);
> >> +        if (err < 0) {
> >> +            av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n");
> >> +            goto fail;
> >> +        }
> >> +
> >> +        for (i = 0; i < au.nb_units; i++) {
> >> +            for (j = 0; j < ctx->nb_types; j++) {
> >> +                if (au.units[i].type == ctx->type_list[j])
> >> +                    break;
> >> +            }
> >> +            if (ctx->remove ? j <  ctx->nb_types
> >> +                            : j >= ctx->nb_types) {
> >> +                ff_cbs_delete_unit(ctx->cbc, &au, i);
> >> +                --i;
> >> +            }
> >> +        }
> >> +
> >> +        if (au.nb_units > 0)
> >> +            break;
> >> +
> >> +        // Don't return packets with nothing in them.
> >> +        av_packet_free(&in);
> >> +        ff_cbs_fragment_uninit(ctx->cbc, &au);
> >> +    }
> >> +
> >> +    err = ff_cbs_write_packet(ctx->cbc, out, &au);
> >> +    if (err < 0) {
> >> +        av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
> >> +        goto fail;
> >> +    }
> >> +
> >> +    err = av_packet_copy_props(out, in);
> >> +    if (err < 0)
> >> +        goto fail;
> >> +
> >> +    err = 0;
> > 
> > Unnecessary. av_packet_copy_props() already sets it to zero on success.
> 
> I'm not entirely sure it's clearer, but ok.
> 
> >> +fail:
> >> +    ff_cbs_fragment_uninit(ctx->cbc, &au);
> >> +
> >> +    av_packet_free(&in);
> >> +
> >> +    return err;
> >> +}
> >> +
> >> +static int filter_units_init(AVBSFContext *bsf) {
> >> +    FilterUnitsContext *ctx = bsf->priv_data;
> >> +    int err;
> >> +
> >> +    if (!(!ctx->pass_types ^ !ctx->remove_types)) {
> >> +        av_log(bsf, AV_LOG_ERROR, "Exactly one of pass_types or "
> >> +               "remove_types is required.\n");
> >> +        return AVERROR(EINVAL);
> >> +    }
> >> +
> >> +    if (ctx->pass_types) {
> >> +        ctx->remove = 0;
> >> +        err = filter_units_make_type_list(ctx->pass_types,
> >> +                                          &ctx->type_list, &ctx->nb_types);
> >> +        if (err < 0) {
> >> +            av_log(bsf, AV_LOG_ERROR, "Failed to parse pass_types.\n");
> >> +            return AVERROR(EINVAL);
> > 
> > return err. It can also be ENOMEM.
> 
> Fixed.
> 
> >> +        }
> >> +    } else {
> >> +        ctx->remove = 1;
> >> +        err = filter_units_make_type_list(ctx->remove_types,
> >> +                                          &ctx->type_list, &ctx->nb_types);
> >> +        if (err < 0) {
> >> +            av_log(bsf, AV_LOG_ERROR, "Failed to parse remove_types.\n");
> >> +            return AVERROR(EINVAL);
> > 
> > Same.
> 
> Same.
> 
> >> +        }
> >> +    }
> >> +
> >> +    err = ff_cbs_init(&ctx->cbc, bsf->par_in->codec_id, bsf);
> >> +    if (err < 0)
> >> +        return err;
> >> +
> >> +    // Don't actually decompose anything, we only want the unit data.
> >> +    ctx->cbc->decompose_unit_types    = ctx->type_list;
> >> +    ctx->cbc->nb_decompose_unit_types = 0;
> >> +
> >> +    if (bsf->par_in->extradata) {
> >> +        CodedBitstreamFragment ps;
> >> +
> >> +        err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in);
> >> +        if (err < 0) {
> >> +            av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
> >> +        } else {
> >> +            err = ff_cbs_write_extradata(ctx->cbc, bsf->par_out, &ps);
> >> +            if (err < 0)
> >> +                av_log(bsf, AV_LOG_ERROR, "Failed to write extradata.\n");
> >> +        }
> >> +
> >> +        ff_cbs_fragment_uninit(ctx->cbc, &ps);
> >> +    } else {
> >> +        err = 0;
> > 
> > Also Unnecessary. It's already zero from ff_cbs_init() above.
> 
> Right.
> 
> >> +    }
> >> +
> >> +    return err;
> >> +}
> >> +
> >> +static void filter_units_close(AVBSFContext *bsf) {
> >> +    FilterUnitsContext *ctx = bsf->priv_data;
> >> +
> >> +    av_freep(&ctx->type_list);
> >> +
> >> +    ff_cbs_close(&ctx->cbc);
> >> +}
> >> +
> >> +#define OFFSET(x) offsetof(FilterUnitsContext, x) static const 
> >> +AVOption filter_units_options[] = {
> >> +    { "pass_types",   "List of unit types to pass through the filter.",
> >> +        OFFSET(pass_types),   AV_OPT_TYPE_STRING, { .str = NULL } },
> >> +    { "remove_types", "List of unit types to remove in the filter.",
> >> +        OFFSET(remove_types), AV_OPT_TYPE_STRING, { .str = NULL } },
> >> +
> >> +    { NULL }
> >> +};
> >> +
> >> +static const AVClass filter_units_class = {
> >> +    .class_name = "filter_units_bsf",
> > 
> > Nit: Maybe just "filter_units".
> 
> Sure.
> 
> >> +    .item_name  = av_default_item_name,
> >> +    .option     = filter_units_options,
> >> +    .version    = LIBAVUTIL_VERSION_INT,
> >> +};
> >> +
> >> +static const enum AVCodecID filter_units_codec_ids[] = {
> >> +    AV_CODEC_ID_H264,
> >> +    AV_CODEC_ID_HEVC,
> >> +    AV_CODEC_ID_NONE,
> >> +};
> >> +
> >> +const AVBitStreamFilter ff_filter_units_bsf = {
> >> +    .name           = "filter_units",
> >> +    .priv_data_size = sizeof(FilterUnitsContext),
> >> +    .priv_class     = &filter_units_class,
> >> +    .init           = &filter_units_init,
> >> +    .close          = &filter_units_close,
> >> +    .filter         = &filter_units_filter,
> > 
> > The & is unnecessary for at least the last three.
> > 
> >> +    .codec_ids      = filter_units_codec_ids,
> >> +};
> >>
> > 
> > What about a "passthrough" case, when neither pass_types or 
> > remove_types are provided, instead of erroring out? It's the standard 
> > behavior among most if not all bsfs (h264_mp4toannexb, vp9_superframe, aac_adtstoasc, etc).
> > Imagine a batch process calling ffmpeg with this filter and each time 
> > with different input files and arguments. If for some file there's no 
> > need to remove anything, erroring out would break such a process.
> > 
> > Also, doing av_packet_move_ref(out, in) for this, like in other 
> > filters, will be much faster than going through the entire cbs process 
> > even if the result is a noop.
> 
> Ok, added.
> 
> (Note that move_ref still isn't possible in general for packets which appear to be unmodified, because the encapsulation might be different - e.g. currently CBS always writes Annex B for H.26[45], so if the input is [AH]VCC it still needs to be rewritten.  That's also why the extradata gets rewritten rather than just read.)
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

Thanks Mark Thompson! (and Hendrik Leppkes :)) 
We tested this patch now and it solves our original issue, would love to see this getting merged.

Eran



More information about the ffmpeg-devel mailing list