| Message ID | 20170607074519.13780-1-kmark937@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
I also have to wonder if it would be advantageous to add the cast on the right side as well. That way the var_values variables will have the proper truncated values on future evaluations. Open to comments on that. On Wed, Jun 7, 2017 at 3:45 AM, Kevin Mark <kmark937@gmail.com> wrote: > - eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; > + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : res; to perhaps: + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : (int) res; Without that extra cast I assume the values in eval_w and var_values[VAR_OUT_W], var_values[VAR_OW] could be different. I doubt most users expect that those values could ever be non-integers which has implications for how you're writing your expression.
Still interested in thoughts on this patch/discussion. Thanks, Kevin On Wed, Jun 7, 2017 at 3:54 AM, Kevin Mark <kmark937@gmail.com> wrote: > I also have to wonder if it would be advantageous to add the cast on > the right side as well. That way the var_values variables will have > the proper truncated values on future evaluations. Open to comments on > that. > > On Wed, Jun 7, 2017 at 3:45 AM, Kevin Mark <kmark937@gmail.com> wrote: >> - eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; >> + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : res; > > to perhaps: > + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res > == 0 ? inlink->w : (int) res; > > Without that extra cast I assume the values in eval_w and > var_values[VAR_OUT_W], var_values[VAR_OW] could be different. I doubt > most users expect that those values could ever be non-integers which > has implications for how you're writing your expression.
On Wed, Jun 07, 2017 at 03:54:26AM -0400, Kevin Mark wrote: > I also have to wonder if it would be advantageous to add the cast on > the right side as well. That way the var_values variables will have > the proper truncated values on future evaluations. Open to comments on > that. > > On Wed, Jun 7, 2017 at 3:45 AM, Kevin Mark <kmark937@gmail.com> wrote: > > - eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; > > + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : res; > > to perhaps: > + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res > == 0 ? inlink->w : (int) res; > > Without that extra cast I assume the values in eval_w and > var_values[VAR_OUT_W], var_values[VAR_OW] could be different. I doubt > most users expect that those values could ever be non-integers which > has implications for how you're writing your expression. why is there a cast at all ? [...]
On Mon, Jun 12, 2017 at 9:42 PM, Michael Niedermayer
<michael@niedermayer.cc> wrote:
> why is there a cast at all ?
The cast is there because if you run this:
ffmpeg -frames:v 5 -filter_complex
"sws_flags=+accurate_rnd+bitexact;testsrc=size=320x240
[main];testsrc=size=640x360 [ref];[main][ref]
scale2ref=0:print(ow/641) [main][ref];[ref] nullsink" -map "[main]"
-flags +bitexact -fflags +bitexact -f md5 -
This works just fine without a cast for ow. 0 == 0 is true so we set
it to 640. But for oh, the print() shows that ow/641 is 0.998440. When
it is truncated from a double to an integer (eval_h = res) it becomes
0. But in our comparison, 0.998440 == 0 is false so in this case
eval_h will be truncated to 0 which is exactly the behavior we're
trying to correct. Adding that cast resolves the issue. 0.998440 == 0
is false but (int) 0.998440 == 0 is true.
For the extra cast I was talking about consider this:
ffmpeg -frames:v 5 -filter_complex
"sws_flags=+accurate_rnd+bitexact;testsrc=size=320x240
[main];testsrc=size=640x360 [ref];[main][ref]
scale2ref=500/6:print(print(ow)*5) [main][ref];[ref] nullsink" -map
"[main]" -flags +bitexact -fflags +bitexact -f md5 -
That will print() 83.333333 and then 416.666667. A user might
(reasonably, in my opinion) expect that the ow value (or oh) is always
an integer. With the extra cast you'll see 83.000000 and 415.000000
printed. 83.333333 truncates to 83 so no (noticeable) change for ow
but 416.666667 does not truncate to 415 so this is an example of a
place where the lack of truncation for ow/oh does change the outcome.
I hope this clears it up. Perhaps that code should just be entirely
refactored to be a little more clear?
Thanks,
Kevin
On Tue, Jun 13, 2017 at 12:10:07AM -0400, Kevin Mark wrote: > On Mon, Jun 12, 2017 at 9:42 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > why is there a cast at all ? > > The cast is there because if you run this: > > ffmpeg -frames:v 5 -filter_complex > "sws_flags=+accurate_rnd+bitexact;testsrc=size=320x240 > [main];testsrc=size=640x360 [ref];[main][ref] > scale2ref=0:print(ow/641) [main][ref];[ref] nullsink" -map "[main]" > -flags +bitexact -fflags +bitexact -f md5 - > > This works just fine without a cast for ow. 0 == 0 is true so we set > it to 640. But for oh, the print() shows that ow/641 is 0.998440. When > it is truncated from a double to an integer (eval_h = res) it becomes > 0. But in our comparison, 0.998440 == 0 is false so in this case > eval_h will be truncated to 0 which is exactly the behavior we're > trying to correct. Adding that cast resolves the issue. 0.998440 == 0 > is false but (int) 0.998440 == 0 is true. > > For the extra cast I was talking about consider this: > > ffmpeg -frames:v 5 -filter_complex > "sws_flags=+accurate_rnd+bitexact;testsrc=size=320x240 > [main];testsrc=size=640x360 [ref];[main][ref] > scale2ref=500/6:print(print(ow)*5) [main][ref];[ref] nullsink" -map > "[main]" -flags +bitexact -fflags +bitexact -f md5 - > > That will print() 83.333333 and then 416.666667. A user might > (reasonably, in my opinion) expect that the ow value (or oh) is always > an integer. With the extra cast you'll see 83.000000 and 415.000000 > printed. 83.333333 truncates to 83 so no (noticeable) change for ow > but 416.666667 does not truncate to 415 so this is an example of a > place where the lack of truncation for ow/oh does change the outcome. ok, makes sense i agree the 2nd cast seems a good idea > > I hope this clears it up. Perhaps that code should just be entirely > refactored to be a little more clear? > > Thanks, > Kevin > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Tue, Jun 13, 2017 at 10:04 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > ok, makes sense > i agree the 2nd cast seems a good idea Great, thanks Michael. I'll submit an updated patch with the additional cast.
diff --git a/libavfilter/scale.c b/libavfilter/scale.c index 03745ddcb8..a6c32e3978 100644 --- a/libavfilter/scale.c +++ b/libavfilter/scale.c @@ -158,19 +158,19 @@ int ff_scale_eval_dimensions(void *log_ctx, av_expr_parse_and_eval(&res, (expr = w_expr), names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx); - eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = res; + eval_w = var_values[VAR_OUT_W] = var_values[VAR_OW] = (int) res == 0 ? inlink->w : res; if ((ret = av_expr_parse_and_eval(&res, (expr = h_expr), names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0) goto fail; - eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = res; + eval_h = var_values[VAR_OUT_H] = var_values[VAR_OH] = (int) res == 0 ? inlink->h : res; /* evaluate again the width, as it may depend on the output height */ if ((ret = av_expr_parse_and_eval(&res, (expr = w_expr), names, var_values, NULL, NULL, NULL, NULL, NULL, 0, log_ctx)) < 0) goto fail; - eval_w = res; + eval_w = (int) res == 0 ? inlink->w : res; w = eval_w; h = eval_h; @@ -186,13 +186,10 @@ int ff_scale_eval_dimensions(void *log_ctx, factor_h = -h; } - if (w < 0 && h < 0) - eval_w = eval_h = 0; - - if (!(w = eval_w)) + if (w < 0 && h < 0) { w = inlink->w; - if (!(h = eval_h)) h = inlink->h; + } /* Make sure that the result is divisible by the factor we determined * earlier. If no factor was set, it is nothing will happen as the default
The input width and height is known at parse time so there's no reason ow/oh should not be usable when using 0 as the width or height expression. Previously in "scale=0:ow" ow would be set to "0" which works, conveniently, as "scale=0:0" is perfectly valid input but this breaks down when you do something like "scale=0:ow/4" which one could reasonably expect to work as well, but does not as ow is 0 not the real value. This change handles the 0 case for w/h immediately so the ow/oh variables work as expected. Consequently, the rest of the code does not need to handle 0 input. w/h will always be > 0 or < 0. Signed-off-by: Kevin Mark <kmark937@gmail.com> --- libavfilter/scale.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)