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

Vittorio Giovara vittorio.giovara at gmail.com
Tue Mar 14 23:58:06 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.
> Strictly speaking, changing the struct fields to uint32_t would be
> enough. Leaving the av_spherical_tile_bounds() signature as is
> wouldn't be an issue since it doesn't take any of these fields as
> input arguments. It handles them internally and just outputs pixel
> values which can stay as size_t, like those AVFrame fields.

Yeah I was thinking about that too.
I'll propose an alternative patch without that, and see what happens.

More information about the ffmpeg-devel mailing list