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

Submitted by Michael Niedermayer on March 12, 2017, 9:16 p.m.

Details

Message ID 20170312211646.27123-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer March 12, 2017, 9:16 p.m.
Using the same type across platforms is more robust and avoids platform
specific issues from differences in range.
Also fixed point integers are on a semantical level not size_t

Previous version reviewed-by: James Almer <jamrial@gmail.com>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 ffprobe.c             |  2 +-
 libavformat/dump.c    |  6 +++---
 libavutil/spherical.c |  6 +++---
 libavutil/spherical.h | 16 ++++++++--------
 4 files changed, 15 insertions(+), 15 deletions(-)

Comments

James Almer March 12, 2017, 11:32 p.m.
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.
> Also fixed point integers are on a semantical level not size_t
> 
> Previous version reviewed-by: James Almer <jamrial@gmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  ffprobe.c             |  2 +-
>  libavformat/dump.c    |  6 +++---
>  libavutil/spherical.c |  6 +++---
>  libavutil/spherical.h | 16 ++++++++--------
>  4 files changed, 15 insertions(+), 15 deletions(-)

Needs FATE update.
Ronald S. Bultje March 13, 2017, 12:54 a.m.
Hi,

On Sun, Mar 12, 2017 at 7:32 PM, James Almer <jamrial@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.
> > Also fixed point integers are on a semantical level not size_t
> >
> > Previous version reviewed-by: James Almer <jamrial@gmail.com>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  ffprobe.c             |  2 +-
> >  libavformat/dump.c    |  6 +++---
> >  libavutil/spherical.c |  6 +++---
> >  libavutil/spherical.h | 16 ++++++++--------
> >  4 files changed, 15 insertions(+), 15 deletions(-)
>
> Needs FATE update.


Since this is Vittorio's code, it's probably good to ask him [CC]? From
memory, he seemed to prefer size_t.

Ronald
Michael Niedermayer March 13, 2017, 3:21 a.m.
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@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.
> > > Also fixed point integers are on a semantical level not size_t
> > >
> > > Previous version reviewed-by: James Almer <jamrial@gmail.com>
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  ffprobe.c             |  2 +-
> > >  libavformat/dump.c    |  6 +++---
> > >  libavutil/spherical.c |  6 +++---
> > >  libavutil/spherical.h | 16 ++++++++--------
> > >  4 files changed, 15 insertions(+), 15 deletions(-)
> >
> > Needs FATE update.
> 
> 
> Since this is Vittorio's code, it's probably good to ask him [CC]? From

yes,

semi on topic: but i think people interrested in FFmpeg
development and the direction the code goes should subscribe to
ffmpeg-devel. This CC-ing is not too practical

[...]
wm4 March 13, 2017, 10:45 a.m.
On Sun, 12 Mar 2017 22:16:46 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> Using the same type across platforms is more robust and avoids platform
> specific issues from differences in range.
> Also fixed point integers are on a semantical level not size_t
> 
> Previous version reviewed-by: James Almer <jamrial@gmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---

As I said on the int64_t patch, this is going to cause chaos and should
be avoided.
Nicolas George March 13, 2017, 10:55 a.m.
Le duodi 22 ventôse, an CCXXV, Michael Niedermayer a écrit :
> Using the same type across platforms is more robust and avoids platform
> specific issues from differences in range.
> Also fixed point integers are on a semantical level not size_t
> 
> Previous version reviewed-by: James Almer <jamrial@gmail.com>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

If all the numbers are fixed-points in 0.32 precision, then there is
absolutely no doubt that this is morally correct.

Regards,
Vittorio Giovara March 14, 2017, 3:37 p.m.
On Sun, Mar 12, 2017 at 11:21 PM, Michael Niedermayer
<michael@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@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).

>> > > Also fixed point integers are on a semantical level not size_t

This is only theoretical, and, since we're talking about semantics,
you're breaking ABI by using sizeof(AVSphericalMapping) outside of
libavutil.

