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. > 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
