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

[James Almer <jamrial@gmail.com>]

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.

> 
> 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".