On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun <andreas.cadhal...@googlemail.com> wrote: > On 10.12.2016 18:40, Hendrik Leppkes wrote: >> On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun >> <andreas.cadhal...@googlemail.com> wrote: >>> On 10.12.2016 17:29, Hendrik Leppkes wrote: >>>> On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun >>>> <andreas.cadhal...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
