[FFmpeg-devel] [PATCH] Bink Video decoder for FFmpeg 0.6

[Kostya]

On Tue, Feb 16, 2010 at 08:08:51PM +0100, Vitor Sessak wrote:
> Kostya wrote:
>> I've fixes some bugs and added some checks. This should be mature enough
>> to include it into upcoming FFmpeg release.
>
> Nice!
>
> Some comments:
>
>> P.S. Quant matrices have different quantizers so the biggest common
>> denominator is 1/6 which is quite inconvenient to scale single matrix,
>> thus whole 16 matrices instead.
>
> I don't understand this comment (maybe because I'm not very experienced  
> in video codecs), but I think it should be documented in a comment  
> somewhere in the code.

That's an answer for a question from Michael earlier.

>> +static int decode_frame(AVCodecContext *avctx, void *data, int *data_size, AVPacket *pkt)
>> +{
>> +    BinkContext * const c = avctx->priv_data;
>> +    GetBitContext gb;
>> +    int blk, bw, bh;
>> +    int i, j, plane, plane_idx, bx, by;
>> +    uint8_t *dst, *prev, *ref, *ref_start, *ref_end;
>> +    int v, col[2];
>> +    const uint8_t *scan;
>> +    const uint32_t *quant;
>> +    int xoff, yoff;
>> +    DECLARE_ALIGNED_16(DCTELEM, block[64]);
>> +
>> +    if(c->pic.data[0])
>> +        avctx->release_buffer(avctx, &c->pic);
>> +
>> +    if(avctx->get_buffer(avctx, &c->pic) < 0){
>> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>> +        return -1;
>> +    }
>> +
>> +    init_get_bits(&gb, pkt->data, pkt->size*8);
>> +    if (c->version >= 'i')
>> +        skip_bits_long(&gb, 32);
>
> Is a check for enough bits in the packet missing?

hmm, could be

>> +static av_cold int decode_init(AVCodecContext *avctx)
>> +{
>> +    BinkContext * const c = avctx->priv_data;
>> +    static VLC_TYPE table[16 * 128][2];
>> +    int i;
>> +
>> +    c->version = avctx->codec_tag >> 24;
>> +    if (c->version < 'c') {
>> +        av_log(avctx, AV_LOG_ERROR, "Too old version '%c'\n", c->version);
>> +        return -1;
>> +    }
>
> av_log_missing_feature()?

No, it needs quite different decoder for that, it just checks that wrong
data is not fed to this decoder.

>> +    if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) {
>> +        return 1;
>> +    }
>
> This is AFAIK checked in common code already.

I'll check, thanks.

> -Vitor