[FFmpeg-devel] [PATCH v3 1/5] libavcodec/vc2enc: Split out common functions between software and hardware encoders

[Andreas Rheinhardt]

IndecisiveTurtle:
> From: IndecisiveTurtle <geoster3d at gmail.com>
> 
> ---
>  libavcodec/Makefile        |   2 +-
>  libavcodec/vc2enc.c        | 513 +------------------------------------
>  libavcodec/vc2enc_common.c | 376 +++++++++++++++++++++++++++
>  libavcodec/vc2enc_common.h | 196 ++++++++++++++
>  4 files changed, 581 insertions(+), 506 deletions(-)
>  create mode 100644 libavcodec/vc2enc_common.c
>  create mode 100644 libavcodec/vc2enc_common.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 7bd1dbec9a..d4ebd86866 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -769,7 +769,7 @@ OBJS-$(CONFIG_VC1_CUVID_DECODER)       += cuviddec.o
>  OBJS-$(CONFIG_VC1_MMAL_DECODER)        += mmaldec.o
>  OBJS-$(CONFIG_VC1_QSV_DECODER)         += qsvdec.o
>  OBJS-$(CONFIG_VC1_V4L2M2M_DECODER)     += v4l2_m2m_dec.o
> -OBJS-$(CONFIG_VC2_ENCODER)             += vc2enc.o vc2enc_dwt.o diractab.o
> +OBJS-$(CONFIG_VC2_ENCODER)             += vc2enc.o vc2enc_dwt.o vc2enc_common.o diractab.o

Overlong line

>  OBJS-$(CONFIG_VCR1_DECODER)            += vcr1.o
>  OBJS-$(CONFIG_VMDAUDIO_DECODER)        += vmdaudio.o
>  OBJS-$(CONFIG_VMDVIDEO_DECODER)        += vmdvideo.o
> diff --git a/libavcodec/vc2enc_common.h b/libavcodec/vc2enc_common.h
> new file mode 100644
> index 0000000000..eaaf5ac99c
> --- /dev/null
> +++ b/libavcodec/vc2enc_common.h
> @@ -0,0 +1,196 @@
> +/*
> + * Copyright (C) 2016 Open Broadcast Systems Ltd.
> + * Author        2016 Rostislav Pehlivanov <atomnuker at gmail.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_VC2ENC_COMMON_H
> +#define AVCODEC_VC2ENC_COMMON_H
> +
> +#include "avcodec.h"
> +#include "dirac.h"
> +#include "put_bits.h"
> +
> +#include "vc2enc_dwt.h"
> +#include "diractab.h"
> +
> +/* The limited size resolution of each slice forces us to do this */
> +#define SSIZE_ROUND(b) (FFALIGN((b), s->size_scaler) + 4 + s->prefix_bytes)
> +
> +/* Decides the cutoff point in # of slices to distribute the leftover bytes */
> +#define SLICE_REDIST_TOTAL 150

Need not be put in a header.

