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

[Michael Niedermayer]

On Fri, May 20, 2022 at 08:53:35AM -0300, James Almer wrote:
> 
> 
> On 5/20/2022 7:28 AM, Michael Niedermayer 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
> > 
> > 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
> 
> Not making it public allows us to not commit to a fixed design *now*.
> Unless there's a clear need for this to be part of the public API, i don't
> think it's a good idea to do so. This change is being made because this API
> is afaict needed in lavc, and not by some project using lavu.
> 

> Removing/changing an avpriv symbol or struct is a matter or waiting for the
> nearest major bump. No need for an arbitrary 2 year wait period, compat
> wrappers, or anything crazy.

yes but is that really easier ?

sheduling changes to happen during a bump vs adding a warper+deprecating
at any time with a automatic #if removing code

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- 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/fde670d9/attachment.sig>