[FFmpeg-devel] avcodec/mlz: simplify

Submitted by Paul B Mahol on July 1, 2017, 10:05 a.m.

Details

Message ID 20170701100552.17049-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol July 1, 2017, 10:05 a.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/mlz.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Michael Niedermayer July 1, 2017, 12:14 p.m.
On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/mlz.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
> index ebce796..715ea5c 100644
> --- a/libavcodec/mlz.c
> +++ b/libavcodec/mlz.c
> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char *buff, int string_code, int *fi
>  }
>  
>  static int input_code(GetBitContext* gb, int len) {
> -    int tmp_code = 0;
> -    int i;
> -    for (i = 0; i < len; ++i) {
> -        tmp_code |= get_bits1(gb) << i;
> -    }
> -    return tmp_code;
> +    return get_bitsz(gb, len);

have you tested this ? (seems not triggered in fate, i ve only found
fuzzed samples that trigger this with len >= 1 quickly)
isnt this the opposite endianness ?


[...]
Paul B Mahol July 1, 2017, 12:18 p.m.
On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>  libavcodec/mlz.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>> index ebce796..715ea5c 100644
>> --- a/libavcodec/mlz.c
>> +++ b/libavcodec/mlz.c
>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
>> *buff, int string_code, int *fi
>>  }
>>
>>  static int input_code(GetBitContext* gb, int len) {
>> -    int tmp_code = 0;
>> -    int i;
>> -    for (i = 0; i < len; ++i) {
>> -        tmp_code |= get_bits1(gb) << i;
>> -    }
>> -    return tmp_code;
>> +    return get_bitsz(gb, len);
>
> have you tested this ? (seems not triggered in fate, i ve only found
> fuzzed samples that trigger this with len >= 1 quickly)
> isnt this the opposite endianness ?

I created file with 22rev2 als encoder and decoding produce same hash,
before and after this patch.

ALS uses big endian bit reader.
Michael Niedermayer July 1, 2017, 12:36 p.m.
On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> ---
> >>  libavcodec/mlz.c | 7 +------
> >>  1 file changed, 1 insertion(+), 6 deletions(-)
> >>
> >> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
> >> index ebce796..715ea5c 100644
> >> --- a/libavcodec/mlz.c
> >> +++ b/libavcodec/mlz.c
> >> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
> >> *buff, int string_code, int *fi
> >>  }
> >>
> >>  static int input_code(GetBitContext* gb, int len) {
> >> -    int tmp_code = 0;
> >> -    int i;
> >> -    for (i = 0; i < len; ++i) {
> >> -        tmp_code |= get_bits1(gb) << i;
> >> -    }
> >> -    return tmp_code;
> >> +    return get_bitsz(gb, len);
> >
> > have you tested this ? (seems not triggered in fate, i ve only found
> > fuzzed samples that trigger this with len >= 1 quickly)
> > isnt this the opposite endianness ?
> 
> I created file with 22rev2 als encoder and decoding produce same hash,
> before and after this patch.
> 
> ALS uses big endian bit reader.

ok, but please change the commit message then as the code before and
afterwards dont read in the same endianness so this is not a
simplification but a bugfix then

[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
Paul B Mahol July 1, 2017, 12:42 p.m.
On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>> >> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >> ---
>> >>  libavcodec/mlz.c | 7 +------
>> >>  1 file changed, 1 insertion(+), 6 deletions(-)
>> >>
>> >> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>> >> index ebce796..715ea5c 100644
>> >> --- a/libavcodec/mlz.c
>> >> +++ b/libavcodec/mlz.c
>> >> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
>> >> *buff, int string_code, int *fi
>> >>  }
>> >>
>> >>  static int input_code(GetBitContext* gb, int len) {
>> >> -    int tmp_code = 0;
>> >> -    int i;
>> >> -    for (i = 0; i < len; ++i) {
>> >> -        tmp_code |= get_bits1(gb) << i;
>> >> -    }
>> >> -    return tmp_code;
>> >> +    return get_bitsz(gb, len);
>> >
>> > have you tested this ? (seems not triggered in fate, i ve only found
>> > fuzzed samples that trigger this with len >= 1 quickly)
>> > isnt this the opposite endianness ?
>>
>> I created file with 22rev2 als encoder and decoding produce same hash,
>> before and after this patch.
>>
>> ALS uses big endian bit reader.
>
> ok, but please change the commit message then as the code before and
> afterwards dont read in the same endianness so this is not a
> simplification but a bugfix then

Hmm, on second look you are probably right.
They changed to this in R23 version and I failed to create file that
triggers this path.
So I will not apply this mostly because R23 support needs more work.
Thilo Borgmann July 1, 2017, 4:47 p.m.
Am 01.07.17 um 14:42 schrieb Paul B Mahol:
> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>> ---
>>>>>  libavcodec/mlz.c | 7 +------
>>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>>>>> index ebce796..715ea5c 100644
>>>>> --- a/libavcodec/mlz.c
>>>>> +++ b/libavcodec/mlz.c
>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
>>>>> *buff, int string_code, int *fi
>>>>>  }
>>>>>
>>>>>  static int input_code(GetBitContext* gb, int len) {
>>>>> -    int tmp_code = 0;
>>>>> -    int i;
>>>>> -    for (i = 0; i < len; ++i) {
>>>>> -        tmp_code |= get_bits1(gb) << i;
>>>>> -    }
>>>>> -    return tmp_code;
>>>>> +    return get_bitsz(gb, len);
>>>>
>>>> have you tested this ? (seems not triggered in fate, i ve only found
>>>> fuzzed samples that trigger this with len >= 1 quickly)
>>>> isnt this the opposite endianness ?
>>>
>>> I created file with 22rev2 als encoder and decoding produce same hash,
>>> before and after this patch.
>>>
>>> ALS uses big endian bit reader.
>>
>> ok, but please change the commit message then as the code before and
>> afterwards dont read in the same endianness so this is not a
>> simplification but a bugfix then
> 
> Hmm, on second look you are probably right.
> They changed to this in R23 version and I failed to create file that
> triggers this path.
> So I will not apply this mostly because R23 support needs more work.

Do I understand correctly, this patch works with RM22 only and fails on RM23?

-Thilo
Paul B Mahol July 1, 2017, 5:03 p.m.
On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> Am 01.07.17 um 14:42 schrieb Paul B Mahol:
>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>> ---
>>>>>>  libavcodec/mlz.c | 7 +------
>>>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>>>>>> index ebce796..715ea5c 100644
>>>>>> --- a/libavcodec/mlz.c
>>>>>> +++ b/libavcodec/mlz.c
>>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
>>>>>> *buff, int string_code, int *fi
>>>>>>  }
>>>>>>
>>>>>>  static int input_code(GetBitContext* gb, int len) {
>>>>>> -    int tmp_code = 0;
>>>>>> -    int i;
>>>>>> -    for (i = 0; i < len; ++i) {
>>>>>> -        tmp_code |= get_bits1(gb) << i;
>>>>>> -    }
>>>>>> -    return tmp_code;
>>>>>> +    return get_bitsz(gb, len);
>>>>>
>>>>> have you tested this ? (seems not triggered in fate, i ve only found
>>>>> fuzzed samples that trigger this with len >= 1 quickly)
>>>>> isnt this the opposite endianness ?
>>>>
>>>> I created file with 22rev2 als encoder and decoding produce same hash,
>>>> before and after this patch.
>>>>
>>>> ALS uses big endian bit reader.
>>>
>>> ok, but please change the commit message then as the code before and
>>> afterwards dont read in the same endianness so this is not a
>>> simplification but a bugfix then
>>
>> Hmm, on second look you are probably right.
>> They changed to this in R23 version and I failed to create file that
>> triggers this path.
>> So I will not apply this mostly because R23 support needs more work.
>
> Do I understand correctly, this patch works with RM22 only and fails on
> RM23?

Probably. I can not confirm because I failed to create sample with
path that triggers this code.
Thilo Borgmann July 2, 2017, 2:05 p.m.
Am 01.07.17 um 19:03 schrieb Paul B Mahol:
> On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>> Am 01.07.17 um 14:42 schrieb Paul B Mahol:
>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
>>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>> ---
>>>>>>>  libavcodec/mlz.c | 7 +------
>>>>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>>>>>>> index ebce796..715ea5c 100644
>>>>>>> --- a/libavcodec/mlz.c
>>>>>>> +++ b/libavcodec/mlz.c
>>>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned char
>>>>>>> *buff, int string_code, int *fi
>>>>>>>  }
>>>>>>>
>>>>>>>  static int input_code(GetBitContext* gb, int len) {
>>>>>>> -    int tmp_code = 0;
>>>>>>> -    int i;
>>>>>>> -    for (i = 0; i < len; ++i) {
>>>>>>> -        tmp_code |= get_bits1(gb) << i;
>>>>>>> -    }
>>>>>>> -    return tmp_code;
>>>>>>> +    return get_bitsz(gb, len);
>>>>>>
>>>>>> have you tested this ? (seems not triggered in fate, i ve only found
>>>>>> fuzzed samples that trigger this with len >= 1 quickly)
>>>>>> isnt this the opposite endianness ?
>>>>>
>>>>> I created file with 22rev2 als encoder and decoding produce same hash,
>>>>> before and after this patch.
>>>>>
>>>>> ALS uses big endian bit reader.
>>>>
>>>> ok, but please change the commit message then as the code before and
>>>> afterwards dont read in the same endianness so this is not a
>>>> simplification but a bugfix then
>>>
>>> Hmm, on second look you are probably right.
>>> They changed to this in R23 version and I failed to create file that
>>> triggers this path.
>>> So I will not apply this mostly because R23 support needs more work.
>>
>> Do I understand correctly, this patch works with RM22 only and fails on
>> RM23?
> 
> Probably. I can not confirm because I failed to create sample with
> path that triggers this code.

Well it should get applied only if it could properly be tested for both
endianness cases.

-Thilo
Paul B Mahol July 2, 2017, 2:22 p.m.
On 7/2/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
> Am 01.07.17 um 19:03 schrieb Paul B Mahol:
>> On 7/1/17, Thilo Borgmann <thilo.borgmann@mail.de> wrote:
>>> Am 01.07.17 um 14:42 schrieb Paul B Mahol:
>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>> On Sat, Jul 01, 2017 at 02:18:17PM +0200, Paul B Mahol wrote:
>>>>>> On 7/1/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>> On Sat, Jul 01, 2017 at 12:05:52PM +0200, Paul B Mahol wrote:
>>>>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>>>>> ---
>>>>>>>>  libavcodec/mlz.c | 7 +------
>>>>>>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
>>>>>>>> index ebce796..715ea5c 100644
>>>>>>>> --- a/libavcodec/mlz.c
>>>>>>>> +++ b/libavcodec/mlz.c
>>>>>>>> @@ -112,12 +112,7 @@ static int decode_string(MLZ* mlz, unsigned
>>>>>>>> char
>>>>>>>> *buff, int string_code, int *fi
>>>>>>>>  }
>>>>>>>>
>>>>>>>>  static int input_code(GetBitContext* gb, int len) {
>>>>>>>> -    int tmp_code = 0;
>>>>>>>> -    int i;
>>>>>>>> -    for (i = 0; i < len; ++i) {
>>>>>>>> -        tmp_code |= get_bits1(gb) << i;
>>>>>>>> -    }
>>>>>>>> -    return tmp_code;
>>>>>>>> +    return get_bitsz(gb, len);
>>>>>>>
>>>>>>> have you tested this ? (seems not triggered in fate, i ve only found
>>>>>>> fuzzed samples that trigger this with len >= 1 quickly)
>>>>>>> isnt this the opposite endianness ?
>>>>>>
>>>>>> I created file with 22rev2 als encoder and decoding produce same hash,
>>>>>> before and after this patch.
>>>>>>
>>>>>> ALS uses big endian bit reader.
>>>>>
>>>>> ok, but please change the commit message then as the code before and
>>>>> afterwards dont read in the same endianness so this is not a
>>>>> simplification but a bugfix then
>>>>
>>>> Hmm, on second look you are probably right.
>>>> They changed to this in R23 version and I failed to create file that
>>>> triggers this path.
>>>> So I will not apply this mostly because R23 support needs more work.
>>>
>>> Do I understand correctly, this patch works with RM22 only and fails on
>>> RM23?
>>
>> Probably. I can not confirm because I failed to create sample with
>> path that triggers this code.
>
> Well it should get applied only if it could properly be tested for both
> endianness cases.

Yes, until then I will not apply this. As float code works fine with R22rev2.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mlz.c b/libavcodec/mlz.c
index ebce796..715ea5c 100644
--- a/libavcodec/mlz.c
+++ b/libavcodec/mlz.c
@@ -112,12 +112,7 @@  static int decode_string(MLZ* mlz, unsigned char *buff, int string_code, int *fi
 }
 
 static int input_code(GetBitContext* gb, int len) {
-    int tmp_code = 0;
-    int i;
-    for (i = 0; i < len; ++i) {
-        tmp_code |= get_bits1(gb) << i;
-    }
-    return tmp_code;
+    return get_bitsz(gb, len);
 }
 
 int ff_mlz_decompression(MLZ* mlz, GetBitContext* gb, int size, unsigned char *buff) {