[FFmpeg-devel] FFmpeg 3.3

[wm4]

On Tue, 14 Mar 2017 17:59:38 +0100
Clément Bœsch <u at pkh.me> wrote:

> On Tue, Mar 14, 2017 at 05:33:41PM +0100, wm4 wrote:
> > On Tue, 14 Mar 2017 17:02:34 +0100
> > Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >   
> > > On Tue, Mar 14, 2017 at 4:49 PM, wm4 <nfxjfg at googlemail.com> wrote:  
> > > > On Tue, 14 Mar 2017 14:19:24 +0000
> > > > Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
> > > >    
> > > >> On 14 March 2017 at 13:38, wm4 <nfxjfg at googlemail.com> wrote:
> > > >>    
> > > >> > On Tue, 14 Mar 2017 10:24:10 -0300
> > > >> > James Almer <jamrial at gmail.com> wrote:
> > > >> >    
> > > >> > > On 3/14/2017 9:31 AM, Rostislav Pehlivanov wrote:    
> > > >> > > > On 14 March 2017 at 11:29, Michael Niedermayer <michael at niedermayer.cc    
> > > >> > >    
> > > >> > > > wrote:
> > > >> > > >    
> > > >> > > >>
> > > >> > > >> What about the spherical API size_t / uint32_t issue ?
> > > >> > > >> we can change it before the release but not after it
> > > >> > > >>
> > > >> > > >>    
> > > >> > > > IMO its better to have the same type on all platforms. It also doesn't    
> > > >> > make    
> > > >> > > > sense to use size_t for something describing a position.    
> > > >> > >
> > > >> > > wm4 argued it would generate problems for being different than libav's.
> > > >> > > We don't care about ABI compatibility anymore so struct field size or
> > > >> > > position isn't a problem.
> > > >> > >
> > > >> > > What kind of trouble would having the function signatures use uint32_t
> > > >> > > here and size_t on libav generate? It's technically breaking API
> > > >> > > compatibility i guess.    
> > > >> >
> > > >> > I'm mostly thinking about things like overflow checks. And of course,
> > > >> > you know, the damn user API.
> > > >> >
> > > >> >    
> > > >> Since the only way to currently get the (float) value of the position on
> > > >> any platform is to measure its size, convert it to bits and calculate the
> > > >> limit and divide it, changing the type to a static wouldn't cause any
> > > >> problems for someone already doing this and will keep compatibility with
> > > >> libav. Someone who assumes size_t is always going to be 64 or 32 bits will
> > > >> be disappointed when their code doesn't work on MIPS/32 bit stuff but its
> > > >> their fault for incorrectly assuming that and its our fault for letting
> > > >> this patch in with size_t in the first place, so we ought to fix it.    
> > > >
> > > > Feel free to send a patch to Libav. Hopefully I won't be the one who
> > > > allows subtle and pointless API divergence over silly reasons.    
> > > 
> > > Using "API divergence" as an excuse to accept silly decision is
> > > equally silly, if not more so.  
> > 
> > Well, you're not the one who will have to deal with an increasing
> > number of different basic types on the same struct fields.
> > 
> > If we do this now, you can bet we'll change AVFrame.crop* from size_t
> > to something else too. And then if Libav changes AVFrame.width/height
> > to size_t too (like they apparently plan to), it will be even worse.
> > You probably don't care about this, because you won't have to deal with
> > the end result, but I'm not only going to have to use this subtle
> > different API, but I'll also probably contribute patches to both
> > projects, and then I'll have to change my patches to use the "correct"
> > type on each side. And what about potential project reunification?
> > Another idiotic thing we'll have to flame about?
> >   
> 
> We are diverging more and more API wise. Feel free to correct me, but I
> thought importing or not a change from Libav was dependent on two main
> factors:
> 
> - how much effort it will cause in a following merge?
> - is it technically a good change?
> 
> Beside the merge commits, with time we also diverged a lot in API, and not
> only in API additions, but also on internal breaking changes (typically in
> lavfi but not limited to).

These are only due to lack of communication.

> We know it's a problem for users who want to support both, but on the
> other hand, we can't always be shaped by Libav decisions, there is a need
> for a technical evaluations of the different solutions.

I doubt that.

> Now back to the current issue.
> 
> Do you agree with the fact that uint32_t is a better technical choice?
> (I think the main argument is that it's a pain to have proper FATE
> coverage if you don't?)

We've already established that the current way FATE is doing it is not
a good idea. It relies on C ABI coincidences. It's equivalent to
parsing file formats by using read() calls with pointers to structs.
For example, big endian fate instances should be failing on side data
represented by structs (but apparently we don't test side data contents
anyway, only their size, which makes this whole side data discussion
even more ridiculous).

> If so, I'm assuming you believe that even if it's better, it's not worth
> the breaking change for users. I do not share the feeling at all if it
> reduces FATE coverage. If you share that sentiment, maybe it's worth
> fighting for that technical argument where you want it to change. Why
> would FFmpeg follow Libav even if you disagree with Libav's argument?
> 
> If not, then I think it's better to defend your technical disagreement
> instead of using a political one. And maybe that size_t BS is going to
> have acceptance here in FFmpeg and the problem will be solved that way.
>