[FFmpeg-devel] libavcodec/als: remove check for predictor order of a block

[Umair Khan]

Hi,

On Wed, Nov 15, 2017 at 4:20 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> 2017-11-14 23:48 GMT+01:00 Carl Eugen Hoyos <ceffmpeg at gmail.com>:
>> 2017-11-13 21:07 GMT+01:00 Thilo Borgmann <thilo.borgmann at mail.de>:
>>> Am 13.11.17 um 21:06 schrieb Thilo Borgmann:
>>>> Am 13.11.17 um 20:02 schrieb Umair Khan:
>>>>> Hi,
>>>>>
>>>>> On Mon, Nov 13, 2017 at 11:06 PM, Thilo Borgmann <thilo.borgmann at mail.de> wrote:
>>>>>> Hi,
>>>>>>
>>>>>>> On Mon, Nov 13, 2017 at 1:09 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>>>>>> 2017-11-12 20:30 GMT+01:00 Umair Khan <omerjerk at gmail.com>:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Mon, Nov 13, 2017 at 12:45 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>>>>>>>>>> 2017-11-12 20:05 GMT+01:00 Umair Khan <omerjerk at gmail.com>:
>>>>>>>>>>
>>>>>>>>>>> The attached patch fixes the address sanitizer issue.
>>>>>>>>>>
>>>>>>>>>> Breaks compilation here, how did you test?
>>>>>>>>>>
>>>>>>>>>> libavcodec/alsdec.c: In function ‘decode_var_block_data’:
>>>>>>>>>> libavcodec/alsdec.c:938:7: error: expected ‘}’ before ‘else’
>>>>>>>>>
>>>>>>>>> Sorry for the faulty patch. Here is the fixed one.
>>>>>>>>
>>>>>>>> The commit message of your patch is:
>>>>>>>> libavcodec/als: fix address sanitization error in decoder
>>>>>>>>
>>>>>>>> Is there an error in current FFmpeg git head that asan
>>>>>>>> shows? If not, the commit message makes no sense.
>>>>>>>>
>>>>>>>> I believe you should send two patches that are meant
>>>>>>>> to be committed together, one of them fixing ticket #6297.
>>>>>>>
>>>>>>> This is the complete patchset.
>>>>>>
>>>>>> I need some days to find time to test this, earliest during the weekend I fear...
>>>>>>
>>>>>> What happens for
>>>>>> block_length < residual_index < opt_order?
>>>>>
>>>>> I didn't really understand this case. What's residual_index? Can you
>>>>> point to the source exactly?
>>>>>
>>>>> As far as the case where opt_order is more than block_length, my
>>>>> second patch handles that case only. The file which Michael sent was
>>>>> having asan issues because of the case when block_length < opt_order.
>>>>>
>>>>>> Another way of asking would be, where is the second loop from specs page 30 for that case?
>>>>>> (ISO/IEC 14496)
>>>>>
>>>>> The second loop is just converting parcor to lpc coefficients which is
>>>>> done here - https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/alsdec.c#L935
>>>>>
>>>>>> I think what puzzles CE is, that the problematic if() from the other patches is still untouched by your patch. So how could this be a valid solution even if your patch would actually improve the prediction part...
>>>>>> And I wonder the same ;)
>>>>>
>>>>> As said, it is valid to have opt_order greater than block_length.
>>>>> However, the decoder loop needs to be checked because we won't predict
>>>>> values more than the length of the block i.e., block_length. We use
>>>>> last K (prediction order, opt_order) values to predict the original K
>>>>> values of the current block.
>>>>>
>>>>>> Did you run FATE with your patch applied? I assume a big difference in output at the first glance (means FATE aks the conformance files should fail...)
>>>>>
>>>>> Yes. I did run FATE. It passes perfectly.
>>>>>
>>>>>> Thanks for driving this forward anyway :)
>>>>>
>>>>> I think the two patches fix the issues completely. I don't see any
>>>>> harm in applying this patchset. :)
>>>>
>>>> Which second patch exactly do you want to be applied along with this one?
>>>
>>> For convenience just send a patch that does both the changes you want to be done...
>
> Sorry:
>
> Why do you want to merge two patches that are not necessarily related:
> Isn't one the revert that fixes a regression and one the new fix for the
> overread.

Just to reiterate, this thread contains all fo the emails with the patches.

One email has both of the patches.
And another email has one patch with both changes combined.

Feel free to apply which ever the one you want. I have run fate tests
and they work all fine. The logic also matches with what is written in
the spec.

I'm now moving to the ALS encoder tasks before we start with the next GSoC.

-Umair