[FFmpeg-devel,4/7] Adds gray floating-point pixel formats.

2018-08-18 23:20 GMT+03:00 Michael Niedermayer <michael@niedermayer.cc>:

> On Sat, Aug 18, 2018 at 02:10:21PM +0300, Sergey Lavrushkin wrote:
> > 2018-08-17 23:28 GMT+03:00 Michael Niedermayer <michael@niedermayer.cc>:
> >
> > > On Fri, Aug 17, 2018 at 12:46:52AM -0300, James Almer wrote:
> > > > On 8/14/2018 1:23 PM, Michael Niedermayer wrote:
> > > > > On Mon, Aug 13, 2018 at 04:58:42PM +0300, Sergey Lavrushkin wrote:
> > > > >>>
> > > > >>> Just use av_clipf instead of FFMIN/FFMAX.
> > > > >>
> > > > >>
> > > > >> Changed FFMIN/FFMAX to av_clip_uint16 and av_clip_uint8.
> > > > >
> > > > > will apply
> > > > >
> > > > > thanks
> > > >
> > > > This broke fate on some 32bit hosts. Guess float pixfmts shouldn't be
> > > > tested for bitexact output. The gbrpf32 ones aren't, for example.
> > > > http://fate.ffmpeg.org/report.cgi?time=20180816131312&slot=
> > > x86_32-debian-kfreebsd-gcc-4.4-cpuflags-mmx
> > >
> > > hmmmm
> > > i remember i had tested this locally on 32bit
> > > can something be slightly adjusted (like an offset or factor) to avoid
> any
> > > values becoming close to 0.5 and rounding differently on platforms ?
> >
> > If not the tests should skip float pixel formats or try the nearest
> > > neighbor scaler
> > >
> >
> > Can it really be the problem with scaler? Do all these failed test use
> > scaling?
> > Is not it the problem, that different platforms can give slightly
> different
> > results for
> > floating-point operations? Does input for the float format is somehow
> > generated
> > for these tests, so the input conversion is tested? Maybe it uses output
> > conversion first?
> > If it is the problem of different floating-point operations results on
> > different platforms,
>
> > maybe it is possible to use precomputed LUT for output conversion, so it
>
> I dont think we should change the "algorithm" to achive "bitexactness"
> we could of course but it feels like the wrong reason to make such a
> change. How its done should be choosen based on what is fast (and to a
> lesser extend clean, simple and maintainable)
>
>
>
> > will give
> > the same results? Or is it possible to modify tests for the float format,
> > so it will
> > check if pixels of the result are just close to some reference.
>
> Its possible to compare to a reference, we do this in some other tests,
> but thats surely more work than just disabling teh specific tests or trying
> to nudge them a little to see if that makes nothing fall too close to n +
> 0.5
>
> >
> >
> > > Sergey, can you look into this (its your patch) ? (just asking to make
> sure
> > > not eevryone thinks someone else will work on this)
> > >
> >
> > Yes, I can, just need to know, what is possible to do to fix this issue,
> > besides skipping the tests.
>
> most things are possible
>

Hi,

I am having trouble reproducing this error. These tests are fine for 32-bit
VMs on
my computers. So the only thing I can do is to disable these tests for
these formats.
Otherwise, I need to test other changes somehow. Here is the patch, that
skips
pixfmts tests for these formats.

Hello,

I am having trouble reproducing this error. These tests are fine for 32-bit
> VMs on
> my computers. So the only thing I can do is to disable these tests for
> these formats.
> Otherwise, I need to test other changes somehow. Here is the patch, that
> skips
> pixfmts tests for these formats.
>
>
What is the conversion function which create "problem" in tests ?

Martin

On Mon, Aug 20, 2018 at 11:27:05PM +0300, Sergey Lavrushkin wrote:
> 2018-08-18 23:20 GMT+03:00 Michael Niedermayer <michael@niedermayer.cc>:
> 
> > On Sat, Aug 18, 2018 at 02:10:21PM +0300, Sergey Lavrushkin wrote:
> > > 2018-08-17 23:28 GMT+03:00 Michael Niedermayer <michael@niedermayer.cc>:
> > >
> > > > On Fri, Aug 17, 2018 at 12:46:52AM -0300, James Almer wrote:
> > > > > On 8/14/2018 1:23 PM, Michael Niedermayer wrote:
> > > > > > On Mon, Aug 13, 2018 at 04:58:42PM +0300, Sergey Lavrushkin wrote:
> > > > > >>>
> > > > > >>> Just use av_clipf instead of FFMIN/FFMAX.
> > > > > >>
> > > > > >>
> > > > > >> Changed FFMIN/FFMAX to av_clip_uint16 and av_clip_uint8.
> > > > > >
> > > > > > will apply
> > > > > >
> > > > > > thanks
> > > > >
> > > > > This broke fate on some 32bit hosts. Guess float pixfmts shouldn't be
> > > > > tested for bitexact output. The gbrpf32 ones aren't, for example.
> > > > > http://fate.ffmpeg.org/report.cgi?time=20180816131312&slot=
> > > > x86_32-debian-kfreebsd-gcc-4.4-cpuflags-mmx
> > > >
> > > > hmmmm
> > > > i remember i had tested this locally on 32bit
> > > > can something be slightly adjusted (like an offset or factor) to avoid
> > any
> > > > values becoming close to 0.5 and rounding differently on platforms ?
> > >
> > > If not the tests should skip float pixel formats or try the nearest
> > > > neighbor scaler
> > > >
> > >
> > > Can it really be the problem with scaler? Do all these failed test use
> > > scaling?
> > > Is not it the problem, that different platforms can give slightly
> > different
> > > results for
> > > floating-point operations? Does input for the float format is somehow
> > > generated
> > > for these tests, so the input conversion is tested? Maybe it uses output
> > > conversion first?
> > > If it is the problem of different floating-point operations results on
> > > different platforms,
> >
> > > maybe it is possible to use precomputed LUT for output conversion, so it
> >
> > I dont think we should change the "algorithm" to achive "bitexactness"
> > we could of course but it feels like the wrong reason to make such a
> > change. How its done should be choosen based on what is fast (and to a
> > lesser extend clean, simple and maintainable)
> >
> >
> >
> > > will give
> > > the same results? Or is it possible to modify tests for the float format,
> > > so it will
> > > check if pixels of the result are just close to some reference.
> >
> > Its possible to compare to a reference, we do this in some other tests,
> > but thats surely more work than just disabling teh specific tests or trying
> > to nudge them a little to see if that makes nothing fall too close to n +
> > 0.5
> >
> > >
> > >
> > > > Sergey, can you look into this (its your patch) ? (just asking to make
> > sure
> > > > not eevryone thinks someone else will work on this)
> > > >
> > >
> > > Yes, I can, just need to know, what is possible to do to fix this issue,
> > > besides skipping the tests.
> >
> > most things are possible
> >
> 
> Hi,
> 
> I am having trouble reproducing this error. These tests are fine for 32-bit
> VMs on
> my computers. 

