[FFmpeg-devel] [PATCH] Implement optimal huffman encoding for (M)JPEG.
Michael Niedermayer
michael at niedermayer.cc
Thu Feb 9 14:20:09 EET 2017
On Thu, Feb 09, 2017 at 03:44:59AM +0100, Michael Niedermayer wrote:
> On Wed, Feb 01, 2017 at 11:23:04PM -0800, Jerry Jiang wrote:
[...]
> 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
more reviewing related to what is in git + fixes
[...]
>+static inline void ff_mjpeg_encode_code(MJpegContext *s, uint8_t table_id, int code)
static functions dont need ff_ prefix
>+{
>+ MJpegHuffmanCode *c = &s->huff_buffer[s->huff_ncode++];
>+ av_assert0(s->huff_ncode < s->huff_capacity);
>+ c->table_id = table_id;
>+ c->code = code;
>+}
>+
>+/**
>+ * Add the coefficient's data to the JPEG buffer.
>+ *
>+ * @param s The MJpegContext which contains the JPEG buffer.
>+ * @param table_id Which Huffman table the code belongs to.
>+ * @param val The coefficient.
>+ * @param run The run-bits.
>+ */
>+static void ff_mjpeg_encode_coef(MJpegContext *s, uint8_t table_id, int val, int run)
static functions dont need ff_ prefix
[...]
>-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));
this will always reallocate the buffer to its maximum, in fact it
will allocate a much larger buffer than the maximum as
av_fast_realloc() overallocates
so this is always slower and uses more memory than if the maximum
buffer size is allocated during init.
I understand the intent might have been to allocate only what is
needed but the code does not do that
>+ if (!new_buf) {
>+ m->error = AVERROR(ENOMEM);
>+ } else {
>+ m->huff_buffer = new_buf;
>+ }
>+}
>+
>+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(s->avctx, AV_LOG_ERROR, "Unsupported encoding method\n");
>+ 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;
i_tex_bits is for rate control, iam not sure what this use is trying to
do but the value its set to is not correct and i see nothing that
corrects it. This is likely what breaks 2pass
[...]
>@@ -372,14 +414,116 @@ void ff_mjpeg_escape_FF(PutBitContext *pb, int start)
> }
> }
>
>+/**
>+ * Builds all 4 optimal Huffman tables.
>+ *
>+ * Uses the data stored in the JPEG buffer to compute the tables.
>+ * Stores the Huffman tables in the bits_* and val_* arrays in the MJpegContext.
>+ *
>+ * @param m MJpegContext containing the JPEG buffer.
>+ */
>+static void ff_mjpeg_build_optimal_huffman(MJpegContext *m)
>+{
>+ int i, ret, table_id, code;
>+
>+ MJpegEncHuffmanContext dc_luminance_ctx;
>+ MJpegEncHuffmanContext dc_chrominance_ctx;
>+ MJpegEncHuffmanContext ac_luminance_ctx;
>+ MJpegEncHuffmanContext ac_chrominance_ctx;
>+ MJpegEncHuffmanContext *ctx[4] = {&dc_luminance_ctx,
>+ &dc_chrominance_ctx,
>+ &ac_luminance_ctx,
>+ &ac_chrominance_ctx};
>+ for (i = 0; i < 4; i++) {
>+ ff_mjpeg_encode_huffman_init(ctx[i]);
>+ }
>+ for (i = 0; i < m->huff_ncode; i++) {
>+ table_id = m->huff_buffer[i].table_id;
>+ code = m->huff_buffer[i].code;
>+
>+ ff_mjpeg_encode_huffman_increment(ctx[table_id], code);
>+ }
>+
>+ ret = ff_mjpeg_encode_huffman_close(&dc_luminance_ctx,
>+ m->bits_dc_luminance,
>+ m->val_dc_luminance, 12);
>+ av_assert0(!ret);
>+ ret = ff_mjpeg_encode_huffman_close(&dc_chrominance_ctx,
>+ m->bits_dc_chrominance,
>+ m->val_dc_chrominance, 12);
>+ av_assert0(!ret);
>+ ret = ff_mjpeg_encode_huffman_close(&ac_luminance_ctx,
>+ m->bits_ac_luminance,
>+ m->val_ac_luminance, 256);
>+ av_assert0(!ret);
>+ ret = ff_mjpeg_encode_huffman_close(&ac_chrominance_ctx,
>+ m->bits_ac_chrominance,
>+ m->val_ac_chrominance, 256);
>+ av_assert0(!ret);
if the error cannot occur the assert belongs in the function,
theres no need to return a constant that is asserted on outside
>+
>+ ff_mjpeg_build_huffman_codes(m->huff_size_dc_luminance,
>+ m->huff_code_dc_luminance,
>+ m->bits_dc_luminance,
>+ m->val_dc_luminance);
>+ ff_mjpeg_build_huffman_codes(m->huff_size_dc_chrominance,
>+ m->huff_code_dc_chrominance,
>+ m->bits_dc_chrominance,
>+ m->val_dc_chrominance);
>+ ff_mjpeg_build_huffman_codes(m->huff_size_ac_luminance,
>+ m->huff_code_ac_luminance,
>+ m->bits_ac_luminance,
>+ m->val_ac_luminance);
>+ ff_mjpeg_build_huffman_codes(m->huff_size_ac_chrominance,
>+ m->huff_code_ac_chrominance,
>+ m->bits_ac_chrominance,
>+ m->val_ac_chrominance);
>+}
>+
>+/**
>+ * Writes the complete JPEG frame.
>+ *
>+ * Header + values + stuffing.
>+ *
>+ * @param s The MpegEncContext.
>+ * @return int Error code, 0 if successful.
>+ */
> int ff_mjpeg_encode_stuffing(MpegEncContext *s)
> {
> int i;
> PutBitContext *pbc = &s->pb;
> int mb_y = s->mb_y - !s->mb_x;
>+ int ret;
>+ MJpegContext *m;
>+
>+ m = s->mjpeg_ctx;
>+
>+ if (m->error)
>+ return m->error;
>+
>+ if (s->huffman == HUFFMAN_TABLE_OPTIMAL) {
>+ ff_mjpeg_build_optimal_huffman(m);
>+
>+ // Replace the VLCs with the optimal ones.
>+ // The default ones may be used for trellis during quantization.
>+ ff_init_uni_ac_vlc(m->huff_size_ac_luminance, m->uni_ac_vlc_len);
>+ ff_init_uni_ac_vlc(m->huff_size_ac_chrominance, m->uni_chroma_ac_vlc_len);
This is wrong and will break trellis with non default tables, trellis
needs to use the same VLCs as are used for encoding. which if non
default ones are used should be these non default ones
the code generating the default tables wrote them in shared
tables (uni*ac_vlc_len), now after the patch the default code is
shared, called from 2 places and the tables moved into the context yet
it still generates only the default tables. This looks unfinished
More so the default tables are unneccessary re-generated every frame
this is just broken unless iam missing some code
>+ s->intra_ac_vlc_length =
>+ s->intra_ac_vlc_last_length = m->uni_ac_vlc_len;
>+ s->intra_chroma_ac_vlc_length =
>+ s->intra_chroma_ac_vlc_last_length = m->uni_chroma_ac_vlc_len;
>+ }
>+
>+ ff_mjpeg_encode_picture_header(s->avctx, &s->pb, &s->intra_scantable,
>+ s->pred, s->intra_matrix, s->chroma_intra_matrix);
>+ ff_mjpeg_encode_picture_frame(s);
>+ if (m->error < 0) {
>+ ret = m->error;
>+ return ret;
>+ }
the whole m->error system is broken
there is no such thing as an error during encoding with optimal tables
there could be an error if allocation was done based on used codes,
iam not sure this would make sense but the code doesnt do that.
So either always allocate the maximum (which is less than currently
allocated), or reallocate depending on used codes
also either way no error check is neeed at encode_block() level.
A check at ff_mjpeg_encode_mb() is sufficient. The number of blocks
per mb is constant
>+
>+ ret = ff_mpv_reallocate_putbitbuffer(s, put_bits_count(&s->pb) / 8 + 100,
>+ put_bits_count(&s->pb) / 4 + 1000);
>
>- int ret = ff_mpv_reallocate_putbitbuffer(s, put_bits_count(&s->pb) / 8 + 100,
>- put_bits_count(&s->pb) / 4 + 1000);
> if (ret < 0) {
> av_log(s->avctx, AV_LOG_ERROR, "Buffer reallocation failed\n");
> goto fail;
>diff --git a/libavcodec/mjpegenc_common.h b/libavcodec/mjpegenc_common.h
>index 6e51ca04f8..d9a565dfa9 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 ff_init_uni_ac_vlc(const uint8_t huff_size_ac[256], uint8_t *uni_ac_vlc_len);
This function is *jpeg specific, nothing in its name suggests that
also other codecs have their own init_uni_ac_vlc() so thats very
confusing naming wise
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
-------------- 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/430aa511/attachment.sig>
More information about the ffmpeg-devel
mailing list