[FFmpeg-devel] [PATCH] Implement optimal huffman encoding for (M)JPEG.

[Rostislav Pehlivanov]

On 28 December 2016 at 06:22, Jerry Jiang <jerryjiang1128 at gmail.com> wrote:

> -
> +{ "huffman", "Huffman table strategy", OFFSET(huffman), AV_OPT_TYPE_INT,
> { .i64 = HUFFMAN_TABLE_DEFAULT }, 1, 2, VE, "huffman" },
> +    { "default", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
> HUFFMAN_TABLE_DEFAULT }, INT_MIN, INT_MAX, VE, "huffman" },
> +    { "optimal", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
> HUFFMAN_TABLE_OPTIMAL }, INT_MIN, INT_MAX, VE, "huffman" },
>  { NULL},
>  };
>

You forgot to readjust the range on the huffman option, it should be 0, 1,
not 1, 2.

Apart from that I did some tests. The overall compression of the encoder is
improved by constant 15%, which is impressive. The performance overhead is
on average 7% which is insignificant compared to the compression
improvements, so I'd approve turning the option on by default (though I'll
agree its better if you submit a separate patch for that). The output with
huffman optimizations enabled showed a slight gain in SSIM, which is
probably from the rate control from mpegvideo_enc.

The only problem I have with the code is that it uses a linked list and
calls malloc during runtime. Couldn't the linked list be replaced with an
array allocated during init (it should be okay, I don't think resolution is
allowed to change during encoding)? That way it'll be way less messy than a
linked list and there'll be less things to go wrong. Performance will
probably improve too. You always iterate over all buffers anyway.