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

[Michael Niedermayer]

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220520/2e65e653/attachment.sig>