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

Vittorio Giovara vittorio.giovara at gmail.com
Tue Mar 14 23:16:21 EET 2017


On Tue, Mar 14, 2017 at 4:41 PM, James Almer <jamrial at gmail.com> wrote:
> 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.

If that change goes in, I withdraw any objection on this patch.
-- 
Vittorio


More information about the ffmpeg-devel mailing list