Hi,
2016-05-29 21:51 GMT+02:00 Paul B Mahol <one...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
