[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

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