>> > > Previous version reviewed-by: James Almer <jamrial@gmail.com>
>> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> > > ---
>> > >  ffprobe.c             |  2 +-
>> > >  libavformat/dump.c    |  6 +++---
>> > >  libavutil/spherical.c |  6 +++---
>> > >  libavutil/spherical.h | 16 ++++++++--------
>> > >  4 files changed, 15 insertions(+), 15 deletions(-)
>> >
>> > Needs FATE update.
>>
>>
>> Since this is Vittorio's code, it's probably good to ask him [CC]? From
>
> yes,
>
> semi on topic: but i think people interrested in FFmpeg
> development and the direction the code goes should subscribe to
> ffmpeg-devel. This CC-ing is not too practical

My subscription to ffmpeg-security was rejected, if you can rectify
that, I'll subscribe to ffmpeg-devel too.
Hendrik Leppkes March 14, 2017, 4 p.m.
On Tue, Mar 14, 2017 at 4:37 PM, Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
> On Sun, Mar 12, 2017 at 11:21 PM, Michael Niedermayer
> <michael@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@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).

Changing types of a new struct shortly after it has been introduced
and not part of any release is perfectly acceptable.

- Hendrik
James Almer March 14, 2017, 4:12 p.m.
On 3/14/2017 12:37 PM, Vittorio Giovara wrote:
> On Sun, Mar 12, 2017 at 11:21 PM, Michael Niedermayer
> <michael@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@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.

> 
>>>>> 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.

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.

> 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.

> 
>>>>> Previous version reviewed-by: James Almer <jamrial@gmail.com>
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  ffprobe.c             |  2 +-
>>>>>  libavformat/dump.c    |  6 +++---
>>>>>  libavutil/spherical.c |  6 +++---
>>>>>  libavutil/spherical.h | 16 ++++++++--------
>>>>>  4 files changed, 15 insertions(+), 15 deletions(-)
>>>>
>>>> Needs FATE update.
>>>
>>>
>>> Since this is Vittorio's code, it's probably good to ask him [CC]? From
>>
>> yes,
>>
>> semi on topic: but i think people interrested in FFmpeg
>> development and the direction the code goes should subscribe to
>> ffmpeg-devel. This CC-ing is not too practical
> 
> My subscription to ffmpeg-security was rejected, if you can rectify
> that, I'll subscribe to ffmpeg-devel too.
>
Vittorio Giovara March 14, 2017, 7:46 p.m.
On Tue, Mar 14, 2017 at 12:12 PM, James Almer <jamrial@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@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@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.

>>>>>> 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

> 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.

>> 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? 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.
James Almer March 14, 2017, 8:41 p.m.
On 3/14/2017 4:46 PM, Vittorio Giovara wrote:
> On Tue, Mar 14, 2017 at 12:12 PM, James Almer <jamrial@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@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@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.
Vittorio Giovara March 14, 2017, 9:16 p.m.
On Tue, Mar 14, 2017 at 4:41 PM, James Almer <jamrial@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@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@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@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.
James Almer March 14, 2017, 9:27 p.m.
On 3/14/2017 6:16 PM, Vittorio Giovara wrote:
> On Tue, Mar 14, 2017 at 4:41 PM, James Almer <jamrial@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@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@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@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.
Vittorio Giovara March 14, 2017, 9:43 p.m.
On Tue, Mar 14, 2017 at 5:27 PM, James Almer <jamrial@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@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@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@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@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 Giovara March 14, 2017, 9:58 p.m.
On Tue, Mar 14, 2017 at 5:27 PM, James Almer <jamrial@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@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@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@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@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.

Patch hide | download patch | download mbox

