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

[Paul B Mahol]

On 5/30/16, Christophe Gisquet <christophe.gisquet at gmail.com> wrote:
> 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 ?

I added check for offsets locally before those are set.

>
>> +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.

Will add something.

>
>> +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.

Changed, doesn't help but maybe will for other archs.

>
>> +        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)

Our Huffyuv decoder is still missing gradient prediction...

>
>> +        } 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...

Just tried, didn't help much here.

>
>> +    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)

Moved.

>
> Otherwise, I don't see much of anything that would require another
> reviewing round.
> --
> Christophe
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>