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

[Moritz Barsnick]

On Tue, Dec 27, 2016 at 07:37:09 +0000, Jerry Jiang wrote:
> This is my first patch submitted to FFmpeg, so I'm sure that I missed
> something. Please bear with me. :P This patch implements the solution
> outlined here: https://guru.multimedia.cx/small-tasks-for-ffmpeg/

Apart from the newline-corrupted patch:

> +    if (s->out_format == FMT_MJPEG && s->huffman == 2) {
                                                       ^
[...]
> +    if (s->huffman == 2) {
                         ^
and

> +{ "huffman", "Huffman table strategy", OFFSET(huffman), AV_OPT_TYPE_INT, { .i64 = 1 }, 1, 2, VE, "huffman" },
                                                                                     ^
> +    { "default", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 }, INT_MIN, INT_MAX, VE, "huffman" },
                                                         ^
> +    { "optimal", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 2 }, INT_MIN, INT_MAX, VE, "huffman" },
                                                         ^
For readability, '1' and '2', which are just arbitrary numbers, could
be expressed with enums (starting with 0, as we do in the C world ;-)).

> +static int compare_by_prob(const void *a, const void *b) {
> +    PTable a_val = *(PTable *)a;
> +    PTable b_val = *(PTable *)b;
> +    if (a_val.prob < b_val.prob) {
> +        return -1;
> +    }
> +    if (a_val.prob > b_val.prob) {
> +        return 1;
> +    }
> +    return 0;
> +}

Doesn't ffmpeg have a macro for this? FFSIGN(a->prob - b->prob); ?

> +static int compare_by_length(const void *a, const void *b) {
> +    HuffTable a_val = *(HuffTable *)a;
> +    HuffTable b_val = *(HuffTable *)b;
> +    if (a_val.length < b_val.length) {
> +        return -1;
> +    }
> +    if (a_val.length > b_val.length) {
> +        return 1;
> +    }
> +    return 0;
> +}

Same here. Perhaps a bit overengineered, unless I'm missing something.

Moritz