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

Vittorio Giovara vittorio.giovara at gmail.com
Tue Mar 14 23:43:04 EET 2017


On Tue, Mar 14, 2017 at 5:27 PM, James Almer <jamrial at gmail.com> wrote:
> On 3/14/2017 6:16 PM, Vittorio Giovara wrote:
>> 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.
>
> But will you try to apply this patch on libav's side as well? wm4's
> concern is the divergence this will create between the two projects.

Yes that is a valid concern, I'll forward this patch to libav-devel
and move the discussion there.
-- 
Vittorio


More information about the ffmpeg-devel mailing list