[FFmpeg-devel] [PATCH] avcodec: add MagicYUV decoder

[Christophe Gisquet]

Hi,

2016-05-29 21:51 GMT+02:00 Paul B Mahol <onemda at gmail.com>:
> +typedef struct Slice {
> +    uint32_t start;
> +    uint32_t size;
> +} Slice;

I'm not a security expert, but is there a reason for not using plain int there ?

> +typedef struct MagicYUVContext {
> +    AVFrame            *p;
> +    int                 slice_height;
> +    int                 nb_slices;
> +    int                 planes;
> +    uint8_t             *buf;
> +    int                 hshift[4];
> +    int                 vshift[4];
> +    Slice               *slices[4];
> +    int                 slices_size[4];
> +    uint8_t             freq[4][256];
> +    VLC                 vlc[4];
> +    HuffYUVDSPContext   hdsp;
> +} MagicYUVContext;

I guess someone able to understand the code immediately understand
what those are, but that's pretty sparse comment-wise.

> +typedef struct HuffEntry {

And another Huffman+prediction codec... I don't really see any
valuable addition here... :(

> +    uint8_t  sym;
> +    uint8_t  len;
> +    uint32_t code;
> +} HuffEntry;
> +
> +static int ff_magy_huff_cmp_len(const void *a, const void *b)
> +{
> +    const HuffEntry *aa = a, *bb = b;
> +    return (aa->len - bb->len) * 256 + aa->sym - bb->sym;
> +}
> +
> +static int build_huff(VLC *vlc, uint8_t *freq)
> +{
> +    HuffEntry he[256];
> +    uint32_t codes[256];
> +    uint8_t bits[256];
> +    uint8_t syms[256];
> +    uint32_t code;
> +    int i, last;
> +
> +    for (i = 0; i < 256; i++) {
> +        he[i].sym = 255 - i;
> +        he[i].len = freq[i];
> +    }
> +    qsort(he, 256, sizeof(*he), ff_magy_huff_cmp_len);

ffmpeg seems to have libavutil/qsort.h, but I don't even know how much
effort is needed to use it here.

> +        pred = get_bits(&b, 8);
> +        dst = p->data[i] + j * sheight * stride;
> +        for (k = 0; k < height; k++) {
> +            for (x = 0; x < width; x++) {
> +                int pix;
> +                if (get_bits_left(&b) <= 0) {
> +                    return AVERROR_INVALIDDATA;
> +                }
> +                pix = get_vlc2(&b, s->vlc[i].table, s->vlc[i].bits, 3);
> +                if (pix < 0) {
> +                    return AVERROR_INVALIDDATA;
> +                }
> +                dst[x] = 255 - pix;
> +            }
> +            dst += stride;
> +        }
> +
> +        if (pred == LEFT) {
> +            dst = p->data[i] + j * sheight * stride;
> +            s->hdsp.add_hfyu_left_pred(dst, dst, width, 0);
> +            dst += stride;
> +            for (k = 1; k < height; k++) {
> +                s->hdsp.add_hfyu_left_pred(dst, dst, width, dst[-stride]);
> +                dst += stride;
> +            }
> +        } else if (pred == GRADIENT) {
[...]

That's somewhat similar to png paeth, except not actually reusable. I
wonder if there's something in libavcodec that could be reused, in
which case moving it to the hdsp context would be nice)

> +        } else if (pred == MEDIAN) {
[...]
> +        } else {

So, that's maybe a detail at this point, and you want to move quickly
to other stuff, but:
would you like to look at e.g. huffyuvdec or pngdec for a code that is
not as nice looking, but more cache-friendly?

Basically, you move the first line out of the loops, and then do
sequentially, per row in the loop, bitstream reading, reconstruction
(residual+prediction) and any post-processing...

> +    if (decorrelate) {
> +        uint8_t *b = p->data[0];
> +        uint8_t *g = p->data[1];
> +        uint8_t *r = p->data[2];
> +
> +        for (i = 0; i < p->height; i++) {
> +            for (x = 0; x < p->width; x++) {
> +                b[x] += g[x];
> +                r[x] += g[x];
> +            }
> +            b += p->linesize[0];
> +            g += p->linesize[1];
> +            r += p->linesize[2];
> +        }
> +    }

... in particular, this step, that could be done line-wise, inside the
threaded decoding, if I'm not mistaken. (cf. also png's deloco)

Otherwise, I don't see much of anything that would require another
reviewing round.
-- 
Christophe