[FFmpeg-devel] [RFC] avpriv cleanup

[wm4]

On Sun, 25 Mar 2018 23:46:25 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Sun, Mar 25, 2018 at 05:55:37PM +0200, wm4 wrote:
> > On Sun, 25 Mar 2018 17:34:51 +0200
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Sun, Mar 25, 2018 at 05:07:33PM +0200, wm4 wrote:  
> > > > On Sun, 25 Mar 2018 17:00:32 +0200
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > Hi all
> > > > > 
> > > > > Nicolas wrote an interresting comment about avpriv, and as this better
> > > > > belongs into a new thread, here it is in a new thread
> > > > > 
> > > > > On Sun, Mar 25, 2018 at 03:32:33PM +0200, Nicolas George wrote:    
> > > > > > Josh de Kock (2018-03-23):      
> > > > > [...]
> > > > >          
> > > > > > You can observe just that by the fact that you needed to add an avpriv
> > > > > > function to let lavd communicate with lavf. Each time an avpriv function
> > > > > > appears, it shows there is something flawed in the design.      
> > > > > 
> > > > > If this is true, in general, then we can improve designs by removing
> > > > > avpriv_* functions ...
> > > > > 
> > > > > looking at what we have as avprivs, in the tree, i think some of these
> > > > > could be indeed usefull as public APIs. And we need to already keep them
> > > > > compatible as they are exported as avpriv too, so making such changes does
> > > > > indeed in some cases look like a good idea to me
> > > > > 
> > > > > There are some avprivs we have currently though that are quite specific
> > > > > to format intgernals, i dont think that its always a flaw to use avpriv
> > > > > functions thus
> > > > > 
> > > > > but i think we should evaluate each of the currently existing avpriv
> > > > > functions and check if the API wouldnt be usefull to user applications.
> > > > > And if it wouldnt be better to make it properly public    
> > > >     
> > >   
> > > > This might apply to some cases, but in general: just no.    
> > > 
> > > That is what i meant :)
> > > 
> > >   
> > > > 
> > > > We make things avpriv_ *because* we don't want them to be public API,
> > > > but the odd multiple-library design forces us to. Not more, not less.
> > > >     
> > >   
> > > > What is this thread even for? If you want to make certain functions
> > > > public, post specific RFCs or patches. In general, we probably all
> > > > agree that avpriv functions are necessary hacks to deal with the split
> > > > library nonsense.    
> > > 
> > > The thread is for finding out peoples oppinon before anyone spends time
> > > writing patches.
> > > For example it takes time to go over the functions and time to write patches
> > > that turn functions with proper documention into public functions.
> > > 
> > > If people object to it, no point in anyone doing any of this work
> > > 
> > > Also there have been people looking for things to work on. If we agree
> > > that looking over avpriv functions for more generally usefull ones 
> > > makes sense then we need to discuss this in some thread for anyone
> > > to be able to see that suggestion.  
> > 
> > Well, just generally suggesting to remove avpriv functions is not going
> > to work, because they exist for a reason (see e.g. Nicolas George or my
> > post). Every specific case likely needs a separate discussion, so it
> > doesn't make sense to have some sort of summary discussion.  
> 
> well, the summary discussion certainly is very effective to get a feeling
> for peoples oppinion on this in general.
> 
> 
> >   
> > > For a specific function
> > > As a random example, lets take avpriv_set_systematic_pal2()
> > > It provides a palette for 8bit per pixel formats which have a constant
> > > palette. It sounds usefull to have this public to me ...  
> > 
> > For that specific example I'd rather argue we should get rid of
> > AV_PIX_FMT_FLAG_PSEUDOPAL. It's an awful hack to allow code to treat 1
> > byte formats as paletted, such as AV_PIXFMT_GRAY8. Basically AVFrames
> > with this pixfmt must come with a palette that acts as lookup table,
> > that treats the 1 byte value as index into the table to convert it to
> > RGB. The avpriv_set_systematic_pal2() just sets the palette for those
> > formats.
> > 
> > This is broken and bad for the following reasons:
> >   
> 
> > 1. The affected pixfmts are NOT paletted. So they shouldn't have a
> >    palette. It's against the principle of least astonishment (basically
> >    surprising the user with some broken obscure bullshit, instead of
> >    things just working). The only reason why I know about this pseudi
> >    PAL feature is because once I got a segfault by passing a self
> >    allocated GRAY8 frame to libswscale.  
> 
> avpriv_set_systematic_pal2() sets the palette you where missing, that
> caused the issue.
> how is that an argument to not make it public !?

I already described how the function you want to make public is
inherently broken.

The whole mechanism is a broken idea though and should be removed.

> 
> > 2. I suspect it's a hack only used by libswscale to speed up some
> >    trivial conversions. But libswscale can just create and keep such
> >    lookup table internally. Not a problem. This mechanism is not needed
> >    in the public API.  
> 
> 
> > 3. Thus why clutter public API and burden the API user with this?  
> 
> you already provided the arguments for this above.
> A. You tried to allocate a AVFrame manually, you need to setup things
>    correctly. And iam not sure the API actually allows users to allocate
>    AVFrames manually this way.
>    its not for swscale alone

Like where?

> B. It allows using palette specifc code for more cases, simplifying code.
>    That can be code in a user application, the function is usefull for
>    this without any AVFrames

Pretty crap idea. Why can't I treat 10 bit formats as paletted? The LUT
would be small enough to make this useful. On the other hand, nobody
stops you from doing this internally.

What makes you think the LUT has to be part of the AVFrame?

> 
> 
> > 4. It's actually broken. The actual RGB mapping depends on the color
> >    levels for GRAY8 (because GRAY8 is essentially a Y plane). The
> >    lookup table is valid only for full range GRAY8, for other values
> >    it's broken. The API function you suggest to make public is broken
> >    in this way too, because it takes only the pixfmt, not levels or
> >    other colorimetry info.  
> 
> Thats a rather obscure corner case, and making this function public
> would be a good opertuniy to look into fixing that

You see: if it had been public (like you argued it should be) it'd be
much harder to fix.

> 
> > 5. Even if the things before gets fixed, nobody is going to call the
> >    function after updating color levels on the AVFrame. Thus it's
> >    fragile and error prone.  
> 
> This and the previous arguments are only about gray8. 
> The obvious fix is not to change the API but to remove
> gray8 from the cases the function is used for.
> 
> 
> > 
> > Do you still want to make that function public?  
> 
> yes, even if PSEUDOPAL is removed this function is usefull to simplify
> code.
> 
> 
> [...]
> 
> thx
> 
> [...]