[FFmpeg-devel] [PATCH] Change type of spherical stuff to uint32_t

James Almer jamrial at gmail.com
Tue Mar 14 22:41:49 EET 2017


On 3/14/2017 4:46 PM, Vittorio Giovara wrote:
> On Tue, Mar 14, 2017 at 12:12 PM, James Almer <jamrial at gmail.com> wrote:
>> On 3/14/2017 12:37 PM, Vittorio Giovara wrote:
>>> On Sun, Mar 12, 2017 at 11:21 PM, Michael Niedermayer
>>> <michael at niedermayer.cc> wrote:
>>>> On Sun, Mar 12, 2017 at 08:54:07PM -0400, Ronald S. Bultje wrote:
>>>>> Hi,
>>>>>
>>>>> On Sun, Mar 12, 2017 at 7:32 PM, James Almer <jamrial at gmail.com> wrote:
>>>>>
>>>>>> On 3/12/2017 6:16 PM, Michael Niedermayer wrote:
>>>>>>> Using the same type across platforms is more robust and avoids platform
>>>>>>> specific issues from differences in range.
>>>
>>> I still think you are curing the symptom rather than the illness.
>>>
>>> Besides, you can't just change types on a whim, you should wait for
>>> the major bump (if at all).
>>
>> As mentioned by Hendrik, it's only six days old and not part of any
>> release, so it can be changed just fine.
> 
> Not really, but I'm not here to discuss policies.

Maybe not in libav, but it is here. Being stuck with a very recent bad
change for no reason is not a sane choice. If it was a month old then
we're talking.
Releases are made precisely to freeze the API/ABI for distros and the
reason why this change either goes in now or doesn't go in at all.

> 
>>>>>>> Also fixed point integers are on a semantical level not size_t
>>>
>>> This is only theoretical,
>>
>> You specifically wrote the API to have the fields store 0.32 fixed point
>> values. Why you choose size_t for a field that's meant to store exactly
>> 32 bits is the question.
> 
> I choose size_t because it represented a `size` as the name implies,
> and since it is very closely related to "cropping rectangle" I picked
> the types of the fields that were in use in AVFrame:
> https://git.libav.org/?p=libav.git;a=blob;f=libavutil/frame.h;h=f9ffb5bbbf852de4728442a3940c6f2ddb752ecd;hb=HEAD#l385

The size of an object in memory, not anything related to the dimensions
of a video. Is the latter supposed to be system dependent now?

And unlike those AVFrames fields which are pixel values, these fields
are *explicitly* meant to store exactly 32 bit long, 0.32 fixed point
values. Anything other than uint32_t or int32_t, types defined in the
standard for this exact scenario, makes no sense.

> 
>> Afaik, size_t could even be 16 bit in some systems. FreeDOS is probably
>> one, and we supposedly support it. Libav does too, so maybe this change
>> should be done in both projects.
> 
> I'm pretty sure both projects will fail to build on 16 bits platform.
> Unless you find a very very smart compiler that will ignore all large
> constants not fitting into int, all out of bounds shifts, tables not
> fitting into even 1MB and such. Then it will probably crash somewhere
> anyway. Someone mentioned me that this would be similar to a certain
> MPEG reference decoder from Fraunhofer that does not compile right in
> 64-bit mode and does not run by default because some of its functions
> require >10MB of stack.

wbs on irc confirmed those targets are 32 bits as far as we support them,
so apparently not a problem for us after all.

> 
>>> and, since we're talking about semantics,
>>> you're breaking ABI by using sizeof(AVSphericalMapping) outside of
>>> libavutil.
>>
>> Well, you're the one that introduced the only current sizeof() check
>> outside of libavutil, in both projects.
> 
> Are you sure?

I mean the one related to AVSphericalMapping. So yes, you introduced
that ABI breaking check at the same time you introduced the relevant
struct.

The rest are of course someone else's fault.

> There are several invalid sizeof() uses of things that
> shouldn't be size-able -- this one is noticeable only because the test
> is printing different things. Since you're mentioning me directly,
> please remember that I also proposed the right fix for this bug, that
> is to remove printing the size of structures from ffprobe.

That change will probably go in as well.



More information about the ffmpeg-devel mailing list