[FFmpeg-devel] mlpenc: fix lossless failures, add sanity checks

Submitted by Jai Luthra on July 9, 2019, 5:23 p.m.

Details

Message ID 20190709172302.14886-1-me@jailuthra.in
State New
Headers show

Commit Message

Jai Luthra July 9, 2019, 5:23 p.m.
Signed-off-by: Jai Luthra <me@jailuthra.in>
---
 libavcodec/mlpenc.c | 72 +++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

Comments

Carl Eugen Hoyos July 9, 2019, 6:26 p.m.
> Am 09.07.2019 um 19:23 schrieb Jai Luthra <me@jailuthra.in>:
> 
> Signed-off-by: Jai Luthra <me@jailuthra.in>
> ---
> libavcodec/mlpenc.c | 72 +++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
> index deb171645c..09a8336b47 100644
> --- a/libavcodec/mlpenc.c
> +++ b/libavcodec/mlpenc.c
> @@ -1,6 +1,7 @@
> /**
>  * MLP encoder
>  * Copyright (c) 2008 Ramiro Polla
> + * Copyright (c) 2016-2019 Jai Luthra
>  *
>  * This file is part of FFmpeg.
>  *
> @@ -26,6 +27,7 @@
> #include "libavutil/crc.h"
> #include "libavutil/avstring.h"
> #include "libavutil/samplefmt.h"
> +#include "libavutil/avassert.h"
> #include "mlp.h"
> #include "lpc.h"
> 
> @@ -93,8 +95,8 @@ typedef struct BestOffset {
>     int16_t max;
> } BestOffset;
> 
> -#define HUFF_OFFSET_MIN    -16384
> -#define HUFF_OFFSET_MAX     16383
> +#define HUFF_OFFSET_MIN    (-16384)
> +#define HUFF_OFFSET_MAX    ( 16383)

Please split the patch into self-contained changes with appropriate commit messages.

Thank you, Carl Eugen
Jai Luthra July 9, 2019, 8:12 p.m.
On Tue, Jul 09, 2019 at 08:26:19PM +0200, Carl Eugen Hoyos wrote:
>
>Please split the patch into self-contained changes with appropriate commit messages.
>
>Thank you, Carl Eugen

Cool, I've done more fixes will send a new patch set as a separate thread

thx

Patch hide | download patch | download mbox

diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
index deb171645c..09a8336b47 100644
--- a/libavcodec/mlpenc.c
+++ b/libavcodec/mlpenc.c
@@ -1,6 +1,7 @@ 
 /**
  * MLP encoder
  * Copyright (c) 2008 Ramiro Polla
+ * Copyright (c) 2016-2019 Jai Luthra
  *
  * This file is part of FFmpeg.
  *
@@ -26,6 +27,7 @@ 
 #include "libavutil/crc.h"
 #include "libavutil/avstring.h"
 #include "libavutil/samplefmt.h"
+#include "libavutil/avassert.h"
 #include "mlp.h"
 #include "lpc.h"
 
@@ -93,8 +95,8 @@  typedef struct BestOffset {
     int16_t max;
 } BestOffset;
 
-#define HUFF_OFFSET_MIN    -16384
-#define HUFF_OFFSET_MAX     16383
+#define HUFF_OFFSET_MIN    (-16384)
+#define HUFF_OFFSET_MAX    ( 16383)
 
 /** Number of possible codebooks (counting "no codebooks") */
 #define NUM_CODEBOOKS       4
