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

Michael Niedermayer michael at niedermayer.cc
Thu Feb 9 04:44:59 EET 2017


On Wed, Feb 01, 2017 at 11:23:04PM -0800, Jerry Jiang wrote:
> > seems to break
> > make fate-vsynth1-mjpeg-444
> 
> Fixed.
> 

Missing commit message


[...]
> -void ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64])
> +// Possibly reallocate the huffman code buffer, assuming blocks_per_mb.
> +// Set s->mjpeg_ctx->error on ENOMEM.
> +static void realloc_huffman(MpegEncContext *s, int blocks_per_mb)
>  {
> -    int i;
> +    MJpegContext *m = s->mjpeg_ctx;
> +    size_t num_mbs, num_blocks, num_codes;
> +    MJpegHuffmanCode *new_buf;
> +    if (m->error) return;
> +    // Make sure we have enough space to hold this frame.
> +    num_mbs = s->mb_width * s->mb_height;
> +    num_blocks = num_mbs * blocks_per_mb;
> +    av_assert0(m->huff_ncode <=
> +               (s->mb_y * s->mb_width + s->mb_x) * blocks_per_mb * 64);
> +    num_codes = num_blocks * 64;
> +
> +    new_buf = av_fast_realloc(m->huff_buffer, &m->huff_capacity,
> +                              num_codes * sizeof(MJpegHuffmanCode));
> +    if (!new_buf) {
> +        m->error = AVERROR(ENOMEM);
> +    } else {
> +        m->huff_buffer = new_buf;
> +    }
> +}

why is the macroblock loop calling a "framebuffer" reallocation
function?
the frame size does not change from one maroblock to the next


> +
> +int ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64])
> +{
> +    int i, is_chroma_420;
> +
> +    // Number of bits used depends on future data.
> +    // So, nothing that relies on encoding many times and taking the
> +    // one with the fewest bits will work properly here.
> +    if (s->i_tex_bits != MJPEG_HUFFMAN_EST_BITS_PER_CODE *
> +        s->mjpeg_ctx->huff_ncode) {

> +        av_log(NULL, AV_LOG_ERROR, "Unsupported encoding method\n");

Missing context for av_log()



> +        return AVERROR(EINVAL);
> +    }
> +
>      if (s->chroma_format == CHROMA_444) {
> +        realloc_huffman(s, 12);
>          encode_block(s, block[0], 0);
>          encode_block(s, block[2], 2);
>          encode_block(s, block[4], 4);
> @@ -196,10 +302,12 @@ void ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64])
>              encode_block(s, block[11], 11);
>          }
>      } else {
> +        is_chroma_420 = (s->chroma_format == CHROMA_420);
> +        realloc_huffman(s, 5 + (is_chroma_420 ? 1 : 3));
>          for(i=0;i<5;i++) {
>              encode_block(s, block[i], i);
>          }
> -        if (s->chroma_format == CHROMA_420) {
> +        if (is_chroma_420) {
>              encode_block(s, block[5], 5);
>          } else {
>              encode_block(s, block[6], 6);
> @@ -207,8 +315,11 @@ void ff_mjpeg_encode_mb(MpegEncContext *s, int16_t block[12][64])
>              encode_block(s, block[7], 7);
>          }
>      }
> +    if (s->mjpeg_ctx->error)
> +        return s->mjpeg_ctx->error;
>  
> -    s->i_tex_bits += get_bits_diff(s);
> +    s->i_tex_bits = MJPEG_HUFFMAN_EST_BITS_PER_CODE * s->mjpeg_ctx->huff_ncode;
> +    return 0;
>  }

this patch also breaks 2 pass rate control

./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -vcodec mjpeg -pass 1 -t 10 -an -b 5000k test1.avi
./ffmpeg -i ~/videos/matrixbench_mpeg2.mpg -vcodec mjpeg -pass 2 -t 10 -an -b 5000k test2.avi



[...]
> diff --git a/libavcodec/mjpegenc_common.c b/libavcodec/mjpegenc_common.c
> index 7a6fe746..8ec3df9 100644
> --- a/libavcodec/mjpegenc_common.c
> +++ b/libavcodec/mjpegenc_common.c
> @@ -33,8 +33,35 @@
>  #include "put_bits.h"
>  #include "mjpegenc.h"
>  #include "mjpegenc_common.h"
> +#include "mjpegenc_huffman.h"
>  #include "mjpeg.h"
>  

> +av_cold void init_uni_ac_vlc(const uint8_t huff_size_ac[256], uint8_t *uni_ac_vlc_len)
> +{
> +    int i;
> +
> +    for (i = 0; i < 128; i++) {
> +        int level = i - 64;
> +        int run;
> +        if (!level)
> +            continue;
> +        for (run = 0; run < 64; run++) {
> +            int len, code, nbits;
> +            int alevel = FFABS(level);
> +
> +            len = (run >> 4) * huff_size_ac[0xf0];
> +
> +            nbits= av_log2_16bit(alevel) + 1;
> +            code = ((15&run) << 4) | nbits;
> +
> +            len += huff_size_ac[code] + nbits;
> +
> +            uni_ac_vlc_len[UNI_AC_ENC_INDEX(run, i)] = len;
> +            // We ignore EOB as its just a constant which does not change generally
> +        }
> +    }
> +}

This code is moved, it should have been in a seperate cosmetic patch



