[FFmpeg-devel] [PATCH 2/2] avcodec: add bink2 video decoder
Peter Ross
pross at xvid.org
Sun Apr 7 02:15:32 EEST 2019
On Wed, Mar 27, 2019 at 09:21:47PM +0100, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
> Missing deblocking.
> ---
> configure | 1 +
> libavcodec/Makefile | 1 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/avcodec.h | 1 +
> libavcodec/bink2.c | 787 +++++++++++++++++++++++
> libavcodec/bink2f.c | 1139 +++++++++++++++++++++++++++++++++
> libavcodec/bink2g.c | 1342 +++++++++++++++++++++++++++++++++++++++
> libavcodec/codec_desc.c | 7 +
> libavformat/bink.c | 3 +-
my comments below.
this is a mammoth amount of work deserving of a better quality review.
> +++ b/libavcodec/bink2.c
> @@ -0,0 +1,787 @@
> +/*
> + * Bink video 2 decoder
> + * Copyright (c) 2014 Konstantin Shishkov
> + * Copyright (c) 2019 Paul B Mahol
> + *
> +static const uint8_t luma_repos[] = {
> + 0, 1, 4, 5, 2, 3, 6, 7, 8, 9, 12, 13, 10, 11, 14, 15,
> +};
identical to msvideo1enc.c remap
consider moving to libavcodec/mathops.h
> +static const int32_t bink2g_dc_pat[] = {
> + 1024, 1218, 1448, 1722, 2048,
> + 2435, 2896, 3444, 4096, 4871,
> + 5793, 6889, 8192, 9742, 11585, 13777, 16384,
> + 19484, 23170, 27555, 32768, 38968, 46341,
> + 55109, 65536, 77936, 92682, 110218, 131072,
> + 155872, 185364, 220436, 262144, 311744,
> + 370728, 440872, 524288,
> +};
this is so close to ff_dirac_qscale_tab it is not funny.
> + 3, 24, 11, 18, 25, 13, 14, 4,
> + 15, 5, 6, 7, 12, 19, 20, 21,
> + 22, 23, 26, 27, 28, 29, 30, 31,
> + 32, 33, 34, 35, 36, 37, 38, 39,
> + 40, 41, 42, 43, 44, 45, 46, 47,
> + 48, 49, 50, 51, 52, 53, 54, 55,
> + 56, 57, 58, 59, 60, 61, 62, 63
> +};
> +
> +static const uint8_t bink2g_scan[64] = {
> + 0, 8, 1, 2, 9, 16, 24, 17,
> + 10, 3, 4, 11, 18, 25, 32, 40,
> + 33, 26, 19, 12, 5, 6, 13, 20,
> + 27, 34, 41, 48, 56, 49, 42, 35,
> + 28, 21, 14, 7, 15, 22, 29, 36,
> + 43, 50, 57, 58, 51, 44, 37, 30,
> + 23, 31, 38, 45, 52, 59, 60, 53,
> + 46, 39, 47, 54, 61, 62, 55, 63,
> +};
this table has more white space than the other scan tables...
> +enum BlockTypes {
> + INTRA_BLOCK = 0, ///< intra DCT block
> + SKIP_BLOCK, ///< skipped block
> + MOTION_BLOCK, ///< block is copied from previous frame with some offset
> + RESIDUE_BLOCK, ///< motion block with some difference added
> +};
> +
> +static const uint8_t ones_count[16] = {
> + 0, 1, 1, 2, 1, 2, 2, 3, 1, 2, 2, 3, 2, 3, 3, 4
> +};
duplicate of libavcodec/vc1_mc.c popcount4
> +static int bink2_decode_frame(AVCodecContext *avctx, void *data,
> + int *got_frame, AVPacket *pkt)
> +{
> + Bink2Context * const c = avctx->priv_data;
> + GetBitContext *gb = &c->gb;
> + AVFrame *frame = data;
> + uint8_t *dst[4];
> + uint8_t *src[4];
> + int stride[4];
> + int sstride[4];
> + uint32_t off = 0;
> + int is_kf = !!(pkt->flags & AV_PKT_FLAG_KEY);
> + int ret, w, h;
> + int height_a;
> +
> + w = avctx->width;
> + h = avctx->height;
> + ret = ff_set_dimensions(avctx, FFALIGN(w, 32), FFALIGN(h, 32));
> + if (ret < 0)
> + return ret;
> + avctx->width = w;
> + avctx->height = h;
> +
> + if ((ret = ff_get_buffer(avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0)
> + return ret;
ret error handling style is inconsistent
> +static int bink2f_decode_inter_luma(Bink2Context *c,
> + float block[4][64],
> + unsigned *prev_cbp, int *prev_q,
> + uint8_t *dst, int stride,
> + int flags)
> +{
> + GetBitContext *gb = &c->gb;
> + float *dc = c->current_dc[c->mb_pos].dc[c->comp];
> + unsigned cbp;
> + int q, dq;
> +
> + *prev_cbp = cbp = bink2f_decode_cbp_luma(gb, *prev_cbp);
> + dq = bink2f_decode_delta_q(gb);
> + q = *prev_q + dq;
> + if (q < 0 || q >= 16)
> + return AVERROR_INVALIDDATA;
> + *prev_q = q;
> +
> + bink2f_decode_dc(c, gb, dc, 1, q, -1023, 1023, 0xA8);
> +
> + for (int i = 0; i < 4; i++) {
> + bink2f_decode_ac(gb, bink2f_luma_scan, block, cbp >> (i * 4),
> + bink2f_ac_quant[q], bink2f_luma_inter_qmat);
check error code?
> + for (int j = 0; j < 4; j++) {
> + block[j][0] = dc[i * 4 + j] * 0.125f;
> + bink2f_idct_add(dst + (luma_repos[i*4+j]&3) * 8 +
> + (luma_repos[i*4+j]>>2) * 8 * stride, stride,
> + block[j]);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int bink2f_decode_inter_chroma(Bink2Context *c,
> + float block[4][64],
> + unsigned *prev_cbp, int *prev_q,
> + uint8_t *dst, int stride,
> + int flags)
> +{
> + GetBitContext *gb = &c->gb;
> + float *dc = c->current_dc[c->mb_pos].dc[c->comp];
> + unsigned cbp;
> + int q, dq;
> +
> + *prev_cbp = cbp = bink2f_decode_cbp_chroma(gb, *prev_cbp);
> + dq = bink2f_decode_delta_q(gb);
> + q = *prev_q + dq;
> + if (q < 0 || q >= 16)
> + return AVERROR_INVALIDDATA;
> + *prev_q = q;
> +
> + bink2f_decode_dc(c, gb, dc, 0, q, -1023, 1023, 0xA8);
> +
> + bink2f_decode_ac(gb, bink2f_chroma_scan, block, cbp,
> + bink2f_ac_quant[q], bink2f_chroma_qmat);
ditto.
> +
> + for (int i = 0; i < 4; i++) {
> + block[i][0] = dc[i] * 0.125f;
> + bink2f_idct_add(dst + (i & 1) * 8 + (i >> 1) * 8 * stride, stride,
> + block[i]);
> + }
> +
> + return 0;
> +}
> +
some of the bink2 f/g functions are identical except for the input/output type.
i guess not much can be done about that as macros will make it more ugly.
> +static float bink2f_average_block(uint8_t *src, int stride)
> +{
> + int sum = 0;
> +
> + for (int i = 0; i < 8; i++) {
> + int avg_a = (src[i+0*stride] + src[i+1*stride] + 1) >> 1;
> + int avg_b = (src[i+2*stride] + src[i+3*stride] + 1) >> 1;
> + int avg_c = (src[i+4*stride] + src[i+5*stride] + 1) >> 1;
> + int avg_d = (src[i+6*stride] + src[i+7*stride] + 1) >> 1;
> + int avg_e = (avg_a + avg_b + 1) >> 1;
> + int avg_f = (avg_c + avg_d + 1) >> 1;
> + int avg_g = (avg_e + avg_f + 1) >> 1;
> + sum += avg_g;
> + }
> +
> + return sum;
> +}
> +
> +static void bink2f_average_chroma(Bink2Context *c, int x, int y,
> + uint8_t *src, int stride,
> + float *dc)
> +{
> + for (int i = 0; i < 4; i++) {
> + int X = i & 1;
> + int Y = i >> 1;
caps here makes actually it less readble.
> + dc[i] = bink2f_average_block(src + x + X * 8 + (y + Y * 8) * stride, stride);
> + }
> +}
> +static int bink2g_get_type(GetBitContext *gb, int *lru)
> +{
> + int val;
> +
> + ff_dlog(NULL, " type show %X @ %d\n", show_bits(gb, 4), get_bits_count(gb));
> + switch (get_unary(gb, 1, 3)) {
> + case 0:
> + val = lru[0];
> + break;
> + case 1:
> + val = lru[1];
> + FFSWAP(int, lru[0], lru[1]);
> + break;
> + case 2:
> + val = lru[3];
> + FFSWAP(int, lru[2], lru[3]);
> + break;
> + case 3:
> + val = lru[2];
> + FFSWAP(int, lru[1], lru[2]);
> + break;
> + }
cases 1-3 could be collapsed.
> +
> + return val;
> +}
> +
> +static int bink2g_decode_dq(GetBitContext *gb)
> +{
> + int dq = get_unary(gb, 1, 4);
> +
> + if (dq == 3)
> + dq += get_bits1(gb);
> + else if (dq == 4)
> + dq += get_bits(gb, 5) + 1;
> + if (dq && get_bits1(gb))
> + dq = -dq;
> +
> + return dq;
> +}
> +
> +typedef uint8_t byte;
> +typedef int8_t sbyte;
> +typedef unsigned uint;
but why?
> +static void bink2g_get_cbp_flags(GetBitContext *gb, int offset, int size, uint8_t *dst)
> +{
> + unsigned flag;
> + int i, j;
> + byte bVar4;
> + sbyte sVar5;
> + byte bVar6;
> + int iVar7;
> + int uVar8;
> + int uVar9;
> + int local_20;
> + int local_1c;
those variable names are not meaningful enough. in fact they look computer generated.
-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190407/cdfdf3f2/attachment.sig>
More information about the ffmpeg-devel
mailing list