yes, i had the same problem when i initially tested the code, it locally works
on 32bit
must be the exact platform or compiler ...


> So the only thing I can do is to disable these tests for
> these formats.
> Otherwise, I need to test other changes somehow. Here is the patch, that
> skips
> pixfmts tests for these formats.

in absence of another solution this should be ok

thx

[...]

>
> > So the only thing I can do is to disable these tests for
> > these formats.
> > Otherwise, I need to test other changes somehow. Here is the patch, that
> > skips
> > pixfmts tests for these formats.
>
> in absence of another solution this should be ok
>
>
I'm not against, removing these tests for now,

But maybe to make tests simpler, we can use/add bit exact conversion
for uint8 to float, we can generate a LUT without float calc
and for uint16 to float, we can add a uint16 to float conversion without
float calc, or maybe, generate a LUT in bit exact mode (probably faster, if
it's acceptable to have a LUT for 16bit entries)

If the bit exact mode is much slower than the "float" version we can keep
both,
in order to use the bit exact for various tests
and use it as reference in order to compare bit exact and "float" conv for
each func.

I can send patch for this, if need.

Martin

Martin Vignali wrote:

>But maybe to make tests simpler, we can use/add bit exact
>conversion for uint8 to float, we can generate a LUT without
>float calc and for uint16 to float, we can add a uint16 to
>float conversion without float calc, or maybe, generate a LUT
>in bit exact mode (probably faster, if it's acceptable to have
>a LUT for 16bit entries)

In my experience, a LUT in bit exact mode is indeed faster. To
me the proposed addition would be useful. And I guess LUTs for
16-bit entries will become necessary anyway at some point...

Thank you! Reto

On Wed, Aug 22, 2018 at 10:20:53AM +0200, Martin Vignali wrote:
> >
> > > So the only thing I can do is to disable these tests for
> > > these formats.
> > > Otherwise, I need to test other changes somehow. Here is the patch, that
> > > skips
> > > pixfmts tests for these formats.
> >
> > in absence of another solution this should be ok
> >
> >
> I'm not against, removing these tests for now,
> 

> But maybe to make tests simpler, we can use/add bit exact conversion
> for uint8 to float, we can generate a LUT without float calc
> and for uint16 to float, we can add a uint16 to float conversion without
> float calc, or maybe, generate a LUT in bit exact mode (probably faster, if
> it's acceptable to have a LUT for 16bit entries)

I think before you design everything around a non float based convertion
it would make sense to evaluate the speed of SIMD for the convertion.
so all the choices are understood


> 
> If the bit exact mode is much slower than the "float" version we can keep
> both,

this is possible, it is not ideal though as the tested and used in practice
code path differs then. Of course a bit exact version does make sense either
way. As some people probably want scaling to be bitexact


> in order to use the bit exact for various tests
> and use it as reference in order to compare bit exact and "float" conv for
> each func.
> 
> I can send patch for this, if need.
> 
> Martin
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> > But maybe to make tests simpler, we can use/add bit exact conversion
> > for uint8 to float, we can generate a LUT without float calc
> > and for uint16 to float, we can add a uint16 to float conversion without
> > float calc, or maybe, generate a LUT in bit exact mode (probably faster,
> if
> > it's acceptable to have a LUT for 16bit entries)
>
> I think before you design everything around a non float based convertion
> it would make sense to evaluate the speed of SIMD for the convertion.
> so all the choices are understood
>
>
> >
> > If the bit exact mode is much slower than the "float" version we can keep
> > both,
>
> this is possible, it is not ideal though as the tested and used in practice
> code path differs then. Of course a bit exact version does make sense
> either
> way. As some people probably want scaling to be bitexact
>
>
Having func too much complicated to test, have more chance to make func not
tested at all
Bit exact mode is convenient for testing inside ffmpeg, but also useful
outside, for example to be sure transcoding pipeline still give the same
output (when i can, i prefer to use this even if the conv is slower,
because is more reliable).

Following your comments, i propose
for a first step to add bit_exact mode for float <-> uint8/16 where i can,
keeping the current func (adding a test for SWS_BIT_EXACT, in order to
choose the right func)

In a second step, test simd for "float" version.
If using SIMD (or not), we can have faster conv, using the "float" method,
we keep each func
otherwise we remove the non bit-exact version, and add a comment in the
code.

If we keep both func, we can make a test func by func, comparing bit exact
with non bit-exact, in order to be sure, both way to make the conversion
have similar result.

Martin