[...]
> diff --git a/libavcodec/mjpegenc_common.h b/libavcodec/mjpegenc_common.h
> index 6e51ca0..7d760f8 100644
> --- a/libavcodec/mjpegenc_common.h
> +++ b/libavcodec/mjpegenc_common.h
> @@ -40,4 +40,6 @@ void ff_mjpeg_init_hvsample(AVCodecContext *avctx, int hsample[4], int vsample[4
>  void ff_mjpeg_encode_dc(PutBitContext *pb, int val,
>                          uint8_t *huff_size, uint16_t *huff_code);
>  
> +av_cold void init_uni_ac_vlc(const uint8_t huff_size_ac[256], uint8_t *uni_ac_vlc_len);

missing ff/av prefix


[...]
> +// Validate the computed lengths satisfy the JPEG restrictions and is optimal.
> +static int check_lengths(int L, int expected_length,
> +                         const int *probs, int nprobs)
> +{
> +    HuffTable lengths[256];
> +    PTable val_counts[256];
> +    int actual_length = 0, i, j, k, prob, length;
> +    int ret = 0;
> +    double cantor_measure = 0;

> +    assert(nprobs <= 256);

should be av_assert*()


[...]
> +static const int probs_zeroes[] = {6, 6, 0, 0, 0};
> +static const int probs_skewed[] = {2, 0, 0, 0, 0, 1, 0, 0, 20, 0, 2,
> +    0, 10, 5, 1, 1, 9, 1, 1, 6, 0, 5, 0, 1, 0, 7, 6, 1, 1, 5, 0, 0, 0, 0,
> +    11, 0, 0, 0, 51, 1, 0, 20, 0, 1, 0, 0, 0, 0, 6, 106, 1, 0, 1, 0, 2, 1,
> +    16, 0, 0, 5, 0, 0, 0, 4, 3, 15, 4, 4, 0, 0, 0, 3, 0, 0, 1, 0, 3, 0, 3,
> +    2, 2, 0, 0, 4, 3, 40, 1, 2, 0, 22, 0, 0, 0, 9, 0, 0, 0, 0, 1, 1, 0, 1,
> +    6, 11, 4, 10, 28, 6, 1, 0, 0, 9, 9, 4, 0, 0, 0, 0, 8, 33844, 2, 0, 2,
> +    1, 1, 5, 0, 0, 1, 9, 1, 0, 4, 14, 4, 0, 0, 3, 8, 0, 51, 9, 6, 1, 1, 2,
> +    2, 3, 1, 5, 5, 29, 0, 0, 0, 0, 14, 29, 6, 4, 13, 12, 2, 3, 1, 0, 5, 4,
> +    1, 1, 0, 0, 29, 1, 0, 0, 0, 0, 4, 0, 0, 1, 0, 1, 7, 0, 42, 0, 0, 0, 0,
> +    0, 2, 0, 3, 9, 0, 0, 0, 2, 1, 0, 0, 6, 5, 6, 1, 2, 3, 0, 0, 0, 3, 0, 0,
> +    28, 0, 2, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 23, 0, 0, 0, 0,
> +    0, 21, 1, 0, 3, 24, 2, 0, 0, 7, 0, 0, 1, 5, 1, 2, 0, 5};
> +static const int probs_sat[] = {74, 8, 14, 7, 9345, 40, 0, 2014, 2, 1,
> +    115, 0, 2, 1, 194, 388, 20, 0, 0, 2, 1, 121, 1, 1583, 0, 16, 21, 2, 132,
> +    2, 15, 9, 13, 1, 0, 2293, 2, 8, 5, 2, 30, 0, 0, 4, 54, 783, 4, 1, 2, 4,
> +    0, 22, 93, 1, 143, 19, 0, 36, 32, 4, 6, 33, 3, 45, 0, 8, 1, 0, 18, 17, 1,
> +    0, 1, 0, 0, 1, 1004, 38, 3, 8, 90, 23, 0, 2819, 3, 0, 970, 158, 9, 6, 4,
> +    48, 4, 0, 1, 0, 0, 60, 3, 62, 0, 2, 2, 2, 279, 66, 16, 1, 20, 0, 7, 9,
> +    32, 1411, 6, 3, 27, 1, 5, 49, 0, 0, 0, 0, 0, 2, 10, 1, 1, 2, 3, 801, 3,
> +    25, 5, 1, 1, 0, 632, 0, 14, 18, 5, 8, 200, 4, 4, 22, 12, 0, 4, 1, 0, 2,
> +    4, 9, 3, 16, 7, 2, 2, 213, 0, 2, 620, 39303, 0, 1, 0, 2, 1, 183781, 1,
> +    0, 0, 0, 94, 7, 3, 4, 0, 4, 306, 43, 352, 76, 34, 13, 11, 0, 51, 1, 13,
> +    19, 0, 26, 0, 7276, 4, 207, 31, 1, 2, 4, 6, 19, 8, 17, 4, 6, 0, 1085, 0,
> +    0, 0, 3, 489, 36, 1, 0, 1, 9420, 294, 28, 0, 57, 5, 0, 9, 2, 0, 1, 2, 2,
> +    0, 0, 9, 2, 29, 2, 2, 7, 0, 5, 490, 0, 7, 5, 0, 1, 8, 0, 0, 23255, 0, 1};

vertical align



> +

> +// Test the example given on @see
> +// http://guru.multimedia.cx/small-tasks-for-ffmpeg/
> +int main(int argc, char **argv)
> +{
> +    int i, ret = 0;
> +    // Probabilities of symbols 0..4
> +    static PTable val_counts[] = {

static isnt needed here this is main()

i sadly dont have time to do a more complete review ATM (need sleep)
but this patch doesnt look like it was ready for being pushed

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170209/fd5919cf/attachment.sig>


More information about the ffmpeg-devel mailing list