[FFmpeg-devel] [libav-devel] [PATCH] aacsbr: break infinite loop in sbr_hf_calc_npatches

[Andreas Cadhalpun]

On 22.04.2015 18:31, Claudio Freire wrote:
> On Wed, Apr 22, 2015 at 12:54 PM, Andreas Cadhalpun
> <andreas.cadhalpun at googlemail.com> wrote:
>> On 22.04.2015 17:35, Claudio Freire wrote:
>>> On Wed, Apr 22, 2015 at 10:23 AM, Andreas Cadhalpun
>>> <andreas.cadhalpun at googlemail.com> wrote:
>>>> +        if (k == last_k && msb == last_msb) {
>>>> +            av_log(ac->avctx, AV_LOG_ERROR, "patch construction failed\n");
>>>> +            return AVERROR_INVALIDDATA;
>>>> +        }
>>>> +        last_k = k;
>>>> +        last_msb = msb;
>>>
>>>
>>> I don't think the INVALIDDATA return will have the desired effect.
>>>
>>> I think you want return -1;
>>
>> This function is only called once and there the check is:
>>     if (sbr_hf_calc_npatches(ac, sbr) < 0)
>>         return -1;
>>
>> Thus returning AVERROR_INVALIDDATA works as well as -1.
> 
> The fact that AVERROR_INVALIDDATA < 0 is a close call on 32 bit platforms.
> 
> Still, it's not a new assumption in the code, so I'll grant you that.

I think there is more generally the assumption that all error codes are negative.

> With the disclaimer that I'm not familiar with this code said, it
> looks like it would be better to attack the reason why it loops
> without increasing npatches

The condition for increasing num_patches never becomes true:
        sbr->patch_num_subbands[sbr->num_patches]  = FFMAX(sb - usb, 0);
Here sb = usb ...
        sbr->patch_start_subband[sbr->num_patches] = sbr->k[0] - odd - sbr->patch_num_subbands[sbr->num_patches];

        if (sbr->patch_num_subbands[sbr->num_patches] > 0) {
... thus this condition is false.
            usb = sb;
            msb = sb;
            sbr->num_patches++;
        } else
            msb = sbr->kx[1];

If you have any other idea how to fix this, please let me know.

> rather than a bandaid like this, but aside
> from that preference (which is personal) the patch seems to make
> sense.

OK.

Best regards,
Andreas