[FFmpeg-devel] [PATCH] opt: check image size when setting it

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun
<andreas.cadhalpun at googlemail.com> wrote:
> On 10.12.2016 17:29, Hendrik Leppkes wrote:
>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun
>> <andreas.cadhalpun at googlemail.com> wrote:
>>> If that is done, the hard limit in av_image_check_size should check for
>>> the maximum allowed value of any pixel format.
>>> But a check here is needed to make sure that width * height doesn't overflow.
>>
>> Why is that needed?
>
> Because the framework currently doesn't support larger sizes and setting
> options to invalid values doesn't make much sense.
>
>> Also, overflow what range exactly? int64?
>
> The range of valid image dimension, which is what av_image_check_size
> is documented to check.
>
>> The generic option code should not make any assumptions how the data is
>> going to be used. As long as its not multiplied right here and now,
>> there is no reason to check.
>
> It's a valid assumption that an option of type AV_OPT_TYPE_IMAGE_SIZE
> is used as image size, so it shouldn't accept values that are invalid
> dimensions in our framework. Also it already doesn't accept negative
> values. Would you prefer to remove this check?

Negative size is inherently invalid for image sizes, and not an
arbitrary limit, so that argument makes no sense.

>
>> As said in an earlier mail, the check doesn't negate the need to check
>> in other places, because AVOption is only one way to set values, so I
>> would rather prefer avoiding adding more artificial limits in very
>> generic code.
>
> The alternative is that every program setting the image size needs to
> check if it is valid before setting the option. That's quite inconvenient.

No, the point is that everything that _uses_ these values needs to
check it either way, so adding checks here doesn't seem to improve
anything and just adds extra burden when these limits are
changed/improved (say by making them pixfmt aware as discussed in
another thread, which this function could never know).

What exactly is this check supposed to fix/improve? Since all
libraries need to check it anyway and would error then, littering
earlier code paths with extra checks seems to not help much.

- Hendrik