[FFmpeg-devel,V1] avcodec/cbs_vp8: Add support for VP8 codec bitstream READ methods

This commit adds VP8 into cbs supported codec list, and enables the
`trace_headers` bitstream filters to support VP8, besides existing AV1,
H.264, H.265 and VP9. It can be useful to debug VP8 stream issues.

Only the READ methods `read_unit` and `split_fragment` are implemented,
the WRITE methods `write_unit` and `assemble_fragment` return
`AVERROR_PATCHWELCOME` error code. It is because the CBS VP8 WRITE is
unlikely used by any applications at the moment. The WRITE methods can
be added later if there are real requirments.

TESTS: ffmpeg -i fate-suite/vp8/frame_size_change.webm -vcodec copy
-bsf:v trace_headers -f null -

Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
---
 configure                            |   4 +-
 libavcodec/Makefile                  |   1 +
 libavcodec/cbs.c                     |   6 +
 libavcodec/cbs_internal.h            |   1 +
 libavcodec/cbs_vp8.c                 | 259 +++++++++++++++++++++++
 libavcodec/cbs_vp8.h                 | 116 +++++++++++
 libavcodec/cbs_vp8_syntax_template.c | 294 +++++++++++++++++++++++++++
 7 files changed, 680 insertions(+), 1 deletion(-)
 create mode 100644 libavcodec/cbs_vp8.c
 create mode 100644 libavcodec/cbs_vp8.h
 create mode 100644 libavcodec/cbs_vp8_syntax_template.c

Dai, Jianhui J:
> This commit adds VP8 into cbs supported codec list, and enables the
> `trace_headers` bitstream filters to support VP8, besides existing AV1,
> H.264, H.265 and VP9. It can be useful to debug VP8 stream issues.
> 
> Only the READ methods `read_unit` and `split_fragment` are implemented,
> the WRITE methods `write_unit` and `assemble_fragment` return
> `AVERROR_PATCHWELCOME` error code. It is because the CBS VP8 WRITE is
> unlikely used by any applications at the moment. The WRITE methods can
> be added later if there are real requirments.
> 
> TESTS: ffmpeg -i fate-suite/vp8/frame_size_change.webm -vcodec copy
> -bsf:v trace_headers -f null -
> 
> Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> ---
>  configure                            |   4 +-
>  libavcodec/Makefile                  |   1 +
>  libavcodec/cbs.c                     |   6 +
>  libavcodec/cbs_internal.h            |   1 +
>  libavcodec/cbs_vp8.c                 | 259 +++++++++++++++++++++++
>  libavcodec/cbs_vp8.h                 | 116 +++++++++++
>  libavcodec/cbs_vp8_syntax_template.c | 294 +++++++++++++++++++++++++++
>  7 files changed, 680 insertions(+), 1 deletion(-)
>  create mode 100644 libavcodec/cbs_vp8.c
>  create mode 100644 libavcodec/cbs_vp8.h
>  create mode 100644 libavcodec/cbs_vp8_syntax_template.c
> 
> diff --git a/configure b/configure
> index bb7be67676..6ba7759e34 100755
> --- a/configure
> +++ b/configure
> @@ -2433,6 +2433,7 @@ CONFIG_EXTRA="
>      cbs_jpeg
>      cbs_mpeg2
>      cbs_vp9
> +    cbs_vp8

vp8 should be before vp9 in all these lists.

