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

Andreas Cadhalpun andreas.cadhalpun at googlemail.com
Sat Dec 10 19:53:58 EET 2016


On 10.12.2016 18:40, Hendrik Leppkes wrote:
> 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.

However, some file formats encode image sizes as negative values to
give them a special meaning like reversed image buffer. So it's not
entirely hypothetical to set height to a negative value.
(The current ffmpeg code does this and only later uses FFABS.)

>>> 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

The improvement is that width/height are at no point set to invalid values.

> 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).

There is no extra burden, because av_image_check_size would have to
be changed in that case anyway to accept the largest value valid
for any pixel format.

> 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.

In general, values should be checked for validity when they are set.

Best regards,
Andreas



More information about the ffmpeg-devel mailing list