diff --git a/ffprobe.c b/ffprobe.c
index b104390990..b3466b1708 100644
--- a/ffprobe.c
+++ b/ffprobe.c
@@ -1793,7 +1793,7 @@  static void print_pkt_side_data(WriterContext *w,
                 print_str("projection", "cubemap");
                 print_int("padding", spherical->padding);
             } else if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) {
-                size_t l, t, r, b;
+                uint32_t l, t, r, b;
                 av_spherical_tile_bounds(spherical, par->width, par->height,
                                          &l, &t, &r, &b);
                 print_str("projection", "tiled equirectangular");
diff --git a/libavformat/dump.c b/libavformat/dump.c
index 505d572301..a6309cbf71 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -370,12 +370,12 @@  static void dump_spherical(void *ctx, AVCodecParameters *par, AVPacketSideData *
     av_log(ctx, AV_LOG_INFO, "(%f/%f/%f) ", yaw, pitch, roll);
 
     if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) {
-        size_t l, t, r, b;
+        uint32_t l, t, r, b;
         av_spherical_tile_bounds(spherical, par->width, par->height,
                                  &l, &t, &r, &b);
-        av_log(ctx, AV_LOG_INFO, "[%zu, %zu, %zu, %zu] ", l, t, r, b);
+        av_log(ctx, AV_LOG_INFO, "[%"PRIu32", %"PRIu32", %"PRIu32", %"PRIu32"] ", l, t, r, b);
     } else if (spherical->projection == AV_SPHERICAL_CUBEMAP) {
-        av_log(ctx, AV_LOG_INFO, "[pad %zu] ", spherical->padding);
+        av_log(ctx, AV_LOG_INFO, "[pad %"PRIu32"] ", spherical->padding);
     }
 }
 
diff --git a/libavutil/spherical.c b/libavutil/spherical.c
index 0ca2dd367a..66056961a0 100644
--- a/libavutil/spherical.c
+++ b/libavutil/spherical.c
@@ -34,9 +34,9 @@  AVSphericalMapping *av_spherical_alloc(size_t *size)
 }
 
 void av_spherical_tile_bounds(AVSphericalMapping *map,
-                              size_t width, size_t height,
-                              size_t *left, size_t *top,
-                              size_t *right, size_t *bottom)
+                              uint32_t width, uint32_t height,
+                              uint32_t *left, uint32_t*top,
+                              uint32_t *right, uint32_t *bottom)
 {
     /* conversion from 0.32 coordinates to pixels */
     uint64_t orig_width  = (uint64_t) width  * UINT32_MAX /
diff --git a/libavutil/spherical.h b/libavutil/spherical.h
index db9bdc0be5..ff25c139ee 100644
--- a/libavutil/spherical.h
+++ b/libavutil/spherical.h
@@ -164,10 +164,10 @@  typedef struct AVSphericalMapping {
      *       projection type (@ref AV_SPHERICAL_EQUIRECTANGULAR_TILE),
      *       and should be ignored in all other cases.
      */
-    size_t bound_left;   ///< Distance from the left edge
-    size_t bound_top;    ///< Distance from the top edge
-    size_t bound_right;  ///< Distance from the right edge
-    size_t bound_bottom; ///< Distance from the bottom edge
+    uint32_t bound_left;   ///< Distance from the left edge
+    uint32_t bound_top;    ///< Distance from the top edge
+    uint32_t bound_right;  ///< Distance from the right edge
+    uint32_t bound_bottom; ///< Distance from the bottom edge
     /**
      * @}
      */
@@ -179,7 +179,7 @@  typedef struct AVSphericalMapping {
      *       (@ref AV_SPHERICAL_CUBEMAP), and should be ignored in all other
      *       cases.
      */
-    size_t padding;
+    uint32_t padding;
 } AVSphericalMapping;
 
 /**
@@ -203,9 +203,9 @@  AVSphericalMapping *av_spherical_alloc(size_t *size);
  * @param bottom Pixels from the bottom edge.
  */
 void av_spherical_tile_bounds(AVSphericalMapping *map,
-                              size_t width, size_t height,
-                              size_t *left, size_t *top,
-                              size_t *right, size_t *bottom);
+                              uint32_t width, uint32_t height,
+                              uint32_t *left, uint32_t *top,
+                              uint32_t *right, uint32_t *bottom);
 /**
  * @}
  * @}