> +
> +typedef struct VC2BaseVideoFormat {
> +    enum AVPixelFormat pix_fmt;
> +    AVRational time_base;
> +    int width, height;
> +    uint8_t interlaced, level;
> +    char name[13];
> +} VC2BaseVideoFormat;
> +
> +extern const VC2BaseVideoFormat base_video_fmts[];
> +extern const int base_video_fmts_len;
> +
> +enum VC2_QM {
> +    VC2_QM_DEF = 0,
> +    VC2_QM_COL,
> +    VC2_QM_FLAT,
> +
> +    VC2_QM_NB
> +};
> +
> +typedef struct SubBand {
> +    dwtcoef *buf;
> +    ptrdiff_t stride;
> +    int width;
> +    int height;
> +    int shift;
> +} SubBand;
> +
> +typedef struct Plane {
> +    SubBand band[MAX_DWT_LEVELS][4];
> +    dwtcoef *coef_buf;
> +    int width;
> +    int height;
> +    int dwt_width;
> +    int dwt_height;
> +    ptrdiff_t coef_stride;
> +} Plane;
> +
> +typedef struct SliceArgs {
> +    const struct VC2EncContext *ctx;
> +    union {
> +        int cache[DIRAC_MAX_QUANT_INDEX];
> +        uint8_t *buf;
> +    };
> +    int x;
> +    int y;
> +    int quant_idx;
> +    int bits_ceil;
> +    int bits_floor;
> +    int bytes;
> +} SliceArgs;
> +
> +typedef struct TransformArgs {
> +    const struct VC2EncContext *ctx;
> +    Plane *plane;
> +    const void *idata;
> +    ptrdiff_t istride;
> +    int field;
> +    VC2TransformContext t;
> +} TransformArgs;
> +
> +typedef struct VC2EncContext {
> +    AVClass *av_class;
> +    PutBitContext pb;
> +    Plane plane[3];
> +    AVCodecContext *avctx;
> +    DiracVersionInfo ver;
> +
> +    SliceArgs *slice_args;
> +    TransformArgs transform_args[3];
> +
> +    /* For conversion from unsigned pixel values to signed */
> +    int diff_offset;
> +    int bpp;
> +    int bpp_idx;
> +
> +    /* Picture number */
> +    uint32_t picture_number;
> +
> +    /* Base video format */
> +    int base_vf;
> +    int level;
> +    int profile;
> +
> +    /* Quantization matrix */
> +    uint8_t quant[MAX_DWT_LEVELS][4];
> +    int custom_quant_matrix;
> +
> +    /* Division LUT */
> +    uint32_t qmagic_lut[116][2];
> +
> +    int num_x; /* #slices horizontally */
> +    int num_y; /* #slices vertically */
> +    int prefix_bytes;
> +    int size_scaler;
> +    int chroma_x_shift;
> +    int chroma_y_shift;
> +
> +    /* Rate control stuff */
> +    int frame_max_bytes;
> +    int slice_max_bytes;
> +    int slice_min_bytes;
> +    int q_ceil;
> +    int q_avg;
> +
> +    /* Options */
> +    double tolerance;
> +    int wavelet_idx;
> +    int wavelet_depth;
> +    int strict_compliance;
> +    int slice_height;
> +    int slice_width;
> +    int interlaced;
> +    enum VC2_QM quant_matrix;
> +
> +    /* Parse code state */
> +    uint32_t next_parse_offset;
> +    enum DiracParseCodes last_parse_code;
> +} VC2EncContext;
> +
> +extern uint16_t interleaved_ue_golomb_tab[256];
> +extern uint16_t top_interleaved_ue_golomb_tab[256];
> +extern uint8_t golomb_len_tab[256];

Missing hidden visibility for all of this.

> +
> +static inline void put_vc2_ue_uint_inline(PutBitContext *pb, uint32_t val)
> +{
> +    uint64_t pbits = 1;
> +    int bits = 1;
> +
> +    ++val;
> +
> +    while (val >> 8) {
> +        pbits |= (uint64_t)interleaved_ue_golomb_tab[val & 0xff] << bits;
> +        val  >>= 8;
> +        bits  += 16;
> +    }
> +    pbits |= (uint64_t)top_interleaved_ue_golomb_tab[val] << bits;
> +    bits  += golomb_len_tab[val];
> +
> +    put_bits63(pb, bits, pbits);
> +}
> +
> +static inline int count_vc2_ue_uint(uint32_t val)
> +{
> +    return 2 * av_log2(val + 1) + 1;
> +}

This need not be put in a header at all.

> +
> +av_cold void vc2_init_static_data(void);

If you export such a function and not the corresponding AVOnce, then you
are most likely adding a race. And indeed you do in patch 5/5.

> +
> +void put_vc2_ue_uint(PutBitContext *pb, uint32_t val);
> +
> +void vc2_init_quant_matrix(VC2EncContext *s);
> +
> +void vc2_encode_parse_info(VC2EncContext *s, enum DiracParseCodes pcode);
> +
> +void vc2_encode_seq_header(VC2EncContext *s);
> +
> +void vc2_encode_picture_start(VC2EncContext *s);

1. Missing ff_ prefix.
2. These functions should not be exported, they should be static in
vc2enc_common.c and a new function that does what is currently being
done in lines 932-950 of vc2enc.c should be created instead and called
by both encoders.

> +
> +#endif