>      deflate_wrapper
>      dirac_parse
>      dnn
> @@ -2714,6 +2715,7 @@ cbs_h265_select="cbs"
>  cbs_jpeg_select="cbs"
>  cbs_mpeg2_select="cbs"
>  cbs_vp9_select="cbs"
> +cbs_vp8_select="cbs"
>  dct_select="rdft"
>  deflate_wrapper_deps="zlib"
>  dirac_parse_select="golomb"
> @@ -3284,7 +3286,7 @@ h264_redundant_pps_bsf_select="cbs_h264"
>  hevc_metadata_bsf_select="cbs_h265"
>  mjpeg2jpeg_bsf_select="jpegtables"
>  mpeg2_metadata_bsf_select="cbs_mpeg2"
> -trace_headers_bsf_select="cbs"
> +trace_headers_bsf_select="cbs cbs_vp8"
>  vp9_metadata_bsf_select="cbs_vp9"
>  
>  # external libraries
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 3cf4444b7e..a4e5d7d7a5 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -79,6 +79,7 @@ OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o cbs_sei.o h2645_parse.o
>  OBJS-$(CONFIG_CBS_JPEG)                += cbs_jpeg.o
>  OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> +OBJS-$(CONFIG_CBS_VP8)                 += cbs_vp8.o vpx_rac.o
>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
>  OBJS-$(CONFIG_DEFLATE_WRAPPER)         += zlib_wrapper.o
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 504197e06d..f4ff05181f 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -49,6 +49,9 @@ static const CodedBitstreamType *const cbs_type_table[] = {
>  #if CONFIG_CBS_VP9
>      &ff_cbs_type_vp9,
>  #endif
> +#if CONFIG_CBS_VP8
> +    &ff_cbs_type_vp8,
> +#endif
>  };
>  
>  const enum AVCodecID ff_cbs_all_codec_ids[] = {
> @@ -69,6 +72,9 @@ const enum AVCodecID ff_cbs_all_codec_ids[] = {
>  #endif
>  #if CONFIG_CBS_VP9
>      AV_CODEC_ID_VP9,
> +#endif
> +#if CONFIG_CBS_VP8
> +    AV_CODEC_ID_VP8,
>  #endif
>      AV_CODEC_ID_NONE
>  };
> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> index e585c77934..9ea6393a9f 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -248,6 +248,7 @@ extern const CodedBitstreamType ff_cbs_type_h265;
>  extern const CodedBitstreamType ff_cbs_type_jpeg;
>  extern const CodedBitstreamType ff_cbs_type_mpeg2;
>  extern const CodedBitstreamType ff_cbs_type_vp9;
> +extern const CodedBitstreamType ff_cbs_type_vp8;
>  
>  
>  #endif /* AVCODEC_CBS_INTERNAL_H */
> diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c
> new file mode 100644
> index 0000000000..ee5c5cbceb
> --- /dev/null
> +++ b/libavcodec/cbs_vp8.c
> @@ -0,0 +1,259 @@
> +/*
> + * 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 "libavutil/avassert.h"
> +
> +#include "cbs.h"
> +#include "cbs_internal.h"
> +#include "cbs_vp8.h"
> +
> +#include "vp8data.h"
> +
> +static av_unused int cbs_vp8_trace_syntax_element(CodedBitstreamContext *ctx,

Why is this marked as av_unused?

> +                                                  int pos, const char *name,
> +                                                  const int *subscripts,
> +                                                  int32_t value, int width)
> +{
> +    if (ctx->trace_enable) {
> +        char bits[33] = "-";
> +
> +        av_assert0(width >= 0 && width <= 24);
> +        if (width > 0) {
> +            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, pos, name, subscripts, bits, value);
> +    }
> +
> +    return 0;
> +}
> +
> +static int cbs_vp8_read(CodedBitstreamContext *ctx, GetBitContext *gbc,
> +                        int width, int32_t *write_to)
> +{
> +    uint32_t value;
> +
> +    av_assert0(width > 0 && width <= 24);
> +
> +    if (get_bits_left(gbc) < width + 1) {

Why the +1?

> +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid value: bitstream ended.\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    value     = get_bits_le(gbc, width);

You are reading from a bitstream reader that has not been opened as
little-endian. This will work, but it is not how it is supposed to be.

> +    *write_to = value;
> +    return 0;
> +}
> +
> +static av_unused int cbs_vp8_rac_get(VPXRangeCoder *c, int bits)
> +{
> +    int value = 0;
> +
> +    while (bits--)
> +        value = (value << 1) | vpx_rac_get_prob(c, 128);
> +
> +    return value;
> +}
> +
> +static int cbs_vp8_rac_get_bits_count(GetBitContext *gbc, VPXRangeCoder *c)
> +{
> +    int bits_count = (c->buffer - gbc->buffer) * 8;
> +    bits_count += c->bits;

Your mixing of VPXRangeCoder and GetBitContext is incredibly hacky; if I
see this correctly, then you are not even advancing the GetBitContext
position for all the rac stuff that you are reading (meaning that the
byte-alignment code in trailing_bits() is nonsense).
Furthermore, the rac functions do not report when they run out of input,
as is common for CBS. You should probably write CBS versions of them
that closely follow the spec (read: that need not be optimized for speed).

> +    return bits_count;
> +}
> +
> +#define HEADER(name) \
> +    do { \
> +        ff_cbs_trace_header(ctx, name); \
> +    } while (0)
> +
> +#define CHECK(call) \
> +    do { \
> +        err = (call); \
> +        if (err < 0) \
> +            return err; \
> +    } while (0)
> +
> +#define FUNC_NAME(rw, codec, name) cbs_##codec##_##rw##_##name
> +#define FUNC_VP8(rw, name) FUNC_NAME(rw, vp8, name)
> +#define FUNC(name) FUNC_VP8(READWRITE, name)
> +
> +#define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
> +
> +#define vp8_f(width, name) vp8_xf(width, name, current->name)
> +
> +#define vp8_rac_unsigned(width, name) \
> +    do { \
> +        uint32_t value; \
> +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> +        value   = cbs_vp8_rac_get(c, width); \
> +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, 0); \

IMO it is better to have a function cbs_vp8_rac_read_unsigned() for
this; vp8_rac_unsigned and vp8_rac_unsigned_subs would then just be
wrappers around this function like it is done for xs() in cbs_vp9.

> +        current->name = value; \
> +    } while (0)
> +
> +#define vp8_rac_unsigned_subs(width, name, subs, ...) \
> +    do { \
> +        uint32_t value; \
> +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> +        value   = cbs_vp8_rac_get(c, width); \
> +        cbs_vp8_trace_syntax_element(ctx, pos, #name, \
> +                                     SUBSCRIPTS(subs, __VA_ARGS__), value, 0); \
> +        current->name = value; \
> +    } while (0)
> +
> +#define vp8_rac_signed(width, name) \
> +    do { \
> +        uint32_t value; \

Why is this not signed? Same below.

> +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> +        value   = cbs_vp8_rac_get(c, width); \
> +        if (cbs_vp8_rac_get(c, 1)) \
> +            value = -value; \
> +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, 0); \

Same as above for vp8_rac_unsigned.

> +        current->name = value; \
> +    } while (0)
> +
> +#define vp8_rac_signed_subs(width, name, subs, ...) \
> +    do { \
> +        uint32_t value; \
> +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> +        value   = cbs_vp8_rac_get(c, width); \
> +        if (cbs_vp8_rac_get(c, 1)) \
> +            value = -value; \
> +        cbs_vp8_trace_syntax_element(ctx, pos, #name, \
> +                                     SUBSCRIPTS(subs, __VA_ARGS__), value, 0); \
> +        current->name = value; \
> +    } while (0)
> +
> +#define READ
> +#define READWRITE read
> +#define RWContext GetBitContext
> +
> +#define vp8_xf(width, name, var) \
> +    do { \
> +        uint32_t value; \
> +        int pos = get_bits_count(rw); \
> +        CHECK(cbs_vp8_read(ctx, rw, width, &value)); \
> +        var = value; \
> +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, width); \
> +    } while (0)
> +
> +#define fixed(width, name, value) \
> +    do { \
> +        av_unused uint32_t fixed_value; \
> +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, 0, &fixed_value, \
> +                                   value, value)); \
> +    } while (0)
> +
> +#define byte_alignment(rw) (get_bits_count(rw) % 8)
> +
> +#include "cbs_vp8_syntax_template.c"
> +
> +static int cbs_vp8_split_fragment(CodedBitstreamContext *ctx,
> +                                  CodedBitstreamFragment *frag, int header)
> +{
> +    int err;
> +
> +    if (frag->data_size == 0)
> +        return AVERROR_INVALIDDATA;
> +
> +    err = ff_cbs_append_unit_data(frag, 0, frag->data, frag->data_size,
> +                                  frag->data_ref);
> +    if (err < 0)
> +        return err;
> +
> +    return 0;
> +}
> +
> +static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
> +                             CodedBitstreamUnit *unit)
> +{
> +    VP8RawFrame *frame;
> +    GetBitContext gbc;
> +    int err, pos;
> +
> +    err = init_get_bits(&gbc, unit->data, 8 * unit->data_size);
> +    if (err < 0)
> +        return err;
> +
> +    err = ff_cbs_alloc_unit_content(ctx, unit);
> +    if (err < 0)
> +        return err;
> +    frame = unit->content;
> +
> +    err = cbs_vp8_read_frame(ctx, &gbc, frame);
> +    if (err < 0)
> +        return err;
> +
> +    pos = get_bits_count(&gbc);
> +    av_assert0(pos % 8 == 0);
> +    pos /= 8;
> +    av_assert0(pos <= unit->data_size);
> +
> +    if (pos == unit->data_size)
> +        return AVERROR_INVALIDDATA;
> +
> +    frame->data_ref = av_buffer_ref(unit->data_ref);
> +    if (!frame->data_ref)
> +        return AVERROR(ENOMEM);
> +
> +    frame->data      = unit->data + pos;
> +    frame->data_size = unit->data_size - pos;
> +
> +    return 0;
> +}
> +
> +static int cbs_vp8_write_unit(CodedBitstreamContext *ctx,
> +                              CodedBitstreamUnit *unit, PutBitContext *pbc)
> +{
> +    return AVERROR_PATCHWELCOME;
> +}
> +
> +static int cbs_vp8_assemble_fragment(CodedBitstreamContext *ctx,
> +                                     CodedBitstreamFragment *frag)
> +{
> +    return AVERROR_PATCHWELCOME;
> +}
> +
> +static void cbs_vp8_flush(CodedBitstreamContext *ctx)
> +{
> +    return;
> +}

No need for a useless flush. Just leave the function empty.
(One could also do the same for the write_unit and assemble_fragment
function: Nothing should call these functions at all.)

> +
> +static const CodedBitstreamUnitTypeDescriptor cbs_vp8_unit_types[] = {
> +    CBS_UNIT_TYPE_INTERNAL_REF(0, VP8RawFrame, data),
> +    CBS_UNIT_TYPE_END_OF_LIST
> +};
> +
> +const CodedBitstreamType ff_cbs_type_vp8 = {
> +    .codec_id          = AV_CODEC_ID_VP8,
> +
> +    .priv_data_size    = sizeof(CodedBitstreamVP8Context),
> +
> +    .unit_types        = cbs_vp8_unit_types,
> +
> +    .split_fragment    = &cbs_vp8_split_fragment,
> +    .read_unit         = &cbs_vp8_read_unit,
> +    .write_unit        = &cbs_vp8_write_unit,
> +
> +    .flush             = &cbs_vp8_flush,
> +
> +    .assemble_fragment = &cbs_vp8_assemble_fragment,
> +};
> diff --git a/libavcodec/cbs_vp8.h b/libavcodec/cbs_vp8.h
> new file mode 100644
> index 0000000000..176d95853b
> --- /dev/null
> +++ b/libavcodec/cbs_vp8.h
> @@ -0,0 +1,116 @@
> +/*
> + * 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_VP8_H
> +#define AVCODEC_CBS_VP8_H
> +
> +#include <stddef.h>
> +#include <stdint.h>
> +
> +#include "cbs.h"
> +#include "vpx_rac.h"
> +
> +enum {
> +    VP8_START_CODE_0 = 0x9D,
> +    VP8_START_CODE_1 = 0x01,
> +    VP8_START_CODE_2 = 0x2A,
> +};
> +
> +enum {
> +    VP8_KEY_FRAME     = 0,
> +    VP8_NON_KEY_FRAME = 1,
> +};
> +
> +typedef struct VP8RawFrameHeader {
> +    // frame tag
> +    uint8_t frame_type;
> +    uint8_t profile;
> +    uint8_t show_frame;
> +    uint32_t first_partition_length_in_bytes;
> +
> +    uint32_t start_code_0;
> +    uint32_t start_code_1;
> +    uint32_t start_code_2;
> +
> +    uint16_t width;
> +    uint16_t horizontal_scale;
> +    uint16_t height;
> +    uint16_t vertical_scale;
> +
> +    // frame header
> +    uint8_t color_space;
> +    uint8_t clamping_type;
> +
> +    // segmentation
> +    uint8_t segmentation_enabled;
> +    uint8_t update_segment_map;
> +    uint8_t update_segment_feature_data;
> +    uint8_t segment_feature_mode;
> +    int8_t segment_update_qp[4];
> +    int8_t segment_update_loop_filter_level[4];
> +    uint8_t segment_update_probs[3];
> +
> +    // loop filter
> +    uint8_t loop_filter_type;
> +    uint8_t loop_filter_level;
> +    uint8_t loop_filter_sharpness;
> +    uint8_t mode_ref_lf_delta_enabled;
> +    int8_t ref_lf_deltas[4];
> +    int8_t mode_lf_deltas[4];
> +
> +    uint8_t log2_token_partitions;
> +
> +    // qp
> +    uint8_t base_qindex;
> +    int8_t y1dc_delta_q;
> +    int8_t y2dc_delta_q;
> +    int8_t y2ac_delta_q;
> +    int8_t uvdc_delta_q;
> +    int8_t uvac_delta_q;
> +
> +    // ref
> +    uint8_t refresh_golden_frame;
> +    uint8_t refresh_alternate_frame;
> +    uint8_t ref_frame_sign_bias_golden;
> +    uint8_t ref_frame_sign_bias_alternate;
> +    uint8_t refresh_last_frame;
> +
> +    // token probs
> +    uint8_t refresh_entropy_probs;
> +
> +    uint8_t mb_no_skip_coeff;
> +    uint8_t prob_skip_false;
> +
> +    uint8_t prob_intra;
> +    uint8_t prob_last;
> +    uint8_t prob_golden;
> +} VP8RawFrameHeader;
> +
> +typedef struct VP8RawFrame {
> +    VP8RawFrameHeader header;
> +
> +    uint8_t *data;
> +    AVBufferRef *data_ref;
> +    size_t data_size;
> +} VP8RawFrame;
> +
> +typedef struct CodedBitstreamVP8Context {
> +    VPXRangeCoder c;
> +} CodedBitstreamVP8Context;
> +
> +#endif /* AVCODEC_CBS_VP8_H */
> diff --git a/libavcodec/cbs_vp8_syntax_template.c b/libavcodec/cbs_vp8_syntax_template.c
> new file mode 100644
> index 0000000000..24f4075dab
> --- /dev/null
> +++ b/libavcodec/cbs_vp8_syntax_template.c
> @@ -0,0 +1,294 @@
> +/*
> + * 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
> + */
> +
> +static int FUNC(update_segmentation)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                     VPXRangeCoder *c,
> +                                     VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    int i;

We allow (and prefer) C99 for loops with variable declarations.

> +
> +    vp8_rac_unsigned(1, update_segment_map);
> +    vp8_rac_unsigned(1, update_segment_feature_data);
> +
> +    if (current->update_segment_feature_data) {
> +        vp8_rac_unsigned(1, segment_feature_mode);
> +        // quantizer
> +        for (i = 0; i < 4; i++)
> +            if (cbs_vp8_rac_get(c, 1))
> +                vp8_rac_signed_subs(7, segment_update_qp[i], 1, i);
> +        // loop filter
> +        for (i = 0; i < 4; i++) {
> +            if (cbs_vp8_rac_get(c, 1)) {
> +                vp8_rac_signed_subs(6, segment_update_loop_filter_level[i], 1,
> +                                    i);
> +            }
> +        }
> +    }
> +
> +    if (current->update_segment_map) {
> +        for (i = 0; i < 3; i++)
> +            if (cbs_vp8_rac_get(c, 1))
> +                vp8_rac_unsigned_subs(8, segment_update_probs[i], 1, i);
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif

IMO there is no need for these #else branches given that this file is
not included for anything but reading.

> +}
> +
> +static int FUNC(mode_ref_lf_deltas)(CodedBitstreamContext *ctx,
> +                                           RWContext *rw, VPXRangeCoder *c,
> +                                           VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    vp8_rac_unsigned(1, mode_ref_lf_delta_enabled);
> +    if (current->mode_ref_lf_delta_enabled) {
> +        if (cbs_vp8_rac_get(c, 1)) {
> +            int i;
> +            // ref_lf_deltas
> +            for (i = 0; i < 4; i++)
> +                if (cbs_vp8_rac_get(c, 1))
> +                    vp8_rac_signed_subs(6, ref_lf_deltas[i], 1, i);
> +            // mode_lf_deltas
> +            for (i = 0; i < 4; i++)
> +                if (cbs_vp8_rac_get(c, 1))
> +                    vp8_rac_signed_subs(6, mode_lf_deltas[i], 1, i);
> +        }
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(quantization_params)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                   VPXRangeCoder *c, VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    vp8_rac_unsigned(7, base_qindex);
> +
> +    if (cbs_vp8_rac_get(c, 1))
> +        vp8_rac_signed(4, y1dc_delta_q);
> +
> +    if (cbs_vp8_rac_get(c, 1))
> +        vp8_rac_signed(4, y2dc_delta_q);
> +
> +    if (cbs_vp8_rac_get(c, 1))
> +        vp8_rac_signed(4, y2ac_delta_q);
> +
> +    if (cbs_vp8_rac_get(c, 1))
> +        vp8_rac_signed(4, uvdc_delta_q);
> +
> +    if (cbs_vp8_rac_get(c, 1))
> +        vp8_rac_signed(4, uvac_delta_q);
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(update_token_probs)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                    VPXRangeCoder *c,
> +                                    VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    int i, j, k, l;
> +
> +    for (i = 0; i < 4; ++i) {
> +        for (j = 0; j < 8; ++j) {
> +            for (k = 0; k < 3; ++k) {
> +                for (l = 0; l < 11; ++l)
> +                    if (vpx_rac_get_prob_branchy(
> +                            c, vp8_token_update_probs[i][j][k][l]))
> +                        cbs_vp8_rac_get(c, 8);
> +            }
> +        }
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(update_mv_probs)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                 VPXRangeCoder *c, VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    int i, j;
> +
> +    for (i = 0; i < 2; ++i) {
> +        for (j = 0; j < 19; ++j)
> +            if (cbs_vp8_rac_get(c, 1))
> +                cbs_vp8_rac_get(c, 7);
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(frame_tag)(CodedBitstreamContext *ctx, RWContext *rw,
> +                           VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    int err;
> +
> +    vp8_f(1, frame_type);
> +    vp8_f(3, profile);
> +    vp8_f(1, show_frame);
> +    vp8_f(19, first_partition_length_in_bytes);
> +
> +    if (current->frame_type == VP8_KEY_FRAME) {
> +        fixed(8, start_code_0, VP8_START_CODE_0);
> +        fixed(8, start_code_1, VP8_START_CODE_1);
> +        fixed(8, start_code_2, VP8_START_CODE_2);
> +
> +        vp8_f(14, width);
> +        vp8_f(2, horizontal_scale);
> +        vp8_f(14, height);
> +        vp8_f(2, vertical_scale);
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(frame_header)(CodedBitstreamContext *ctx, RWContext *rw,
> +                              VP8RawFrameHeader *current)
> +{
> +#ifdef READ
> +    CodedBitstreamVP8Context *vp8 = ctx->priv_data;
> +    VPXRangeCoder *c              = &vp8->c;
> +    int header_size;
> +    int pos;
> +    int err;
> +
> +    pos = get_bits_count(rw);
> +    av_assert0(pos % 8 == 0);
> +    pos /= 8;
> +
> +    header_size = current->first_partition_length_in_bytes;
> +
> +    err = ff_vpx_init_range_decoder(c, rw->buffer + pos, header_size);
> +    if (err < 0)
> +        return err;
> +
> +    if (current->frame_type == VP8_KEY_FRAME) {
> +        vp8_rac_unsigned(1, color_space);
> +        vp8_rac_unsigned(1, clamping_type);
> +    }
> +
> +    vp8_rac_unsigned(1, segmentation_enabled);
> +    if (current->segmentation_enabled)
> +        CHECK(FUNC(update_segmentation)(ctx, rw, c, current));
> +
> +    vp8_rac_unsigned(1, loop_filter_type);
> +    vp8_rac_unsigned(6, loop_filter_level);
> +    vp8_rac_unsigned(3, loop_filter_sharpness);
> +
> +    CHECK(FUNC(mode_ref_lf_deltas)(ctx, rw, c, current));
> +
> +    vp8_rac_unsigned(2, log2_token_partitions);
> +
> +    CHECK(FUNC(quantization_params)(ctx, rw, c, current));
> +
> +    if (current->frame_type != VP8_KEY_FRAME) {
> +        vp8_rac_unsigned(1, refresh_golden_frame);
> +        vp8_rac_unsigned(1, refresh_alternate_frame);
> +        if (!current->refresh_golden_frame)
> +            cbs_vp8_rac_get(c, 2);
> +        if (!current->refresh_alternate_frame)
> +            cbs_vp8_rac_get(c, 2);
> +        vp8_rac_unsigned(1, ref_frame_sign_bias_golden);
> +        vp8_rac_unsigned(1, ref_frame_sign_bias_alternate);
> +    }
> +    vp8_rac_unsigned(1, refresh_entropy_probs);
> +    if (current->frame_type != VP8_KEY_FRAME)
> +        vp8_rac_unsigned(1, refresh_last_frame);
> +
> +    CHECK(FUNC(update_token_probs)(ctx, rw, c, current));
> +
> +    vp8_rac_unsigned(1, mb_no_skip_coeff);
> +    if (current->mb_no_skip_coeff)
> +        vp8_rac_unsigned(8, prob_skip_false);
> +
> +    if (current->frame_type != VP8_KEY_FRAME) {
> +        int i;
> +
> +        vp8_rac_unsigned(8, prob_intra);
> +        vp8_rac_unsigned(8, prob_last);
> +        vp8_rac_unsigned(8, prob_golden);
> +
> +        // intra_16x16_prob
> +        if (cbs_vp8_rac_get(c, 1))
> +            for (i = 0; i < 4; i++)
> +                cbs_vp8_rac_get(c, 8);
> +
> +        // intra_chroma_prob
> +        if (cbs_vp8_rac_get(c, 1))
> +            for (i = 0; i < 4; i++)
> +                cbs_vp8_rac_get(c, 8);
> +
> +        CHECK(FUNC(update_mv_probs)(ctx, rw, c, current));
> +    }
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext *rw)
> +{
> +#ifdef READ
> +    int err;
> +
> +    while (byte_alignment(rw) != 0)
> +        fixed(1, zero_bit, 0);
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}
> +
> +static int FUNC(frame)(CodedBitstreamContext *ctx, RWContext *rw,
> +                       VP8RawFrame *current)
> +{
> +#ifdef READ
> +    int err;
> +
> +    HEADER("Frame");
> +
> +    CHECK(FUNC(frame_tag)(ctx, rw, &current->header));
> +    CHECK(FUNC(frame_header)(ctx, rw, &current->header));
> +    CHECK(FUNC(trailing_bits)(ctx, rw));
> +
> +    return 0;
> +#else
> +    return AVERROR_PATCHWELCOME;
> +#endif
> +}

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Monday, May 15, 2023 4:10 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH V1] avcodec/cbs_vp8: Add support for VP8
> codec bitstream READ methods
> 
> Dai, Jianhui J:
> > This commit adds VP8 into cbs supported codec list, and enables the
> > `trace_headers` bitstream filters to support VP8, besides existing
> > AV1, H.264, H.265 and VP9. It can be useful to debug VP8 stream issues.
> >
> > Only the READ methods `read_unit` and `split_fragment` are
> > implemented, the WRITE methods `write_unit` and `assemble_fragment`
> > return `AVERROR_PATCHWELCOME` error code. It is because the CBS VP8
> > WRITE is unlikely used by any applications at the moment. The WRITE
> > methods can be added later if there are real requirments.
> >
> > TESTS: ffmpeg -i fate-suite/vp8/frame_size_change.webm -vcodec copy
> > -bsf:v trace_headers -f null -
> >
> > Signed-off-by: Jianhui Dai <jianhui.j.dai@intel.com>
> > ---
> >  configure                            |   4 +-
> >  libavcodec/Makefile                  |   1 +
> >  libavcodec/cbs.c                     |   6 +
> >  libavcodec/cbs_internal.h            |   1 +
> >  libavcodec/cbs_vp8.c                 | 259 +++++++++++++++++++++++
> >  libavcodec/cbs_vp8.h                 | 116 +++++++++++
> >  libavcodec/cbs_vp8_syntax_template.c | 294
> > +++++++++++++++++++++++++++
> >  7 files changed, 680 insertions(+), 1 deletion(-)  create mode 100644
> > libavcodec/cbs_vp8.c  create mode 100644 libavcodec/cbs_vp8.h  create
> > mode 100644 libavcodec/cbs_vp8_syntax_template.c
> >
> > diff --git a/configure b/configure
> > index bb7be67676..6ba7759e34 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2433,6 +2433,7 @@ CONFIG_EXTRA="
> >      cbs_jpeg
> >      cbs_mpeg2
> >      cbs_vp9
> > +    cbs_vp8
> 
> vp8 should be before vp9 in all these lists.
 
Thank for your review. ACK.

> 
> >      deflate_wrapper
> >      dirac_parse
> >      dnn
> > @@ -2714,6 +2715,7 @@ cbs_h265_select="cbs"
> >  cbs_jpeg_select="cbs"
> >  cbs_mpeg2_select="cbs"
> >  cbs_vp9_select="cbs"
> > +cbs_vp8_select="cbs"
> >  dct_select="rdft"
> >  deflate_wrapper_deps="zlib"
> >  dirac_parse_select="golomb"
> > @@ -3284,7 +3286,7 @@ h264_redundant_pps_bsf_select="cbs_h264"
> >  hevc_metadata_bsf_select="cbs_h265"
> >  mjpeg2jpeg_bsf_select="jpegtables"
> >  mpeg2_metadata_bsf_select="cbs_mpeg2"
> > -trace_headers_bsf_select="cbs"
> > +trace_headers_bsf_select="cbs cbs_vp8"
> >  vp9_metadata_bsf_select="cbs_vp9"
> >
> >  # external libraries
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile index
> > 3cf4444b7e..a4e5d7d7a5 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -79,6 +79,7 @@ OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o
> cbs_sei.o h2645_parse.o
> >  OBJS-$(CONFIG_CBS_JPEG)                += cbs_jpeg.o
> >  OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
> >  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> > +OBJS-$(CONFIG_CBS_VP8)                 += cbs_vp8.o vpx_rac.o
> >  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
> >  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
> >  OBJS-$(CONFIG_DEFLATE_WRAPPER)         += zlib_wrapper.o
> > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index
> > 504197e06d..f4ff05181f 100644
> > --- a/libavcodec/cbs.c
> > +++ b/libavcodec/cbs.c
> > @@ -49,6 +49,9 @@ static const CodedBitstreamType *const
> > cbs_type_table[] = {  #if CONFIG_CBS_VP9
> >      &ff_cbs_type_vp9,
> >  #endif
> > +#if CONFIG_CBS_VP8
> > +    &ff_cbs_type_vp8,
> > +#endif
> >  };
> >
> >  const enum AVCodecID ff_cbs_all_codec_ids[] = { @@ -69,6 +72,9 @@
> > const enum AVCodecID ff_cbs_all_codec_ids[] = {  #endif  #if
> > CONFIG_CBS_VP9
> >      AV_CODEC_ID_VP9,
> > +#endif
> > +#if CONFIG_CBS_VP8
> > +    AV_CODEC_ID_VP8,
> >  #endif
> >      AV_CODEC_ID_NONE
> >  };
> > diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> > index e585c77934..9ea6393a9f 100644
> > --- a/libavcodec/cbs_internal.h
> > +++ b/libavcodec/cbs_internal.h
> > @@ -248,6 +248,7 @@ extern const CodedBitstreamType ff_cbs_type_h265;
> > extern const CodedBitstreamType ff_cbs_type_jpeg;  extern const
> > CodedBitstreamType ff_cbs_type_mpeg2;  extern const CodedBitstreamType
> > ff_cbs_type_vp9;
> > +extern const CodedBitstreamType ff_cbs_type_vp8;
> >
> >
> >  #endif /* AVCODEC_CBS_INTERNAL_H */
> > diff --git a/libavcodec/cbs_vp8.c b/libavcodec/cbs_vp8.c new file mode
> > 100644 index 0000000000..ee5c5cbceb
> > --- /dev/null
> > +++ b/libavcodec/cbs_vp8.c
> > @@ -0,0 +1,259 @@
> > +/*
> > + * 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 "libavutil/avassert.h"
> > +
> > +#include "cbs.h"
> > +#include "cbs_internal.h"
> > +#include "cbs_vp8.h"
> > +
> > +#include "vp8data.h"
> > +
> > +static av_unused int
> > +cbs_vp8_trace_syntax_element(CodedBitstreamContext *ctx,
> 
> Why is this marked as av_unused?

ACK

> 
> > +                                                  int pos, const char *name,
> > +                                                  const int *subscripts,
> > +                                                  int32_t value, int
> > +width) {
> > +    if (ctx->trace_enable) {
> > +        char bits[33] = "-";
> > +
> > +        av_assert0(width >= 0 && width <= 24);
> > +        if (width > 0) {
> > +            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, pos, name, subscripts, bits, value);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int cbs_vp8_read(CodedBitstreamContext *ctx, GetBitContext *gbc,
> > +                        int width, int32_t *write_to) {
> > +    uint32_t value;
> > +
> > +    av_assert0(width > 0 && width <= 24);
> > +
> > +    if (get_bits_left(gbc) < width + 1) {
> 
> Why the +1?

ACK

> 
> > +        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid value: bitstream
> ended.\n");
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    value     = get_bits_le(gbc, width);
> 
> You are reading from a bitstream reader that has not been opened as little-
> endian. This will work, but it is not how it is supposed to be.

ACK

> 
> > +    *write_to = value;
> > +    return 0;
> > +}
> > +
> > +static av_unused int cbs_vp8_rac_get(VPXRangeCoder *c, int bits) {
> > +    int value = 0;
> > +
> > +    while (bits--)
> > +        value = (value << 1) | vpx_rac_get_prob(c, 128);
> > +
> > +    return value;
> > +}
> > +
> > +static int cbs_vp8_rac_get_bits_count(GetBitContext *gbc,
> > +VPXRangeCoder *c) {
> > +    int bits_count = (c->buffer - gbc->buffer) * 8;
> > +    bits_count += c->bits;
> 
> Your mixing of VPXRangeCoder and GetBitContext is incredibly hacky; if I see
> this correctly, then you are not even advancing the GetBitContext position for all
> the rac stuff that you are reading (meaning that the byte-alignment code in
> trailing_bits() is nonsense).

Right.
VP8 bitstream comprise 3 parts: Frame Tag (uncompressed header), Frame Header (Boolean Entropy Encoder) and MBs.
The `Frame Tag` is parsed by GetBitContext, ` Frame Header` is parsed by VPXRangeCoder. 
I should check trailing_bits() after FrameHeader. Will fix it

> Furthermore, the rac functions do not report when they run out of input, as is
> common for CBS. You should probably write CBS versions of them that closely
> follow the spec (read: that need not be optimized for speed).

I would like the cbs_vp8 to be lightweight, that reuse existing VPXRangeCoder.
I will check and fix it. 

> 
> > +    return bits_count;
> > +}
> > +
> > +#define HEADER(name) \
> > +    do { \
> > +        ff_cbs_trace_header(ctx, name); \
> > +    } while (0)
> > +
> > +#define CHECK(call) \
> > +    do { \
> > +        err = (call); \
> > +        if (err < 0) \
> > +            return err; \
> > +    } while (0)
> > +
> > +#define FUNC_NAME(rw, codec, name) cbs_##codec##_##rw##_##name
> > +#define FUNC_VP8(rw, name) FUNC_NAME(rw, vp8, name) #define
> > +FUNC(name) FUNC_VP8(READWRITE, name)
> > +
> > +#define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs,
> > +__VA_ARGS__ }) : NULL)
> > +
> > +#define vp8_f(width, name) vp8_xf(width, name, current->name)
> > +
> > +#define vp8_rac_unsigned(width, name) \
> > +    do { \
> > +        uint32_t value; \
> > +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> > +        value   = cbs_vp8_rac_get(c, width); \
> > +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, 0); \
> 
> IMO it is better to have a function cbs_vp8_rac_read_unsigned() for this;
> vp8_rac_unsigned and vp8_rac_unsigned_subs would then just be wrappers
> around this function like it is done for xs() in cbs_vp9.

ACK

> 
> > +        current->name = value; \
> > +    } while (0)
> > +
> > +#define vp8_rac_unsigned_subs(width, name, subs, ...) \
> > +    do { \
> > +        uint32_t value; \
> > +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> > +        value   = cbs_vp8_rac_get(c, width); \
> > +        cbs_vp8_trace_syntax_element(ctx, pos, #name, \
> > +                                     SUBSCRIPTS(subs, __VA_ARGS__), value, 0); \
> > +        current->name = value; \
> > +    } while (0)
> > +
> > +#define vp8_rac_signed(width, name) \
> > +    do { \
> > +        uint32_t value; \
> 
> Why is this not signed? Same below.

ACK

> 
> > +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> > +        value   = cbs_vp8_rac_get(c, width); \
> > +        if (cbs_vp8_rac_get(c, 1)) \
> > +            value = -value; \
> > +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, 0); \
> 
> Same as above for vp8_rac_unsigned.

ACK

> 
> > +        current->name = value; \
> > +    } while (0)
> > +
> > +#define vp8_rac_signed_subs(width, name, subs, ...) \
> > +    do { \
> > +        uint32_t value; \
> > +        int pos = cbs_vp8_rac_get_bits_count(rw, c); \
> > +        value   = cbs_vp8_rac_get(c, width); \
> > +        if (cbs_vp8_rac_get(c, 1)) \
> > +            value = -value; \
> > +        cbs_vp8_trace_syntax_element(ctx, pos, #name, \
> > +                                     SUBSCRIPTS(subs, __VA_ARGS__), value, 0); \
> > +        current->name = value; \
> > +    } while (0)
> > +
> > +#define READ
> > +#define READWRITE read
> > +#define RWContext GetBitContext
> > +
> > +#define vp8_xf(width, name, var) \
> > +    do { \
> > +        uint32_t value; \
> > +        int pos = get_bits_count(rw); \
> > +        CHECK(cbs_vp8_read(ctx, rw, width, &value)); \
> > +        var = value; \
> > +        cbs_vp8_trace_syntax_element(ctx, pos, #name, 0, value, width); \
> > +    } while (0)
> > +
> > +#define fixed(width, name, value) \
> > +    do { \
> > +        av_unused uint32_t fixed_value; \
> > +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, 0, &fixed_value, \
> > +                                   value, value)); \
> > +    } while (0)
> > +
> > +#define byte_alignment(rw) (get_bits_count(rw) % 8)
> > +
> > +#include "cbs_vp8_syntax_template.c"
> > +
> > +static int cbs_vp8_split_fragment(CodedBitstreamContext *ctx,
> > +                                  CodedBitstreamFragment *frag, int
> > +header) {
> > +    int err;
> > +
> > +    if (frag->data_size == 0)
> > +        return AVERROR_INVALIDDATA;
> > +
> > +    err = ff_cbs_append_unit_data(frag, 0, frag->data, frag->data_size,
> > +                                  frag->data_ref);
> > +    if (err < 0)
> > +        return err;
> > +
> > +    return 0;
> > +}
> > +
> > +static int cbs_vp8_read_unit(CodedBitstreamContext *ctx,
> > +                             CodedBitstreamUnit *unit) {
> > +    VP8RawFrame *frame;
> > +    GetBitContext gbc;
> > +    int err, pos;
> > +
> > +    err = init_get_bits(&gbc, unit->data, 8 * unit->data_size);
> > +    if (err < 0)
> > +        return err;
> > +
> > +    err = ff_cbs_alloc_unit_content(ctx, unit);
> > +    if (err < 0)
> > +        return err;
> > +    frame = unit->content;
> > +
> > +    err = cbs_vp8_read_frame(ctx, &gbc, frame);
> > +    if (err < 0)
> > +        return err;
> > +
> > +    pos = get_bits_count(&gbc);
> > +    av_assert0(pos % 8 == 0);
> > +    pos /= 8;
> > +    av_assert0(pos <= unit->data_size);
> > +
> > +    if (pos == unit->data_size)
> > +        return AVERROR_INVALIDDATA;
> > +
> > +    frame->data_ref = av_buffer_ref(unit->data_ref);
> > +    if (!frame->data_ref)
> > +        return AVERROR(ENOMEM);
> > +
> > +    frame->data      = unit->data + pos;
> > +    frame->data_size = unit->data_size - pos;
> > +
> > +    return 0;
> > +}
> > +
> > +static int cbs_vp8_write_unit(CodedBitstreamContext *ctx,
> > +                              CodedBitstreamUnit *unit, PutBitContext
> > +*pbc) {
> > +    return AVERROR_PATCHWELCOME;
> > +}
> > +
> > +static int cbs_vp8_assemble_fragment(CodedBitstreamContext *ctx,
> > +                                     CodedBitstreamFragment *frag) {
> > +    return AVERROR_PATCHWELCOME;
> > +}
> > +
> > +static void cbs_vp8_flush(CodedBitstreamContext *ctx) {
> > +    return;
> > +}
> 
> No need for a useless flush. Just leave the function empty.
> (One could also do the same for the write_unit and assemble_fragment
> function: Nothing should call these functions at all.)

ACK

> 
> > +
> > +static const CodedBitstreamUnitTypeDescriptor cbs_vp8_unit_types[] = {
> > +    CBS_UNIT_TYPE_INTERNAL_REF(0, VP8RawFrame, data),
> > +    CBS_UNIT_TYPE_END_OF_LIST
> > +};
> > +
> > +const CodedBitstreamType ff_cbs_type_vp8 = {
> > +    .codec_id          = AV_CODEC_ID_VP8,
> > +
> > +    .priv_data_size    = sizeof(CodedBitstreamVP8Context),
> > +
> > +    .unit_types        = cbs_vp8_unit_types,
> > +
> > +    .split_fragment    = &cbs_vp8_split_fragment,
> > +    .read_unit         = &cbs_vp8_read_unit,
> > +    .write_unit        = &cbs_vp8_write_unit,
> > +
> > +    .flush             = &cbs_vp8_flush,
> > +
> > +    .assemble_fragment = &cbs_vp8_assemble_fragment, };
> > diff --git a/libavcodec/cbs_vp8.h b/libavcodec/cbs_vp8.h new file mode
> > 100644 index 0000000000..176d95853b
> > --- /dev/null
> > +++ b/libavcodec/cbs_vp8.h
> > @@ -0,0 +1,116 @@
> > +/*
> > + * 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_VP8_H
> > +#define AVCODEC_CBS_VP8_H
> > +
> > +#include <stddef.h>
> > +#include <stdint.h>
> > +
> > +#include "cbs.h"
> > +#include "vpx_rac.h"
> > +
> > +enum {
> > +    VP8_START_CODE_0 = 0x9D,
> > +    VP8_START_CODE_1 = 0x01,
> > +    VP8_START_CODE_2 = 0x2A,
> > +};
> > +
> > +enum {
> > +    VP8_KEY_FRAME     = 0,
> > +    VP8_NON_KEY_FRAME = 1,
> > +};
> > +
> > +typedef struct VP8RawFrameHeader {
> > +    // frame tag
> > +    uint8_t frame_type;
> > +    uint8_t profile;
> > +    uint8_t show_frame;
> > +    uint32_t first_partition_length_in_bytes;
> > +
> > +    uint32_t start_code_0;
> > +    uint32_t start_code_1;
> > +    uint32_t start_code_2;
> > +
> > +    uint16_t width;
> > +    uint16_t horizontal_scale;
> > +    uint16_t height;
> > +    uint16_t vertical_scale;
> > +
> > +    // frame header
> > +    uint8_t color_space;
> > +    uint8_t clamping_type;
> > +
> > +    // segmentation
> > +    uint8_t segmentation_enabled;
> > +    uint8_t update_segment_map;
> > +    uint8_t update_segment_feature_data;
> > +    uint8_t segment_feature_mode;
> > +    int8_t segment_update_qp[4];
> > +    int8_t segment_update_loop_filter_level[4];
> > +    uint8_t segment_update_probs[3];
> > +
> > +    // loop filter
> > +    uint8_t loop_filter_type;
> > +    uint8_t loop_filter_level;
> > +    uint8_t loop_filter_sharpness;
> > +    uint8_t mode_ref_lf_delta_enabled;
> > +    int8_t ref_lf_deltas[4];
> > +    int8_t mode_lf_deltas[4];
> > +
> > +    uint8_t log2_token_partitions;
> > +
> > +    // qp
> > +    uint8_t base_qindex;
> > +    int8_t y1dc_delta_q;
> > +    int8_t y2dc_delta_q;
> > +    int8_t y2ac_delta_q;
> > +    int8_t uvdc_delta_q;
> > +    int8_t uvac_delta_q;
> > +
> > +    // ref
> > +    uint8_t refresh_golden_frame;
> > +    uint8_t refresh_alternate_frame;
> > +    uint8_t ref_frame_sign_bias_golden;
> > +    uint8_t ref_frame_sign_bias_alternate;
> > +    uint8_t refresh_last_frame;
> > +
> > +    // token probs
> > +    uint8_t refresh_entropy_probs;
> > +
> > +    uint8_t mb_no_skip_coeff;
> > +    uint8_t prob_skip_false;
> > +
> > +    uint8_t prob_intra;
> > +    uint8_t prob_last;
> > +    uint8_t prob_golden;
> > +} VP8RawFrameHeader;
> > +
> > +typedef struct VP8RawFrame {
> > +    VP8RawFrameHeader header;
> > +
> > +    uint8_t *data;
> > +    AVBufferRef *data_ref;
> > +    size_t data_size;
> > +} VP8RawFrame;
> > +
> > +typedef struct CodedBitstreamVP8Context {
> > +    VPXRangeCoder c;
> > +} CodedBitstreamVP8Context;
> > +
> > +#endif /* AVCODEC_CBS_VP8_H */
> > diff --git a/libavcodec/cbs_vp8_syntax_template.c
> > b/libavcodec/cbs_vp8_syntax_template.c
> > new file mode 100644
> > index 0000000000..24f4075dab
> > --- /dev/null
> > +++ b/libavcodec/cbs_vp8_syntax_template.c
> > @@ -0,0 +1,294 @@
> > +/*
> > + * 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  */
> > +
> > +static int FUNC(update_segmentation)(CodedBitstreamContext *ctx,
> RWContext *rw,
> > +                                     VPXRangeCoder *c,
> > +                                     VP8RawFrameHeader *current) {
> > +#ifdef READ
> > +    int i;
> 
> We allow (and prefer) C99 for loops with variable declarations.

ACK

> 
> > +
> > +    vp8_rac_unsigned(1, update_segment_map);
> > +    vp8_rac_unsigned(1, update_segment_feature_data);
> > +
> > +    if (current->update_segment_feature_data) {
> > +        vp8_rac_unsigned(1, segment_feature_mode);
> > +        // quantizer
> > +        for (i = 0; i < 4; i++)
> > +            if (cbs_vp8_rac_get(c, 1))
> > +                vp8_rac_signed_subs(7, segment_update_qp[i], 1, i);
> > +        // loop filter
> > +        for (i = 0; i < 4; i++) {
> > +            if (cbs_vp8_rac_get(c, 1)) {
> > +                vp8_rac_signed_subs(6, segment_update_loop_filter_level[i], 1,
> > +                                    i);
> > +            }
> > +        }
> > +    }
> > +
> > +    if (current->update_segment_map) {
> > +        for (i = 0; i < 3; i++)
> > +            if (cbs_vp8_rac_get(c, 1))
> > +                vp8_rac_unsigned_subs(8, segment_update_probs[i], 1, i);
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> 
> IMO there is no need for these #else branches given that this file is not included
> for anything but reading.

The intention is that possible we want the cbs_vp8 WRITE methods in near future.
If the WRITE methods are unlikely needed, I will remove it.

> 
> > +}
> > +
> > +static int FUNC(mode_ref_lf_deltas)(CodedBitstreamContext *ctx,
> > +                                           RWContext *rw, VPXRangeCoder *c,
> > +                                           VP8RawFrameHeader
> > +*current) { #ifdef READ
> > +    vp8_rac_unsigned(1, mode_ref_lf_delta_enabled);
> > +    if (current->mode_ref_lf_delta_enabled) {
> > +        if (cbs_vp8_rac_get(c, 1)) {
> > +            int i;
> > +            // ref_lf_deltas
> > +            for (i = 0; i < 4; i++)
> > +                if (cbs_vp8_rac_get(c, 1))
> > +                    vp8_rac_signed_subs(6, ref_lf_deltas[i], 1, i);
> > +            // mode_lf_deltas
> > +            for (i = 0; i < 4; i++)
> > +                if (cbs_vp8_rac_get(c, 1))
> > +                    vp8_rac_signed_subs(6, mode_lf_deltas[i], 1, i);
> > +        }
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(quantization_params)(CodedBitstreamContext *ctx,
> RWContext *rw,
> > +                                   VPXRangeCoder *c,
> > +VP8RawFrameHeader *current) { #ifdef READ
> > +    vp8_rac_unsigned(7, base_qindex);
> > +
> > +    if (cbs_vp8_rac_get(c, 1))
> > +        vp8_rac_signed(4, y1dc_delta_q);
> > +
> > +    if (cbs_vp8_rac_get(c, 1))
> > +        vp8_rac_signed(4, y2dc_delta_q);
> > +
> > +    if (cbs_vp8_rac_get(c, 1))
> > +        vp8_rac_signed(4, y2ac_delta_q);
> > +
> > +    if (cbs_vp8_rac_get(c, 1))
> > +        vp8_rac_signed(4, uvdc_delta_q);
> > +
> > +    if (cbs_vp8_rac_get(c, 1))
> > +        vp8_rac_signed(4, uvac_delta_q);
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(update_token_probs)(CodedBitstreamContext *ctx,
> RWContext *rw,
> > +                                    VPXRangeCoder *c,
> > +                                    VP8RawFrameHeader *current) {
> > +#ifdef READ
> > +    int i, j, k, l;
> > +
> > +    for (i = 0; i < 4; ++i) {
> > +        for (j = 0; j < 8; ++j) {
> > +            for (k = 0; k < 3; ++k) {
> > +                for (l = 0; l < 11; ++l)
> > +                    if (vpx_rac_get_prob_branchy(
> > +                            c, vp8_token_update_probs[i][j][k][l]))
> > +                        cbs_vp8_rac_get(c, 8);
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(update_mv_probs)(CodedBitstreamContext *ctx, RWContext
> *rw,
> > +                                 VPXRangeCoder *c, VP8RawFrameHeader
> > +*current) { #ifdef READ
> > +    int i, j;
> > +
> > +    for (i = 0; i < 2; ++i) {
> > +        for (j = 0; j < 19; ++j)
> > +            if (cbs_vp8_rac_get(c, 1))
> > +                cbs_vp8_rac_get(c, 7);
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(frame_tag)(CodedBitstreamContext *ctx, RWContext *rw,
> > +                           VP8RawFrameHeader *current) { #ifdef READ
> > +    int err;
> > +
> > +    vp8_f(1, frame_type);
> > +    vp8_f(3, profile);
> > +    vp8_f(1, show_frame);
> > +    vp8_f(19, first_partition_length_in_bytes);
> > +
> > +    if (current->frame_type == VP8_KEY_FRAME) {
> > +        fixed(8, start_code_0, VP8_START_CODE_0);
> > +        fixed(8, start_code_1, VP8_START_CODE_1);
> > +        fixed(8, start_code_2, VP8_START_CODE_2);
> > +
> > +        vp8_f(14, width);
> > +        vp8_f(2, horizontal_scale);
> > +        vp8_f(14, height);
> > +        vp8_f(2, vertical_scale);
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(frame_header)(CodedBitstreamContext *ctx, RWContext *rw,
> > +                              VP8RawFrameHeader *current) { #ifdef
> > +READ
> > +    CodedBitstreamVP8Context *vp8 = ctx->priv_data;
> > +    VPXRangeCoder *c              = &vp8->c;
> > +    int header_size;
> > +    int pos;
> > +    int err;
> > +
> > +    pos = get_bits_count(rw);
> > +    av_assert0(pos % 8 == 0);
> > +    pos /= 8;
> > +
> > +    header_size = current->first_partition_length_in_bytes;
> > +
> > +    err = ff_vpx_init_range_decoder(c, rw->buffer + pos, header_size);
> > +    if (err < 0)
> > +        return err;
> > +
> > +    if (current->frame_type == VP8_KEY_FRAME) {
> > +        vp8_rac_unsigned(1, color_space);
> > +        vp8_rac_unsigned(1, clamping_type);
> > +    }
> > +
> > +    vp8_rac_unsigned(1, segmentation_enabled);
> > +    if (current->segmentation_enabled)
> > +        CHECK(FUNC(update_segmentation)(ctx, rw, c, current));
> > +
> > +    vp8_rac_unsigned(1, loop_filter_type);
> > +    vp8_rac_unsigned(6, loop_filter_level);
> > +    vp8_rac_unsigned(3, loop_filter_sharpness);
> > +
> > +    CHECK(FUNC(mode_ref_lf_deltas)(ctx, rw, c, current));
> > +
> > +    vp8_rac_unsigned(2, log2_token_partitions);
> > +
> > +    CHECK(FUNC(quantization_params)(ctx, rw, c, current));
> > +
> > +    if (current->frame_type != VP8_KEY_FRAME) {
> > +        vp8_rac_unsigned(1, refresh_golden_frame);
> > +        vp8_rac_unsigned(1, refresh_alternate_frame);
> > +        if (!current->refresh_golden_frame)
> > +            cbs_vp8_rac_get(c, 2);
> > +        if (!current->refresh_alternate_frame)
> > +            cbs_vp8_rac_get(c, 2);
> > +        vp8_rac_unsigned(1, ref_frame_sign_bias_golden);
> > +        vp8_rac_unsigned(1, ref_frame_sign_bias_alternate);
> > +    }
> > +    vp8_rac_unsigned(1, refresh_entropy_probs);
> > +    if (current->frame_type != VP8_KEY_FRAME)
> > +        vp8_rac_unsigned(1, refresh_last_frame);
> > +
> > +    CHECK(FUNC(update_token_probs)(ctx, rw, c, current));
> > +
> > +    vp8_rac_unsigned(1, mb_no_skip_coeff);
> > +    if (current->mb_no_skip_coeff)
> > +        vp8_rac_unsigned(8, prob_skip_false);
> > +
> > +    if (current->frame_type != VP8_KEY_FRAME) {
> > +        int i;
> > +
> > +        vp8_rac_unsigned(8, prob_intra);
> > +        vp8_rac_unsigned(8, prob_last);
> > +        vp8_rac_unsigned(8, prob_golden);
> > +
> > +        // intra_16x16_prob
> > +        if (cbs_vp8_rac_get(c, 1))
> > +            for (i = 0; i < 4; i++)
> > +                cbs_vp8_rac_get(c, 8);
> > +
> > +        // intra_chroma_prob
> > +        if (cbs_vp8_rac_get(c, 1))
> > +            for (i = 0; i < 4; i++)
> > +                cbs_vp8_rac_get(c, 8);
> > +
> > +        CHECK(FUNC(update_mv_probs)(ctx, rw, c, current));
> > +    }
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext
> > +*rw) { #ifdef READ
> > +    int err;
> > +
> > +    while (byte_alignment(rw) != 0)
> > +        fixed(1, zero_bit, 0);
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> > +
> > +static int FUNC(frame)(CodedBitstreamContext *ctx, RWContext *rw,
> > +                       VP8RawFrame *current) { #ifdef READ
> > +    int err;
> > +
> > +    HEADER("Frame");
> > +
> > +    CHECK(FUNC(frame_tag)(ctx, rw, &current->header));
> > +    CHECK(FUNC(frame_header)(ctx, rw, &current->header));
> > +    CHECK(FUNC(trailing_bits)(ctx, rw));
> > +
> > +    return 0;
> > +#else
> > +    return AVERROR_PATCHWELCOME;
> > +#endif
> > +}
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".