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

[Niklas Haas <ffmpeg@haasn.xyz>]

On Fri, 20 May 2022 08:53:35 -0300 James Almer <jamrial@gmail.com> 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.

I see it being useful outside FFmpeg.

> 
> 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.
> 
> > 
> > 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
> >      
> > 
> > [...]
> > 
> > 
> > _______________________________________________
> > 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".
_______________________________________________
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".