[FFmpeg-devel] [PATCH 03/20] lavc: Add coded bitstream read/write API

Mark Thompson sw at jkqxz.net
Thu Sep 14 10:31:28 EEST 2017


On 14/09/17 01:42, Michael Niedermayer wrote:
> On Wed, Sep 13, 2017 at 12:43:53AM +0100, Mark Thompson wrote:
>> (cherry picked from commit 18f1706f331bf5dd565774eae680508c8d3a97ad)
>> (cherry picked from commit 44cde38c8acbef7d5250e6d1b52b1020871e093b)
>> ---
>>  configure                 |   1 +
>>  libavcodec/Makefile       |   1 +
>>  libavcodec/cbs.c          | 466 ++++++++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/cbs.h          | 274 +++++++++++++++++++++++++++
>>  libavcodec/cbs_internal.h |  83 +++++++++
>>  5 files changed, 825 insertions(+)
>>  create mode 100644 libavcodec/cbs.c
>>  create mode 100644 libavcodec/cbs.h
>>  create mode 100644 libavcodec/cbs_internal.h
>>
>> diff --git a/configure b/configure
>> index 4dd11efe5e..2e84b1f892 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2112,6 +2112,7 @@ CONFIG_EXTRA="
>>      blockdsp
>>      bswapdsp
>>      cabac
>> +    cbs
>>      dirac_parse
>>      dvprofile
>>      exif
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index 999632cf9e..d9612d68d0 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -59,6 +59,7 @@ OBJS-$(CONFIG_AUDIODSP)                += audiodsp.o
>>  OBJS-$(CONFIG_BLOCKDSP)                += blockdsp.o
>>  OBJS-$(CONFIG_BSWAPDSP)                += bswapdsp.o
>>  OBJS-$(CONFIG_CABAC)                   += cabac.o
>> +OBJS-$(CONFIG_CBS)                     += cbs.o
>>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
>>  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>> new file mode 100644
>> index 0000000000..13bc25728f
>> --- /dev/null
>> +++ b/libavcodec/cbs.c
>> @@ -0,0 +1,466 @@
>> +/*
>> + * 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 <string.h>
>> +
>> +#include "config.h"
>> +
>> +#include "libavutil/avassert.h"
>> +#include "libavutil/common.h"
>> +
>> +#include "cbs.h"
>> +#include "cbs_internal.h"
>> +
>> +
>> +static const CodedBitstreamType *cbs_type_table[] = {
>> +};
>> +
>> +int ff_cbs_init(CodedBitstreamContext *ctx,
>> +                enum AVCodecID codec_id, void *log_ctx)
>> +{
>> +    const CodedBitstreamType *type;
>> +    int i;
>> +
>> +    type = NULL;
>> +    for (i = 0; i < FF_ARRAY_ELEMS(cbs_type_table); i++) {
>> +        if (cbs_type_table[i]->codec_id == codec_id) {
>> +            type = cbs_type_table[i];
>> +            break;
>> +        }
>> +    }
>> +    if (!type)
>> +        return AVERROR(EINVAL);
>> +
>> +    ctx->log_ctx = log_ctx;
>> +    ctx->codec   = type;
>> +
>> +    ctx->priv_data = av_mallocz(ctx->codec->priv_data_size);
>> +    if (!ctx->priv_data)
>> +        return AVERROR(ENOMEM);
>> +
>> +    ctx->decompose_unit_types = NULL;
>> +
>> +    ctx->trace_enable = 0;
>> +    ctx->trace_level  = AV_LOG_TRACE;
>> +
>> +    return 0;
>> +}
>> +
>> +void ff_cbs_close(CodedBitstreamContext *ctx)
>> +{
>> +    if (ctx->codec && ctx->codec->close)
>> +        ctx->codec->close(ctx);
>> +
>> +    av_freep(&ctx->priv_data);
>> +}
>> +
>> +static void cbs_unit_uninit(CodedBitstreamContext *ctx,
>> +                            CodedBitstreamUnit *unit)
>> +{
>> +    if (ctx->codec->free_unit && unit->content && !unit->content_external)
>> +        ctx->codec->free_unit(unit);
>> +
>> +    av_freep(&unit->data);
>> +    unit->data_size = 0;
>> +    unit->data_bit_padding = 0;
>> +}
>> +
>> +void ff_cbs_fragment_uninit(CodedBitstreamContext *ctx,
>> +                            CodedBitstreamFragment *frag)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < frag->nb_units; i++)
>> +        cbs_unit_uninit(ctx, &frag->units[i]);
>> +    av_freep(&frag->units);
>> +    frag->nb_units = 0;
>> +
>> +    av_freep(&frag->data);
>> +    frag->data_size        = 0;
>> +    frag->data_bit_padding = 0;
>> +}
>> +
>> +static int cbs_read_fragment_content(CodedBitstreamContext *ctx,
>> +                                     CodedBitstreamFragment *frag)
>> +{
>> +    int err, i, j;
>> +
>> +    for (i = 0; i < frag->nb_units; i++) {
>> +        if (ctx->decompose_unit_types) {
>> +            for (j = 0; j < ctx->nb_decompose_unit_types; j++) {
>> +                if (ctx->decompose_unit_types[j] == frag->units[i].type)
>> +                    break;
>> +            }
>> +            if (j >= ctx->nb_decompose_unit_types)
>> +                continue;
>> +        }
>> +
>> +        err = ctx->codec->read_unit(ctx, &frag->units[i]);
>> +        if (err == AVERROR(ENOSYS)) {
>> +            av_log(ctx->log_ctx, AV_LOG_WARNING,
>> +                   "Decomposition unimplemented for unit %d "
>> +                   "(type %d).\n", i, frag->units[i].type);
>> +        } else if (err < 0) {
>> +            av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d "
>> +                   "(type %d).\n", i, frag->units[i].type);
>> +            return err;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>> +                          CodedBitstreamFragment *frag,
>> +                          const AVCodecParameters *par)
>> +{
>> +    int err;
>> +
>> +    memset(frag, 0, sizeof(*frag));
>> +
>> +    frag->data      = par->extradata;
>> +    frag->data_size = par->extradata_size;
>> +
>> +    err = ctx->codec->split_fragment(ctx, frag, 1);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    frag->data      = NULL;
>> +    frag->data_size = 0;
>> +
>> +    return cbs_read_fragment_content(ctx, frag);
>> +}
>> +
>> +int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>> +                       CodedBitstreamFragment *frag,
>> +                       const AVPacket *pkt)
>> +{
>> +    int err;
>> +
>> +    memset(frag, 0, sizeof(*frag));
>> +
>> +    frag->data      = pkt->data;
>> +    frag->data_size = pkt->size;
>> +
>> +    err = ctx->codec->split_fragment(ctx, frag, 0);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    frag->data      = NULL;
>> +    frag->data_size = 0;
>> +
>> +    return cbs_read_fragment_content(ctx, frag);
>> +}
>> +
>> +int ff_cbs_read(CodedBitstreamContext *ctx,
>> +                CodedBitstreamFragment *frag,
>> +                const uint8_t *data, size_t size)
>> +{
>> +    int err;
>> +
>> +    memset(frag, 0, sizeof(*frag));
>> +
>> +    // (We won't write to this during split.)
>> +    frag->data      = (uint8_t*)data;
>> +    frag->data_size = size;
>> +
>> +    err = ctx->codec->split_fragment(ctx, frag, 0);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    frag->data      = NULL;
>> +    frag->data_size = 0;
>> +
>> +    return cbs_read_fragment_content(ctx, frag);
>> +}
>> +
>> +
>> +int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>> +                               CodedBitstreamFragment *frag)
>> +{
>> +    int err, i;
>> +
>> +    for (i = 0; i < frag->nb_units; i++) {
>> +        if (!frag->units[i].content)
>> +            continue;
>> +
>> +        err = ctx->codec->write_unit(ctx, &frag->units[i]);
>> +        if (err < 0) {
>> +            av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to write unit %d "
>> +                   "(type %d).\n", i, frag->units[i].type);
>> +            return err;
>> +        }
>> +    }
>> +
>> +    err = ctx->codec->assemble_fragment(ctx, frag);
>> +    if (err < 0) {
>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to assemble fragment.\n");
>> +        return err;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>> +                           AVCodecParameters *par,
>> +                           CodedBitstreamFragment *frag)
>> +{
>> +    int err;
>> +
>> +    err = ff_cbs_write_fragment_data(ctx, frag);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    av_freep(&par->extradata);
>> +
>> +    par->extradata = av_malloc(frag->data_size +
>> +                               AV_INPUT_BUFFER_PADDING_SIZE);
>> +    if (!par->extradata)
>> +        return AVERROR(ENOMEM);
>> +
>> +    memcpy(par->extradata, frag->data, frag->data_size);
>> +    memset(par->extradata + frag->data_size, 0,
>> +           AV_INPUT_BUFFER_PADDING_SIZE);
>> +    par->extradata_size = frag->data_size;
>> +
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>> +                        AVPacket *pkt,
>> +                        CodedBitstreamFragment *frag)
>> +{
>> +    int err;
>> +
>> +    err = ff_cbs_write_fragment_data(ctx, frag);
>> +    if (err < 0)
>> +        return err;
>> +
> 
>> +    av_new_packet(pkt, frag->data_size);
>> +    if (err < 0)
>> +        return err;
> 
> that does not work

I think I'm missing something.  Can you be more precise about what doesn't work?

>> +
>> +    memcpy(pkt->data, frag->data, frag->data_size);
>> +    pkt->size = frag->data_size;
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +void ff_cbs_trace_header(CodedBitstreamContext *ctx,
>> +                         const char *name)
>> +{
>> +    if (!ctx->trace_enable)
>> +        return;
>> +
>> +    av_log(ctx->log_ctx, ctx->trace_level, "%s\n", name);
>> +}
>> +
>> +void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx, int position,
>> +                                 const char *name, const char *bits,
>> +                                 int64_t value)
>> +{
>> +    size_t name_len, bits_len;
>> +    int pad;
>> +
>> +    if (!ctx->trace_enable)
>> +        return;
>> +
>> +    av_assert0(value >= INT_MIN && value <= UINT32_MAX);
>> +
>> +    name_len = strlen(name);
>> +    bits_len = strlen(bits);
>> +
>> +    if (name_len + bits_len > 60)
>> +        pad = bits_len + 2;
>> +    else
>> +        pad = 61 - name_len;
>> +
>> +    av_log(ctx->log_ctx, ctx->trace_level, "%-10d  %s%*s = %"PRId64"\n",
>> +           position, name, pad, bits, value);
>> +}
>> +
> 
>> +int ff_cbs_read_unsigned(CodedBitstreamContext *ctx, GetBitContext *gbc,
>> +                         int width, const char *name, uint32_t *write_to,
>> +                         uint32_t range_min, uint32_t range_max)
>> +{
>> +    uint32_t value;
>> +    int position;
>> +
>> +    av_assert0(width <= 32);
> 
> if you assert validity, you should also assert non negativity

Ok, changed.  (Also in _write below.)

>> +
>> +    if (get_bits_left(gbc) < width) {
>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid value at "
>> +               "%s: bitstream ended.\n", name);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    if (ctx->trace_enable)
>> +        position = get_bits_count(gbc);
>> +
>> +    value = get_bits_long(gbc, width);
>> +
>> +    if (ctx->trace_enable) {
>> +        char bits[33];
>> +        int i;
>> +        for (i = 0; i < width; i++)
>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
>> +        bits[i] = 0;
>> +
>> +        ff_cbs_trace_syntax_element(ctx, position, name, bits, value);
>> +    }
>> +
>> +    if (value < range_min || value > range_max) {
>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
>> +               "%"PRIu32", but must be in [%"PRIu32",%"PRIu32"].\n",
>> +               name, value, range_min, range_max);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    *write_to = value;
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
>> +                          int width, const char *name, uint32_t value,
>> +                          uint32_t range_min, uint32_t range_max)
>> +{
>> +    av_assert0(width <= 32);
>> +
>> +    if (value < range_min || value > range_max) {
>> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "%s out of range: "
>> +               "%"PRIu32", but must be in [%"PRIu32",%"PRIu32"].\n",
>> +               name, value, range_min, range_max);
>> +        return AVERROR_INVALIDDATA;
>> +    }
>> +
>> +    if (put_bits_left(pbc) < width)
>> +        return AVERROR(ENOSPC);
>> +
>> +    if (ctx->trace_enable) {
>> +        char bits[33];
>> +        int i;
>> +        for (i = 0; i < width; i++)
>> +            bits[i] = value >> (width - i - 1) & 1 ? '1' : '0';
>> +        bits[i] = 0;
>> +
>> +        ff_cbs_trace_syntax_element(ctx, put_bits_count(pbc), name, bits, value);
>> +    }
>> +
>> +    if (width < 32)
>> +        put_bits(pbc, width, value);
>> +    else
>> +        put_bits32(pbc, value);
>> +
>> +    return 0;
>> +}
>> +
>> +
> 
>> +static int cbs_insert_unit(CodedBitstreamContext *ctx,
>> +                           CodedBitstreamFragment *frag,
>> +                           int position)
>> +{
>> +    CodedBitstreamUnit *units;
>> +
>> +    units = av_malloc_array(frag->nb_units + 1, sizeof(*units));
>> +    if (!units)
>> +        return AVERROR(ENOMEM);
>> +
>> +    if (position > 0)
>> +        memcpy(units, frag->units, position * sizeof(*units));
>> +    if (position < frag->nb_units)
>> +        memcpy(units + position + 1, frag->units + position,
>> +               (frag->nb_units - position) * sizeof(*units));
>> +
>> +    memset(units + position, 0, sizeof(*units));
>> +
>> +    av_freep(&frag->units);
>> +    frag->units = units;
>> +    ++frag->nb_units;
>> +
>> +    return 0;
>> +}
> 
> re-creating the whole array on each insertion seems wastefull of
> CPU time

I'm not sure there is any reason to make something less clean to do better - the expected number of units per fragment is not high.

>> +
>> +int ff_cbs_insert_unit_content(CodedBitstreamContext *ctx,
>> +                               CodedBitstreamFragment *frag,
>> +                               int position, uint32_t type,
>> +                               void *content)
>> +{
>> +    int err;
>> +
>> +    if (position == -1)
>> +        position = frag->nb_units;
>> +    av_assert0(position >= 0 && position <= frag->nb_units);
>> +
>> +    err = cbs_insert_unit(ctx, frag, position);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    frag->units[position].type             = type;
>> +    frag->units[position].content          = content;
>> +    frag->units[position].content_external = 1;
>> +
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx,
>> +                            CodedBitstreamFragment *frag,
>> +                            int position, uint32_t type,
>> +                            uint8_t *data, size_t data_size)
>> +{
>> +    int err;
>> +
>> +    if (position == -1)
>> +        position = frag->nb_units;
>> +    av_assert0(position >= 0 && position <= frag->nb_units);
>> +
>> +    err = cbs_insert_unit(ctx, frag, position);
>> +    if (err < 0)
>> +        return err;
>> +
>> +    frag->units[position].type      = type;
>> +    frag->units[position].data      = data;
>> +    frag->units[position].data_size = data_size;
>> +
>> +    return 0;
>> +}
>> +
>> +int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>> +                       CodedBitstreamFragment *frag,
>> +                       int position)
>> +{
>> +    if (position < 0 || position >= frag->nb_units)
>> +        return AVERROR(EINVAL);
>> +
>> +    cbs_unit_uninit(ctx, &frag->units[position]);
>> +
>> +    --frag->nb_units;
>> +
>> +    if (frag->nb_units == 0) {
>> +        av_freep(&frag->units);
>> +
>> +    } else {
>> +        memmove(frag->units + position,
>> +                frag->units + position + 1,
>> +                (frag->nb_units - position) * sizeof(*frag->units));
>> +
>> +        // Don't bother reallocating the unit array.
>> +    }
>> +
>> +    return 0;
>> +}
> 
>> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
>> new file mode 100644
>> index 0000000000..eeaff379e5
>> --- /dev/null
>> +++ b/libavcodec/cbs.h
>> @@ -0,0 +1,274 @@
>> +/*
>> + * 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
>> + */
>> +
>> +#ifndef AVCODEC_CBS_H
>> +#define AVCODEC_CBS_H
>> +
>> +#include <stddef.h>
>> +#include <stdint.h>
>> +
>> +#include "avcodec.h"
>> +
>> +
>> +struct CodedBitstreamType;
>> +
>> +/**
>> + * Coded bitstream unit structure.
>> + *
>> + * A bitstream unit the the smallest element of a bitstream which
> 
> the the ?

Fixed.

>> + * is meaningful on its own.  For example, an H.264 NAL unit.
>> + *
>> + * See the codec-specific header for the meaning of this for any
>> + * particular codec.
>> + */
>> +typedef struct CodedBitstreamUnit {
>> +    /**
>> +     * Codec-specific type of this unit.
>> +     */
>> +    uint32_t type;
>> +
>> +    /**
>> +     * Pointer to the bitstream form of this unit.
>> +     *
>> +     * May be NULL if the unit currently only exists in decomposed form.
>> +     */
>> +    uint8_t *data;
>> +    /**
>> +     * The number of bytes in the bitstream (including any padding bits
>> +     * in the final byte).
>> +     */
>> +    size_t   data_size;
> 
>> +    /**
>> +     * The number of bits which should be ignored in the final byte.
>> +     *
>> +     * This supports non-byte-aligned bitstreams.
>> +     */
>> +    size_t   data_bit_padding;
> 
> theres no need for this to be size_t, a byte can only contain up to
> 8 bits

It gets used with data_size, so the type matching is nice.  (Also making it smaller would save no space due to padding unless the structure were rearranged, which would be less clear.)

>> +
>> +    /**
>> +     * Pointer to the decomposed form of this unit.
>> +     *
>> +     * The type of this structure depends on both the codec and the
>> +     * type of this unit.  May be NULL if the unit only exists in
>> +     * bitstream form.
>> +     */
>> +    void *content;
>> +    /**
>> +     * Whether the content was supplied externally.
>> +     *
>> +     * If so, it should not be freed when freeing the unit.
>> +     */
>> +    int   content_external;
>> +} CodedBitstreamUnit;
>> +
>> +/**
>> + * Coded bitstream fragment structure, combining one or more units.
>> + *
>> + * This is any sequence of units.  It need not form some greater whole,
>> + * though in many cases it will.  For example, an H.264 access unit,
>> + * which is composed of a sequence of H.264 NAL units.
>> + */
>> +typedef struct CodedBitstreamFragment {
>> +    /**
>> +     * Pointer to the bitstream form of this fragment.
>> +     *
>> +     * May be NULL if the fragment only exists as component units.
>> +     */
>> +    uint8_t *data;
>> +    /**
>> +     * The number of bytes in the bitstream.
>> +     *
>> +     * The number of bytes in the bitstream (including any padding bits
>> +     * in the final byte).
>> +     */
>> +    size_t   data_size;
>> +    /**
>> +     * The number of bits which should be ignored in the final byte.
>> +     */
>> +    size_t data_bit_padding;
>> +
>> +    /**
>> +     * Number of units in this fragment.
>> +     *
>> +     * This may be zero if the fragment only exists in bistream form
>> +     * and has not been decomposed.
> 
> what is a biStream form ?

Fixed.

>> +     */
>> +    int              nb_units;
>> +    /**
>> +     * Pointer to an array of units of length nb_units.
>> +     *
>> +     * Must be NULL if nb_units is zero.
>> +     */
>> +    CodedBitstreamUnit *units;
>> +} CodedBitstreamFragment;
>> +
>> +/**
>> + * Context structure for coded bitstream operations.
>> + */
>> +typedef struct CodedBitstreamContext {
>> +    /**
>> +     * Logging context to be passed to all av_log() calls associated
>> +     * with this context.
>> +     */
>> +    void *log_ctx;
>> +
>> +    /**
>> +     * Internal codec-specific hooks.
>> +     */
>> +    const struct CodedBitstreamType *codec;
>> +
>> +    /**
>> +     * Internal codec-specific data.
>> +     *
>> +     * This contains any information needed when reading/writing
>> +     * bitsteams which will not necessarily be present in a fragment.
>> +     * For example, for H.264 it contains all currently visible
>> +     * parameter sets - they are required to determine the bitstream
>> +     * syntax but need not be present in every access unit.
>> +     */
>> +    void *priv_data;
>> +
> 
>> +    /**
>> +     * Array of unit types which should be decomposed when reading.
>> +     *
>> +     * Types not in this list will be available in bitstream form only.
>> +     * If NULL, all supported types will be decomposed.
>> +     */
>> +    uint32_t *decompose_unit_types;
> 
> this should not be a generic integer.
> a typedef or enum would be better

It's H.264 nal unit types union H.265 nal unit types union MPEG-2 start codes (union any other codec that gets added).

What type should that be?  I chose uint32_t to make it fixed-size and hopefully large enough to be able to make sense for any future codec.

>> +    /**
>> +     * Length of the decompose_unit_types array.
>> +     */
>> +    int    nb_decompose_unit_types;
>> +
>> +    /**
>> +     * Enable trace output during read/write operations.
>> +     */
>> +    int trace_enable;
>> +    /**
>> +     * Log level to use for trace output.
>> +     *
>> +     * From AV_LOG_*; defaults to AV_LOG_TRACE.
>> +     */
>> +    int trace_level;
>> +} CodedBitstreamContext;
>> +
>> +
> 
>> +/**
>> + * Initialise a new context for the given codec.
>> + */
>> +int ff_cbs_init(CodedBitstreamContext *ctx,
>> +                enum AVCodecID codec_id, void *log_ctx);
> 
> allocating the context within the init function would make the
> API more robust with extensions to the context and potential use from
> other libs.
> It is also how most of our APIs work

