[FFmpeg-devel] [PATCH] opt: check image size when setting it
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
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.
More information about the ffmpeg-devel