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

[Rostislav Pehlivanov]

On 9 February 2017 at 02:44, Michael Niedermayer <michael at niedermayer.cc>
wrote:

>
> > +
> > +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()
>

Fixed


>
> [...]
> > 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
>

Fixed


> [...]
> > +// 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*()
>
>
Fixed


>
> [...]
> > +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
>
>
Fixed


>
> > +
>
> > +// 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()
>
>
Fixed


> 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
>

Perhaps I was too hasty to review it

I'll take a look at fixing the 2 pass mode tomorrow if I have the time,
although if you have spare
time and know how to fix it go ahead.