Re: [FFmpeg-devel] [PATCH v4] avutil/csp: create avpriv API for colorspace structs

[Niklas Haas <ffmpeg@haasn.xyz>]

On Fri, 20 May 2022 12:28:15 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Wed, May 18, 2022 at 08:27:48PM +0200, Michael Niedermayer wrote:
> > On Wed, May 18, 2022 at 08:23:38PM +0200, Michael Niedermayer wrote:
> > > On Wed, May 18, 2022 at 11:18:17AM -0400, Leo Izen wrote:
> > > > This commit moves some of the functionality from avfilter/colorspace
> > > > into avutil/csp and exposes it as an avpriv API so it can be used by
> > > > libavcodec and/or libavformat.
> > > [...]
> > > > +#ifndef AVUTIL_CSP_H
> > > > +#define AVUTIL_CSP_H
> > > > +
> > > > +#include "libavutil/pixfmt.h"
> > > > +
> > > > +typedef struct AVLumaCoefficients {
> > > > +    double cr, cg, cb;
> > > > +} AVLumaCoefficients;
> > > > +
> > > > +typedef struct AVPrimaryCoefficients {
> > > > +    double xr, yr, xg, yg, xb, yb;
> > > > +} AVPrimaryCoefficients;
> > > > +
> > > > +typedef struct AVWhitepointCoefficients {
> > > > +    double xw, yw;
> > > > +} AVWhitepointCoefficients;
> > > 
> > > As said, these should not be floating point.
> > > Adding a new public API and changing it later is messy, this
> > > should be changed before its made public
> > 
> > i now see you replaced some public by avpriv in the latest patch
> > but still i think this should be changed to fixed point or AVRational
> > first. Even as API between the libs its messy to change it later
> > it would require us to keep the double API when its changed until
> > the next major bump
> 
> I see some discussion related to this on the IRC log from when i was
> sleeping. Maybe it would be better to keep this on the mailing list
> 
> Also iam not sure my concern was clearly worded so ill sort my argument
> and concerns so its clearer below:
> 
> 1. exactly representing values
>     if you have a 0.1 you can represent that exactly as AVRational 1/10 but
>     maybe shockingly a double cannot.
>         
>     Try a printf %f of 0.1 and it will do 0.100000 looks good but thats deception
>     try that with more precission %100.99f shows this:
>     0.100000000000000005551115123125782702118158340454101562500000000000000000000000000000000000000000000
>     
>     so if we want to use exactly the values from the spec, doubles with a 
>     unit/base of 1 do not work.
>     int or even doubles with a base/unit of 30000 might work exactly if
>     AVRational is unpopular. 30000 instead of 10000 is for that one pesky 1/3
>     
> 1b. the exact value that 0.1 has in float/double depends on the precission
>     IEEE float
>     0.100000001490116119384765625000000000000000000000000000000000000000000000000000000000000000000000000
>     IEEE double
>     0.100000000000000005551115123125782702118158340454101562500000000000000000000000000000000000000000000
>     long double
>     0.100000000000000000001355252715606880542509316001087427139282226562500000000000000000000000000000000
>     So there will be slight differences if (intermediate) types anywhere arent
>     exactly the same

It's worth pointing out that these values are already, in many cases,
arbitrarily rounded. D65, for example, has many different definitions:

- ISO/CIE provide a version rounded to 6 decimal digits but also
  provides the full tabulated spectral curves (sampled at 10nm
  intervals) from which you can derive a higher precision version
- ITU-R always rounds to 4 digits, but newer ITU-R standards (e.g.
  BT.2100) reference the aforementioned ISO document for the definition,
  making it technically ambiguous.
- Older EBU documents even round to 3 digits
- There exist other sources that round to 5 digits also

In my mind, this weakens somewhat the case of making sure these values
have exact representations, if the underlying physical constant being
represented does not have an exact decimal representation to begin with.

> 
> 2. someone said, you need to pick a denominator when doing float -> rational
>     av_d2q() will pick the best denominator for you.
> 
> 3. avpriv_ vs av_
>     avpriv is evil, it combines the pain of ABI/API compatibility while the 
>     public cant use it
> 
> 4. rounding, regressions and inexactness
>     doubles/floats have in the past broken regression tests. They do not
>     always but i suggest we avoid them when theres no clear advantage
>     like higher speed in speed relevant code or much simpler code
>     
> thx
>     
> 
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> The day soldiers stop bringing you their problems is the day you have stopped 
> leading them. They have either lost confidence that you can help or concluded 
> you do not care. Either case is a failure of leadership. - Colin Powell
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".