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

[Hendrik Leppkes]

On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun
<andreas.cadhalpun at googlemail.com> wrote:
> 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.
>

av_image_check_size2 was introduced by Michael now which works in the
context of a pix_fmt, which for many formats allows significantly
larger images then the old function (up to factor 4 larger)
I still see the problem that this option code does not know which
pix_fmt the numbers relate to and as such would keep the old limit in
place despite there being no technical reasons for it.

- Hendrik