@@ -466,9 +468,6 @@  static void default_decoding_params(MLPEncodeContext *ctx,
  */
 static int inline number_sbits(int number)
 {
-    if (number < 0)
-        number++;
-
     return av_log2(FFABS(number)) + 1 + !!number;
 }
 
@@ -807,7 +806,7 @@  static void write_major_sync(MLPEncodeContext *ctx, uint8_t *buf, int buf_size)
 static void write_restart_header(MLPEncodeContext *ctx, PutBitContext *pb)
 {
     RestartHeader *rh = ctx->cur_restart_header;
-    int32_t lossless_check = xor_32_to_8(rh->lossless_check_data);
+    uint8_t lossless_check = xor_32_to_8(rh->lossless_check_data);
     unsigned int start_count = put_bits_count(pb);
     PutBitContext tmpb;
     uint8_t checksum;
@@ -1016,12 +1015,10 @@  static void write_block_data(MLPEncodeContext *ctx, PutBitContext *pb)
         codebook_index  [ch] = cp->codebook  - 1;
         sign_huff_offset[ch] = cp->huff_offset;
 
-        sign_shift = lsb_bits[ch] - 1;
+        sign_shift = lsb_bits[ch] + (cp->codebook ? 2 - cp->codebook : -1);
 
-        if (cp->codebook > 0) {
+        if (cp->codebook > 0)
             sign_huff_offset[ch] -= 7 << lsb_bits[ch];
-            sign_shift += 3 - cp->codebook;
-        }
 
         /* Unsign if needed. */
         if (sign_shift >= 0)
@@ -1031,7 +1028,6 @@  static void write_block_data(MLPEncodeContext *ctx, PutBitContext *pb)
     for (i = 0; i < dp->blocksize; i++) {
         for (ch = rh->min_channel; ch <= rh->max_channel; ch++) {
             int32_t sample = *sample_buffer++ >> dp->quant_step_size[ch];
-
             sample -= sign_huff_offset[ch];
 
             if (codebook_index[ch] >= 0) {
@@ -1251,7 +1247,7 @@  static void input_data_internal(MLPEncodeContext *ctx, const uint8_t *samples,
                 uint32_t abs_sample;
                 int32_t sample;
 
-                sample = is24 ? *samples_32++ >> 8 : *samples_16++ << 8;
+                sample = is24 ? *samples_32++ >> 8 : *samples_16++ * 256U;
 
                 /* TODO Find out if number_sbits can be used for negative values. */
                 abs_sample = FFABS(sample);
@@ -1591,7 +1587,7 @@  static void no_codebook_bits(MLPEncodeContext *ctx,
 {
     DecodingParams *dp = ctx->cur_decoding_params;
     int16_t offset;
-    int32_t unsign;
+    int32_t unsign = 0;
     uint32_t diff;
     int lsb_bits;
 
@@ -1607,7 +1603,8 @@  static void no_codebook_bits(MLPEncodeContext *ctx,
 
     lsb_bits = number_sbits(diff) - 1;
 
-    unsign = 1 << (lsb_bits - 1);
+    if (lsb_bits > 0)
+        unsign = 1 << (lsb_bits - 1);
 
     /* If all samples are the same (lsb_bits == 0), offset must be
      * adjusted because of sign_shift. */
@@ -1699,7 +1696,7 @@  static inline void codebook_bits(MLPEncodeContext *ctx,
     offset_min = FFMAX(min, HUFF_OFFSET_MIN);
     offset_max = FFMIN(max, HUFF_OFFSET_MAX);
 
-    for (;;) {
+    while (offset <= offset_max && offset >= offset_min) {
         BestOffset temp_bo;
 
         codebook_bits_offset(ctx, channel, codebook,
@@ -1709,6 +1706,7 @@  static inline void codebook_bits(MLPEncodeContext *ctx,
         if (temp_bo.bitcount < previous_count) {
             if (temp_bo.bitcount < bo->bitcount)
                 *bo = temp_bo;
+            av_assert0(bo->offset <= HUFF_OFFSET_MAX && bo->offset >= HUFF_OFFSET_MIN);
 
             is_greater = 0;
         } else if (++is_greater >= ctx->max_codebook_search)
@@ -1718,12 +1716,8 @@  static inline void codebook_bits(MLPEncodeContext *ctx,
 
         if (direction) {
             offset = temp_bo.max + 1;
-            if (offset > offset_max)
-                break;
         } else {
             offset = temp_bo.min - 1;
-            if (offset < offset_min)
-                break;
         }
     }
 }
@@ -1796,11 +1790,11 @@  static void determine_bits(MLPEncodeContext *ctx)
 #define SAMPLE_MAX(bitdepth) ((1 << (bitdepth - 1)) - 1)
 #define SAMPLE_MIN(bitdepth) (~SAMPLE_MAX(bitdepth))
 
-#define MSB_MASK(bits)  (-1u << bits)
+#define MSB_MASK(bits)  (-(1u << (bits)))
 
 /** Applies the filter to the current samples, and saves the residual back
  *  into the samples buffer. If the filter is too bad and overflows the
- *  maximum amount of bits allowed (16 or 24), the samples buffer is left as is and
+ *  maximum amount of bits allowed (24), the samples buffer is left as is and
  *  the function returns -1.
  */
 static int apply_filter(MLPEncodeContext *ctx, unsigned int channel)
@@ -1836,7 +1830,7 @@  static int apply_filter(MLPEncodeContext *ctx, unsigned int channel)
         int32_t sample = *sample_buffer;
         unsigned int order;
         int64_t accum = 0;
-        int32_t residual;
+        int64_t residual;
 
         for (filter = 0; filter < NUM_FILTERS; filter++) {
             int32_t *fcoeff = ctx->cur_channel_params[channel].coeff[filter];
@@ -1848,11 +1842,11 @@  static int apply_filter(MLPEncodeContext *ctx, unsigned int channel)
         accum  >>= filter_shift;
         residual = sample - (accum & mask);
 
-        if (residual < SAMPLE_MIN(ctx->wordlength) || residual > SAMPLE_MAX(ctx->wordlength))
+        if (residual < SAMPLE_MIN(24) || residual > SAMPLE_MAX(24))
             return -1;
 
         filter_state_buffer[FIR][i] = sample;
-        filter_state_buffer[IIR][i] = residual;
+        filter_state_buffer[IIR][i] = (int32_t) residual;
 
         sample_buffer += ctx->num_channels;
     }
@@ -1882,7 +1876,7 @@  static void apply_filters(MLPEncodeContext *ctx)
              * Clear filter params and update state. */
             set_filter_params(ctx, channel, FIR, 1);
             set_filter_params(ctx, channel, IIR, 1);
-            apply_filter(ctx, channel);
+            av_assert0(apply_filter(ctx, channel) >= 0);
         }
     }
 }
@@ -1897,8 +1891,8 @@  static void generate_2_noise_channels(MLPEncodeContext *ctx)
 
     for (i = 0; i < ctx->number_of_samples; i++) {
         uint16_t seed_shr7 = seed >> 7;
-        *sample_buffer++ = ((int8_t)(seed >> 15)) << rh->noise_shift;
-        *sample_buffer++ = ((int8_t) seed_shr7)   << rh->noise_shift;
+        *sample_buffer++ = ((int8_t)(seed >> 15)) * (1 << rh->noise_shift);
+        *sample_buffer++ = ((int8_t) seed_shr7)   * (1 << rh->noise_shift);
 
         seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5);
 
@@ -2069,9 +2063,10 @@  static void set_best_codebook(MLPEncodeContext *ctx)
             best_codebook = *best_path++ - ZERO_PATH;
             cur_bo = &ctx->best_offset[index][channel][best_codebook];
 
-            cp->huff_offset = cur_bo->offset;
-            cp->huff_lsbs   = cur_bo->lsb_bits + dp->quant_step_size[channel];
-            cp->codebook    = best_codebook;
+            cp->huff_offset      = cur_bo->offset;
+            cp->sign_huff_offset = -(1 << 23);
+            cp->huff_lsbs        = cur_bo->lsb_bits + dp->quant_step_size[channel];
+            cp->codebook         = best_codebook;
         }
     }
 }
@@ -2210,8 +2205,15 @@  static void process_major_frame(MLPEncodeContext *ctx)
         generate_2_noise_channels(ctx);
         rematrix_channels        (ctx);
 
-        for (channel = rh->min_channel; channel <= rh->max_channel; channel++)
-            apply_filter(ctx, channel);
+        for (channel = rh->min_channel; channel <= rh->max_channel; channel++) {
+            if (apply_filter(ctx, channel) < 0) {
+                /* Filter is horribly wrong.
+                 * Clear filter params and update state. */
+                set_filter_params(ctx, channel, FIR, 1);
+                set_filter_params(ctx, channel, IIR, 1);
+                av_assert0(apply_filter(ctx, channel) >= 0);
+            }
+        }
     }
 }
 
@@ -2277,7 +2279,7 @@  static int mlp_encode_frame(AVCodecContext *avctx, AVPacket *avpkt,
     if (restart_frame) {
         set_major_params(ctx);
         if (ctx->min_restart_interval != ctx->max_restart_interval)
-        process_major_frame(ctx);
+            process_major_frame(ctx);
     }
 
     if (ctx->min_restart_interval == ctx->max_restart_interval)
@@ -2391,7 +2393,7 @@  AVCodec ff_mlp_encoder = {
     .encode2                = mlp_encode_frame,
     .close                  = mlp_encode_close,
     .capabilities           = AV_CODEC_CAP_SMALL_LAST_FRAME | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_EXPERIMENTAL,
-    .sample_fmts            = (const enum AVSampleFormat[]) {AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_NONE},
+    .sample_fmts            = (const enum AVSampleFormat[]) {AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_S32, AV_SAMPLE_FMT_NONE},
     .supported_samplerates  = (const int[]) {44100, 48000, 88200, 96000, 176400, 192000, 0},
     .channel_layouts        = ff_mlp_channel_layouts,
 };
@@ -2407,7 +2409,7 @@  AVCodec ff_truehd_encoder = {
     .encode2                = mlp_encode_frame,
     .close                  = mlp_encode_close,
     .capabilities           = AV_CODEC_CAP_SMALL_LAST_FRAME | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_EXPERIMENTAL,
-    .sample_fmts            = (const enum AVSampleFormat[]) {AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_NONE},
+    .sample_fmts            = (const enum AVSampleFormat[]) {AV_SAMPLE_FMT_S16, AV_SAMPLE_FMT_S32, AV_SAMPLE_FMT_NONE},
     .supported_samplerates  = (const int[]) {44100, 48000, 88200, 96000, 176400, 192000, 0},
     .channel_layouts        = (const uint64_t[]) {AV_CH_LAYOUT_STEREO, AV_CH_LAYOUT_5POINT0_BACK, AV_CH_LAYOUT_5POINT1_BACK, 0},
 };