This API is currently completely libavcodec internal, and I would like to keep it that way for a while at least.  Therefore, there are no extensions and no use from other libs.

Still, I can change it.

>> +
>> +/**
>> + * Close a context and free all internal state.
>> + */
>> +void ff_cbs_close(CodedBitstreamContext *ctx);
>> +
>> +
> 
>> +/**
>> + * Read the extradata bitstream found in codec parameters into a
>> + * fragment, then split into units and decompose.
>> + *
>> + * This also updates the internal state, so will need to be called for
>> + * codecs with extradata to read parameter sets necessary for further
>> + * parsing even if the fragment itself is not desired.
>> + */
>> +int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>> +                          CodedBitstreamFragment *frag,
>> +                          const AVCodecParameters *par);
> 
> are any fields other than extradata and size needed from
> AVCodecParameters ?
> if not then it may be more flexible to just pass the extradata and size

Not currently, but possibly in future.  It's distinguished from ff_cbs_read() by this form (and needs to be to at least some degree, because of AVCC/HVCC headers).

>> +
>> +/**
>> + * Read the data bitstream from a packet into a fragment, then
>> + * split into units and decompose.
>> + */
>> +int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>> +                       CodedBitstreamFragment *frag,
>> +                       const AVPacket *pkt);
>> +
>> +/**
>> + * Read a bitstream from a memory region into a fragment, then
>> + * split into units and decompose.
>> + */
>> +int ff_cbs_read(CodedBitstreamContext *ctx,
>> +                CodedBitstreamFragment *frag,
>> +                const uint8_t *data, size_t size);
>> +
>> +
>> +/**
>> + * Write the content of the fragment to its own internal buffer.
>> + *
>> + * Writes the content of all units and then assembles them into a new
>> + * data buffer.  When modifying the content of decomposed units, this
>> + * can be used to regenerate the bitstream form of units or the whole
>> + * fragment so that it can be extracted for other use.
>> + */
>> +int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>> +                               CodedBitstreamFragment *frag);
>> +
> 
>> +/**
>> + * Write the bitstream of a fragment to the extradata in codec parameters.
>> + */
>> +int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>> +                           AVCodecParameters *par,
>> +                           CodedBitstreamFragment *frag);
> 
> This is missing documentation about allocation. Will the call allocate
> the extradata, should the caller allocate, is it allowed to be allocated
> before the call ?
> 
> Also similar to above, passing a pointer and pointer to size may be
> more flexible

