diff mbox series

[FFmpeg-devel,7/7] mlp: check huff_lsbs only when codebook is used

Message ID 20200202190300.156267-2-me@jailuthra.in
State Accepted
Headers show
Series mlpenc: fix lossless check failures for 16-bit encoding | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Jai Luthra Feb. 2, 2020, 7:03 p.m. UTC 
When no codebook is used, huff_lsbs can be more than 24 and still decode to 
original values once filters are applied.

Signed-off-by: Jai Luthra <me@jailuthra.in>
---
 libavcodec/mlpdec.c | 2 +-
 libavcodec/mlpenc.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

Fixes lossless check failures caused with https://lynne.ee/files/sample.flac

Comments

Michael Niedermayer Feb. 4, 2020, 9:24 p.m. UTC | #1 
On Mon, Feb 03, 2020 at 12:33:00AM +0530, Jai Luthra wrote:
> When no codebook is used, huff_lsbs can be more than 24 and still decode to 
> original values once filters are applied.

huff_lsbs can but get_bits() is limited to 25, you need get_bits_long() beyond

thx

[...]
Jai Luthra Feb. 6, 2020, 9:29 a.m. UTC | #2 
On Tue, Feb 04, 2020 at 10:24:50PM +0100, Michael Niedermayer wrote:
>On Mon, Feb 03, 2020 at 12:33:00AM +0530, Jai Luthra wrote:
>> When no codebook is used, huff_lsbs can be more than 24 and still decode to
>> original values once filters are applied.
>
>huff_lsbs can but get_bits() is limited to 25, you need get_bits_long() beyond

I see, thx Michael!

There are two ways to deal with this:

1) Use get_bits_long() where get_bits() is used to read the lsb_bits from the 
bitstream as it is certainly going to be within 32.

2) Limit huff_lsbs to 25 bits even if no codebook.

I tested encoding various samples and it does not go above 25 bits. It hits 25 
in mlpenc because at the matrixing stage L+R channels get added together in a 
single channel, but I am not sure if other encoders would also adhere to that.

(I was previously wrong in assuming that it hits more than 24 at the filtering 
stage, I tested it [1] and it does so after the re-matrixing stage, before any 
FIR filters)

I prefer (1) as it is cleaner. Let me know if that is OK and I'll send a 
patch.

Cheers,
Jai

[1]: https://github.com/jailuthra/FFmpeg/commit/04cf7262496cd296be3b3beaf049bdb1887f2d84
     with https://lynne.ee/files/sample.flac

>
>thx
>
>[...]
>-- 
>Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
>If you think the mosad wants you dead since a long time then you are either
>wrong or dead since a long time.
Michael Niedermayer Feb. 6, 2020, 1:55 p.m. UTC | #3 
On Thu, Feb 06, 2020 at 02:59:03PM +0530, Jai Luthra wrote:
> On Tue, Feb 04, 2020 at 10:24:50PM +0100, Michael Niedermayer wrote:
> >On Mon, Feb 03, 2020 at 12:33:00AM +0530, Jai Luthra wrote:
> >>When no codebook is used, huff_lsbs can be more than 24 and still decode to
> >>original values once filters are applied.
> >
> >huff_lsbs can but get_bits() is limited to 25, you need get_bits_long() beyond
> 
> I see, thx Michael!
> 
> There are two ways to deal with this:
> 
> 1) Use get_bits_long() where get_bits() is used to read the lsb_bits from
> the bitstream as it is certainly going to be within 32.
> 
> 2) Limit huff_lsbs to 25 bits even if no codebook.
> 
> I tested encoding various samples and it does not go above 25 bits. It hits
> 25 in mlpenc because at the matrixing stage L+R channels get added together
> in a single channel, but I am not sure if other encoders would also adhere
> to that.
> 
> (I was previously wrong in assuming that it hits more than 24 at the
> filtering stage, I tested it [1] and it does so after the re-matrixing
> stage, before any FIR filters)
> 
> I prefer (1) as it is cleaner. Let me know if that is OK and I'll send a
> patch.

whatever you prefer that fixes the issue

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/mlpdec.c b/libavcodec/mlpdec.c
index 39c40914cd..22a6efd63d 100644
--- a/libavcodec/mlpdec.c
+++ b/libavcodec/mlpdec.c
@@ -829,7 +829,7 @@  static int read_channel_params(MLPDecodeContext *m, unsigned int substr,
     cp->codebook  = get_bits(gbp, 2);
     cp->huff_lsbs = get_bits(gbp, 5);
 
-    if (cp->huff_lsbs > 24) {
+    if (cp->codebook > 0 && cp->huff_lsbs > 24) {
         av_log(m->avctx, AV_LOG_ERROR, "Invalid huff_lsbs.\n");
         cp->huff_lsbs = 0;
         return AVERROR_INVALIDDATA;
diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
index af04648097..4e60ffd217 100644
--- a/libavcodec/mlpenc.c
+++ b/libavcodec/mlpenc.c
@@ -987,6 +987,9 @@  static void write_decoding_params(MLPEncodeContext *ctx, PutBitContext *pb,
                     put_bits(pb, 1, 0);
                 }
             }
+            if (cp->codebook > 0 && cp->huff_lsbs > 24) {
+                av_log(ctx->avctx, AV_LOG_ERROR, "Invalid Huff LSBs\n");
+            }
 
             put_bits(pb, 2, cp->codebook );
             put_bits(pb, 5, cp->huff_lsbs);