[FFmpeg-devel] [PATCH 0/2] first steps to resolving float to int undefined behavior

[Ronald S. Bultje]

Hi,

On Sun, Nov 1, 2015 at 7:21 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:

> On Sun, Nov 1, 2015 at 7:15 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> > On Sun, Nov 01, 2015 at 06:51:19PM -0500, Ganesh Ajjanagadde wrote:
> >> On Sun, Nov 1, 2015 at 6:25 PM, Nicolas George <george at nsup.org> wrote:
> >> > Le primidi 11 brumaire, an CCXXIV, Ronald S. Bultje a écrit :
> >> >> So, is this a bug in llrint, or is this a failure to use llrint, or
> is this
> >> >> different from llrint? It sounds to me that llrint should be used,
> not our
> >> >> own alternative.
> >> >
> >> > Is there a sized version of the function? int64rint? Otherwise, these
> >> > functions for the native types are as useless as the native types
> >> > themselves.
> >>
> >> No, not in ISO C, or even libc AFAIK. ISO C tends to favor the
> >> "non-sized" types in their signatures. The reason (I believe) stems
> >> from the fact that an implementation is free to not even define the
> >> sized types:
> >> 7.20.1.1, point 3 - "These types are optional. However, if an
> >> implementation provides integer types with widths of 8, 16, 32, or 64
> >> bits, no padding bits, and (for the signed types) that have a two’s
> >> complement representation [all platforms supported by the project], it
> >> shall define the corresponding typedef names." -
> >> Thus they want to limit their scope to mostly (or perharps only?)
> stdint.h.
> >>
> >> Even otherwise, these functions are somewhat useless due to the
> >> undefined behavior outside the range. All they really do is get
> >> rounding done correctly according to the current FPU environment and
> >> associated rounding modes, which can be manipulated in C99/C11.
> >
> > quite some of the undefined behavior also makes more optimizations
> > possible for advanced compilers
> > random silly example
> >
> > min = 0;
> > for(i=0; i<N; i++) {
> >     float c = whatever()
> >     min = fmin(min, c);
> >     out[i] = llrint(c);
> > }
> >
> > here the compiler can remove any and all handling of NaN and +-Inf
> > from fmin() because llrint(c) implies c is within the range
> > represetable of the integer types
> >
> > with a llrint() equivalent that is defined for all cases this is not
> > possible anymore
>
> Yes, which is why I have mentioned that we need to use a safe version
> only when we need to guarantee safety, and are dealing with arbitrary
> doubles. But such cases are there, such as the ffserver_config patch I
> posted.


OK, so let me sum up my current thoughts:
- I don't really care about ffserver, so I have no opinion on whether the
patch is appropriate. It may well be. Let's assume it should.
- I think the name of this function is confusing. The av_clip* namespace
seems reserved for int to int clips, so using it for a float to int
conversion with clip is kind of weird. I would use a different namespace
for it.
- I'm not sure the code should live in this header. av_clip* is internal
API (afaiu), and ffserver should not use internal api. I know it does, but
it shouldn't, so adding new api specifically so ffserver can use it is ...
all upside-down. Maybe the code should (for now) live in ffserver, or in an
installed header, so it's ok for apps (like ffserver) to use it, if that's
the intention.

So, finally, maybe I'm being pedantic now, but it seems one of these usages
overflows if we go over 2^64 = 16000 petabytes. Is that right? Do we really
care? I mean, we're talking ffserver here, ffserver has much bigger issues
than this.

Ronald