Add " * This replaces any existing extradata in the structure.".

For both of these, the separate extradata API makes callers in bitstream filters a bit simpler, which is I think worth it.

(There is no cbs_write(pointer, size) function because it currently makes more sense as cbs_write_fragment_data() and then manipulate frag->data and frag->data_size directly.  Could consider something further if there are any callers wanting it.)

>> +
>> +/**
>> + * Write the bitstream of a fragment to a packet.
>> + */
>> +int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>> +                        AVPacket *pkt,
>> +                        CodedBitstreamFragment *frag);
>> +
>> +
>> +/**
>> + * Free all allocated memory in a fragment.
>> + */
>> +void ff_cbs_fragment_uninit(CodedBitstreamContext *ctx,
>> +                            CodedBitstreamFragment *frag);
>> +
>> +
>> +/**
>> + * Insert a new unit into a fragment with the given content.
>> + *
>> + * The content structure continues to be owned by the caller, and
>> + * will not be freed when the unit is.
>> + */
>> +int ff_cbs_insert_unit_content(CodedBitstreamContext *ctx,
>> +                               CodedBitstreamFragment *frag,
>> +                               int position, uint32_t type,
>> +                               void *content);
>> +
>> +/**
>> + * Insert a new unit into a fragment with the given data bitstream.
>> + *
>> + * The data buffer will be owned by the unit after this operation.
>> + */
>> +int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx,
>> +                            CodedBitstreamFragment *frag,
>> +                            int position, uint32_t type,
>> +                            uint8_t *data, size_t data_size);
>> +
>> +/**
>> + * Delete a unit from a fragment and free all memory it uses.
>> + */
>> +int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>> +                       CodedBitstreamFragment *frag,
>> +                       int position);
>> +
>> +
>> +#endif /* AVCODEC_CBS_H */
>> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
>> new file mode 100644
>> index 0000000000..3bbc3355c2
>> --- /dev/null
>> +++ b/libavcodec/cbs_internal.h
>> @@ -0,0 +1,83 @@
>> +/*
>> + * 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
>> + */
>> +
>> +#ifndef AVCODEC_CBS_INTERNAL_H
>> +#define AVCODEC_CBS_INTERNAL_H
>> +
>> +#include "avcodec.h"
>> +#include "cbs.h"
>> +#include "get_bits.h"
>> +#include "put_bits.h"
>> +
>> +
>> +typedef struct CodedBitstreamType {
>> +    enum AVCodecID codec_id;
>> +
>> +    size_t priv_data_size;
>> +
> 
>> +    // Split frag->data into coded bitstream units, creating the
>> +    // frag->units array.  Fill data but not content on each unit.
>> +    int (*split_fragment)(CodedBitstreamContext *ctx,
>> +                          CodedBitstreamFragment *frag,
>> +                          int header);
> 
> The "header" parameter lacks documentation

Add "// header is set if the fragment came from a header block, which may require different parsing for some codecs.".

>> +
>> +    // Read the unit->data bitstream and decompose it, creating
>> +    // unit->content.
>> +    int (*read_unit)(CodedBitstreamContext *ctx,
>> +                     CodedBitstreamUnit *unit);
>> +
>> +    // Write the unit->data bitstream from unit->content.
>> +    int (*write_unit)(CodedBitstreamContext *ctx,
>> +                      CodedBitstreamUnit *unit);
>> +
>> +    // Read the data from all of frag->units and assemble it into
>> +    // a bitstream for the whole fragment.
>> +    int (*assemble_fragment)(CodedBitstreamContext *ctx,
>> +                             CodedBitstreamFragment *frag);
>> +
>> +    // Free the content and data of a single unit.
>> +    void (*free_unit)(CodedBitstreamUnit *unit);
>> +
>> +    // Free the codec internal state.
>> +    void (*close)(CodedBitstreamContext *ctx);
>> +} CodedBitstreamType;
>> +
>> +
>> +// Helper functions for trace output.
>> +
>> +void ff_cbs_trace_header(CodedBitstreamContext *ctx,
>> +                         const char *name);
>> +
>> +void ff_cbs_trace_syntax_element(CodedBitstreamContext *ctx,
>> +                                 int position, const char *name,
>> +                                 const char *bitstring, int64_t value);
>> +
>> +
>> +// Helper functions for read/write of common bitstream elements, including
>> +// generation of trace output.
>> +
>> +int ff_cbs_read_unsigned(CodedBitstreamContext *ctx, GetBitContext *gbc,
>> +                         int width, const char *name, uint32_t *write_to,
>> +                         uint32_t range_min, uint32_t range_max);
>> +
>> +int ff_cbs_write_unsigned(CodedBitstreamContext *ctx, PutBitContext *pbc,
>> +                          int width, const char *name, uint32_t value,
>> +                          uint32_t range_min, uint32_t range_max);
>> +
>> +
>> +#endif /* AVCODEC_CBS_INTERNAL_H */
>> -- 
>> 2.11.0
>>

Thanks,

- Mark



More information about the ffmpeg-devel mailing list