[FFmpeg-devel,1/5] mlpenc: fix lossless check error in number_sbits

Submitted by Jai Luthra on July 9, 2019, 8:18 p.m.

Details

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

Commit Message

Jai Luthra July 9, 2019, 8:18 p.m.
we need two bits instead of one bit to represent -1 in bitstream

Signed-off-by: Jai Luthra <me@jailuthra.in>
---
 libavcodec/mlpenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lynne July 9, 2019, 10:14 p.m.
Jul 9, 2019, 9:18 PM by me@jailuthra.in:

> we need two bits instead of one bit to represent -1 in bitstream
>
> Signed-off-by: Jai Luthra <me@jailuthra.in>
> ---
>  libavcodec/mlpenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
> index deb171645c..f4948451f1 100644
> --- a/libavcodec/mlpenc.c
> +++ b/libavcodec/mlpenc.c
> @@ -466,7 +466,7 @@ static void default_decoding_params(MLPEncodeContext *ctx,
>  */
>  static int inline number_sbits(int number)
>  {
> -    if (number < 0)
> +    if (number < -1)
>  number++; 
>

This is different from the first patch's version. Sure its correct now?
Jai Luthra July 10, 2019, 3:44 a.m.
On Wed, Jul 10, 2019 at 12:14:56AM +0200, Lynne wrote:
>Jul 9, 2019, 9:18 PM by me@jailuthra.in:
>
>> we need two bits instead of one bit to represent -1 in bitstream
>>
>> Signed-off-by: Jai Luthra <me@jailuthra.in>
>> ---
>>  libavcodec/mlpenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
>> index deb171645c..f4948451f1 100644
>> --- a/libavcodec/mlpenc.c
>> +++ b/libavcodec/mlpenc.c
>> @@ -466,7 +466,7 @@ static void default_decoding_params(MLPEncodeContext *ctx,
>>  */
>>  static int inline number_sbits(int number)
>>  {
>> -    if (number < 0)
>> +    if (number < -1)
>>  number++;
>>
>
>This is different from the first patch's version. Sure its correct now?

Yep. Previous patch produced valid bitstream too, but this provides better 
compression [1] by representing numbers of the form -2^x with one less bit for 
x >= 1.

This makes more sense, as we can represent -2 in two-bit twos-complement 
notation as `10` so output should be 2 bits (instead of 3 by previous patch). 
(similarly for -4, -8, -16, ...)

The lossless errors were being caused when a block of samples were all either 
-1 or 0. This function implied all samples could be represented as single bit 
each, but down the pipeline after huff vlc calculations, the encoder pushed 
<nothing> on the bitstream for all samples, which was always interpreted as 0 
by the decoder and never -1.

NB: One can argue -1 and 0 in fact can be represented in a single bit as two's 
complement, 0 being `0` and -1 being `1`. But imho, single bit two's 
complement is a weird boundary case, and not considering it solves the issue 
here. If someone has a better idea pls suggest.

[1]: tested using both patches on 
https://samples.ffmpeg.org/flac/When%20I%20Grow%20Up.flac. Previous patch 
compressed it to 28788216 byte MLP stream, this one compresses it to 28787834 
byte MLP stream. Both streams are valid and decode to lossless bit-exact 
output.

thx

--
jlut

Patch hide | download patch | download mbox

diff --git a/libavcodec/mlpenc.c b/libavcodec/mlpenc.c
index deb171645c..f4948451f1 100644
--- a/libavcodec/mlpenc.c
+++ b/libavcodec/mlpenc.c
@@ -466,7 +466,7 @@  static void default_decoding_params(MLPEncodeContext *ctx,
  */
 static int inline number_sbits(int number)
 {
-    if (number < 0)
+    if (number < -1)
         number++;
 
     return av_log2(FFABS(number)) + 1 + !!number;