[FFmpeg-devel] FFmpeg 3.3

James Almer jamrial at gmail.com
Tue Mar 14 19:36:46 EET 2017

On 3/14/2017 2:09 PM, wm4 wrote:
> On Tue, 14 Mar 2017 13:50:53 -0300
> James Almer <jamrial at gmail.com> wrote:
>> On 3/14/2017 1:33 PM, 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?
>>> Can't you see what a giant heap of bullshit this is?  
>> I can see your point for other fields in other structs but in here
>> size_t is simply wrong. It should have never been committed like
>> this to begin with.
> Why is it "simply wrong"?

The API expects values of exactly 32 bits to be stored in these fields,
and afaik, size_t doesn't guarantee that many bits on every platform we
supposedly support.

Besides, fixed size types exist precisely for fields with this kind of
requirements and using anything else is just silly.
yaw/pitch/roll store 16.16 fixed point values and those are fixed size,
so using size_t for these three is inconsistent with how this API was
first devised.

>> We could try to get libav to change it as well, but if they don't
>> then at the very least this one (very obscure) struct will have to
>> diverge between projects.
> What about AVFrame.crop_top etc.?

If the assumption was that those were also guaranteed to be at least 32
bits then i guess they are also technically wrong.

More information about the ffmpeg-devel mailing list