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

[Claudio Freire]

On Thu, Apr 23, 2015 at 12:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Apr 23, 2015 at 12:22:49PM -0300, Claudio Freire wrote:
>> On Wed, Apr 22, 2015 at 1:52 PM, Vittorio Giovara
>> <vittorio.giovara at gmail.com> wrote:
>> >>>> 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.
>> >
>> > What do you mean by "A close call"? All AVERROR_* are negative by
>> > design, and they carry more information than a -1, so their increased
>> > usage is certainly welcome.
>> > The fact that it does not get propagated is a separate issue.
>>
>> Just that it's not obvious, and I was thinking error-prone, but now I
>> think it was just a reaction to it being non-obvious to me.
>>
>> So nothing relevant.
>
> ok, so is the patch the correct solution or are the fields already
> invalid before this loop and should have been checked prior ?

I think either the fields are already invalid before the loop (which
could mean an invalid file), or there is a slight bug on the loop, the
patch just avoids infinite loop in those cases.

What I'm yet unable to decide is whether it's the first or the second
case (invalid input file vs bugged loop). I'll have to carefully
review the specs for that, and sadly I only have drafts.