On 4/7/19, Peter Ross <pr...@xvid.org> wrote:
> On Wed, Mar 27, 2019 at 09:21:47PM +0100, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <one...@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.
I do not see how.
>> +
>> + 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.
Yes, that code needs some refactoring.
>
> -- Peter
> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
