On 5/30/16, Christophe Gisquet <christophe.gisq...@gmail.com> wrote:
> 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 ?
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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
