| Message ID | 3992956b-2511-fcbf-51f0-7f4627b55f0f@googlemail.com |
|---|---|
| State | New |
| Headers | show |
2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavutil/opt.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavutil/opt.c b/libavutil/opt.c > index f855ccb..f713d3f 100644 > --- a/libavutil/opt.c > +++ b/libavutil/opt.c > @@ -32,6 +32,7 @@ > #include "common.h" > #include "dict.h" > #include "eval.h" > +#include "imgutils.h" > #include "log.h" > #include "parseutils.h" > #include "pixdesc.h" > @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, > return 0; > } > ret = av_parse_video_size(dst, dst + 1, val); > - if (ret < 0) > + if (ret < 0) { > av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); > + return ret; > + } > + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); > + if (ret < 0) { > + *dst = 0; > + *(dst + 1) = 0; > + } Doesn't this break valid usecases? Carl Eugen
On Sat, 10 Dec 2016 16:11:15 +0100 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > --- > libavutil/opt.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavutil/opt.c b/libavutil/opt.c > index f855ccb..f713d3f 100644 > --- a/libavutil/opt.c > +++ b/libavutil/opt.c > @@ -32,6 +32,7 @@ > #include "common.h" > #include "dict.h" > #include "eval.h" > +#include "imgutils.h" > #include "log.h" > #include "parseutils.h" > #include "pixdesc.h" > @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, > return 0; > } > ret = av_parse_video_size(dst, dst + 1, val); > - if (ret < 0) > + if (ret < 0) { > av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); > + return ret; > + } > + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); > + if (ret < 0) { > + *dst = 0; > + *(dst + 1) = 0; > + } > return ret; > } > I'd argue that rather than doing this, image allocation functions etc. should error out if the dimensions are too large. This way the allowed dimensions could be enlarged (e.g. by taking the pixel format into account) without changing everything from int to size_t.
On Sat, Dec 10, 2016 at 4:26 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Sat, 10 Dec 2016 16:11:15 +0100 > Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavutil/opt.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/libavutil/opt.c b/libavutil/opt.c >> index f855ccb..f713d3f 100644 >> --- a/libavutil/opt.c >> +++ b/libavutil/opt.c >> @@ -32,6 +32,7 @@ >> #include "common.h" >> #include "dict.h" >> #include "eval.h" >> +#include "imgutils.h" >> #include "log.h" >> #include "parseutils.h" >> #include "pixdesc.h" >> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, >> return 0; >> } >> ret = av_parse_video_size(dst, dst + 1, val); >> - if (ret < 0) >> + if (ret < 0) { >> av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); >> + return ret; >> + } >> + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); >> + if (ret < 0) { >> + *dst = 0; >> + *(dst + 1) = 0; >> + } >> return ret; >> } >> > > I'd argue that rather than doing this, image allocation functions etc. > should error out if the dimensions are too large. > > This way the allowed dimensions could be enlarged (e.g. by taking the > pixel format into account) without changing everything from int to > size_t. I agree, av_image_check_size has some arbitrary technical limits in it right now that may not necessarily apply everywhere you want to set an image size. Allocations would need to be checked anyway with or without this check because most options accessible through AVOptions are also accessible through other means, so I would rather leave the decision of the maximum size up to the code that uses these values. - Hendrik
On 10.12.2016 16:19, Carl Eugen Hoyos wrote: > 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavutil/opt.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/libavutil/opt.c b/libavutil/opt.c >> index f855ccb..f713d3f 100644 >> --- a/libavutil/opt.c >> +++ b/libavutil/opt.c >> @@ -32,6 +32,7 @@ >> #include "common.h" >> #include "dict.h" >> #include "eval.h" >> +#include "imgutils.h" >> #include "log.h" >> #include "parseutils.h" >> #include "pixdesc.h" >> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, >> return 0; >> } >> ret = av_parse_video_size(dst, dst + 1, val); >> - if (ret < 0) >> + if (ret < 0) { >> av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); >> + return ret; >> + } >> + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); >> + if (ret < 0) { >> + *dst = 0; >> + *(dst + 1) = 0; >> + } > > Doesn't this break valid usecases? None that I'm aware of. Which usecases were you thinking about? Best regards, Andreas
On 10.12.2016 16:26, wm4 wrote: > On Sat, 10 Dec 2016 16:11:15 +0100 > Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> --- >> libavutil/opt.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/libavutil/opt.c b/libavutil/opt.c >> index f855ccb..f713d3f 100644 >> --- a/libavutil/opt.c >> +++ b/libavutil/opt.c >> @@ -32,6 +32,7 @@ >> #include "common.h" >> #include "dict.h" >> #include "eval.h" >> +#include "imgutils.h" >> #include "log.h" >> #include "parseutils.h" >> #include "pixdesc.h" >> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, >> return 0; >> } >> ret = av_parse_video_size(dst, dst + 1, val); >> - if (ret < 0) >> + if (ret < 0) { >> av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); >> + return ret; >> + } >> + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); >> + if (ret < 0) { >> + *dst = 0; >> + *(dst + 1) = 0; >> + } >> return ret; >> } >> > > I'd argue that rather than doing this, image allocation functions etc. > should error out if the dimensions are too large. That can be done in addition to this. > This way the allowed dimensions could be enlarged (e.g. by taking the > pixel format into account) without changing everything from int to > size_t. 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. Best regards, Andreas
2016-12-10 16:54 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> Allocations would need to be checked anyway with or without this check
Do you think this is related?
Carl Eugen
2016-12-10 16:55 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: > On 10.12.2016 16:19, Carl Eugen Hoyos wrote: >> 2016-12-10 16:11 GMT+01:00 Andreas Cadhalpun <andreas.cadhalpun@googlemail.com>: >>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>> --- >>> libavutil/opt.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c >>> index f855ccb..f713d3f 100644 >>> --- a/libavutil/opt.c >>> +++ b/libavutil/opt.c >>> @@ -32,6 +32,7 @@ >>> #include "common.h" >>> #include "dict.h" >>> #include "eval.h" >>> +#include "imgutils.h" >>> #include "log.h" >>> #include "parseutils.h" >>> #include "pixdesc.h" >>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, >>> return 0; >>> } >>> ret = av_parse_video_size(dst, dst + 1, val); >>> - if (ret < 0) >>> + if (ret < 0) { >>> av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); >>> + return ret; >>> + } >>> + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); >>> + if (ret < 0) { >>> + *dst = 0; >>> + *(dst + 1) = 0; >>> + } >> >> Doesn't this break valid usecases? > > None that I'm aware of. Which usecases were you thinking about? I thought that if this check is more strict than it was, there must be a usecase that breaks but I wasn't immediately successful in finding one. Carl Eugen
On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > On 10.12.2016 16:26, wm4 wrote: >> On Sat, 10 Dec 2016 16:11:15 +0100 >> Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: >> >>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >>> --- >>> libavutil/opt.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavutil/opt.c b/libavutil/opt.c >>> index f855ccb..f713d3f 100644 >>> --- a/libavutil/opt.c >>> +++ b/libavutil/opt.c >>> @@ -32,6 +32,7 @@ >>> #include "common.h" >>> #include "dict.h" >>> #include "eval.h" >>> +#include "imgutils.h" >>> #include "log.h" >>> #include "parseutils.h" >>> #include "pixdesc.h" >>> @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, >>> return 0; >>> } >>> ret = av_parse_video_size(dst, dst + 1, val); >>> - if (ret < 0) >>> + if (ret < 0) { >>> av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); >>> + return ret; >>> + } >>> + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); >>> + if (ret < 0) { >>> + *dst = 0; >>> + *(dst + 1) = 0; >>> + } >>> return ret; >>> } >>> >> >> I'd argue that rather than doing this, image allocation functions etc. >> should error out if the dimensions are too large. > > That can be done in addition to this. > >> This way the allowed dimensions could be enlarged (e.g. by taking the >> pixel format into account) without changing everything from int to >> size_t. > > 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? Also, overflow what range exactly? int64? 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. 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. - Hendrik
On 10.12.2016 17:29, Hendrik Leppkes wrote: > On Sat, Dec 10, 2016 at 4:55 PM, Andreas Cadhalpun > <andreas.cadhalpun@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? > 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. Best regards, Andreas
On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun <andreas.cadhalpun@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@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. > >> 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 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). 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. - Hendrik
On 10.12.2016 18:40, Hendrik Leppkes wrote: > On Sat, Dec 10, 2016 at 6:20 PM, Andreas Cadhalpun > <andreas.cadhalpun@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@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
On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun <andreas.cadhalpun@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@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@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
On 11.12.2016 10:04, Hendrik Leppkes wrote: > On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun > <andreas.cadhalpun@googlemail.com> wrote: >> On 10.12.2016 18:40, Hendrik Leppkes wrote: >>> 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) Actually there is a factor of 64 between AV_PIX_FMT_MONOWHITE and AV_PIX_FMT_AYUV64LE, the latter of which amounts to the old limit. > 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. And I still think that av_image_check_size should be changed to accept the largest value valid for any pixel format (once every place needing stricter limits is switched to the pixel format sensitive check). Do you disagree with this or what is your point? Best regards, Andreas
On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun <andreas.cadhalpun@googlemail.com> wrote: > On 11.12.2016 10:04, Hendrik Leppkes wrote: >> On Sat, Dec 10, 2016 at 6:53 PM, Andreas Cadhalpun >> <andreas.cadhalpun@googlemail.com> wrote: >>> On 10.12.2016 18:40, Hendrik Leppkes wrote: >>>> 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) > > Actually there is a factor of 64 between AV_PIX_FMT_MONOWHITE and > AV_PIX_FMT_AYUV64LE, the latter of which amounts to the old limit. > >> 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. > > And I still think that av_image_check_size should be changed to > accept the largest value valid for any pixel format (once every place > needing stricter limits is switched to the pixel format sensitive > check). > Do you disagree with this or what is your point? I could probably live with a simple w*h overflow check (more or less), which av_image_check_size then probably would end up being if I understand you right? Thats much higher then the current limit in most cases and while we should try to move this to size_t/ptrdiff_t eventually to lift the limit even higher on 64-bit platforms, its OK for now. - Hendrik
On 11.12.2016 21:03, Hendrik Leppkes wrote: > On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun > <andreas.cadhalpun@googlemail.com> wrote: >> On 11.12.2016 10:04, Hendrik Leppkes wrote: >>> 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. >> >> And I still think that av_image_check_size should be changed to >> accept the largest value valid for any pixel format (once every place >> needing stricter limits is switched to the pixel format sensitive >> check). >> Do you disagree with this or what is your point? > > I could probably live with a simple w*h overflow check (more or less), > which av_image_check_size then probably would end up being if I > understand you right? I don't think so. For example, av_image_check_size2 accepts resolutions like 100000x80000 for AV_PIX_FMT_MONOWHITE and thus av_image_check_size should also accept this, even though the number of pixels is larger than INT_MAX. However, that's not the current state of affairs, so until the work is done to actually use the pixel format specific limits, the option code should check for the old limit. > Thats much higher then the current limit in most cases and while we > should try to move this to size_t/ptrdiff_t eventually to lift the > limit even higher on 64-bit platforms, its OK for now. Note that av_image_check_size is documented to check that "all bytes of the image can be addressed with a signed int", so increasing the limit higher requires using a different function. Best regards, Andreas
On 11.12.2016 21:45, Andreas Cadhalpun wrote: > On 11.12.2016 21:03, Hendrik Leppkes wrote: >> On Sun, Dec 11, 2016 at 7:48 PM, Andreas Cadhalpun >> <andreas.cadhalpun@googlemail.com> wrote: >>> On 11.12.2016 10:04, Hendrik Leppkes wrote: >>>> 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. >>> >>> And I still think that av_image_check_size should be changed to >>> accept the largest value valid for any pixel format (once every place >>> needing stricter limits is switched to the pixel format sensitive >>> check). >>> Do you disagree with this or what is your point? >> >> I could probably live with a simple w*h overflow check (more or less), >> which av_image_check_size then probably would end up being if I >> understand you right? > > I don't think so. For example, av_image_check_size2 accepts resolutions > like 100000x80000 for AV_PIX_FMT_MONOWHITE and thus av_image_check_size > should also accept this, even though the number of pixels is larger > than INT_MAX. However, that's not the current state of affairs, so > until the work is done to actually use the pixel format specific limits, > the option code should check for the old limit. > >> Thats much higher then the current limit in most cases and while we >> should try to move this to size_t/ptrdiff_t eventually to lift the >> limit even higher on 64-bit platforms, its OK for now. > > Note that av_image_check_size is documented to check that > "all bytes of the image can be addressed with a signed int", > so increasing the limit higher requires using a different function. I assume I've convinced you, so I'll apply this patch soon, unless I hear back from you. Best regards, Andreas
diff --git a/libavutil/opt.c b/libavutil/opt.c index f855ccb..f713d3f 100644 --- a/libavutil/opt.c +++ b/libavutil/opt.c @@ -32,6 +32,7 @@ #include "common.h" #include "dict.h" #include "eval.h" +#include "imgutils.h" #include "log.h" #include "parseutils.h" #include "pixdesc.h" @@ -325,8 +326,15 @@ static int set_string_image_size(void *obj, const AVOption *o, const char *val, return 0; } ret = av_parse_video_size(dst, dst + 1, val); - if (ret < 0) + if (ret < 0) { av_log(obj, AV_LOG_ERROR, "Unable to parse option value \"%s\" as image size\n", val); + return ret; + } + ret = av_image_check_size(*dst, *(dst + 1), 0, obj); + if (ret < 0) { + *dst = 0; + *(dst + 1) = 0; + } return ret; }
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavutil/opt.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)