On Tue, Mar 14, 2017 at 4:41 PM, James Almer <jamr...@gmail.com> wrote: > On 3/14/2017 4:46 PM, Vittorio Giovara wrote: >> On Tue, Mar 14, 2017 at 12:12 PM, James Almer <jamr...@gmail.com> wrote: >>> On 3/14/2017 12:37 PM, Vittorio Giovara wrote: >>>> On Sun, Mar 12, 2017 at 11:21 PM, Michael Niedermayer >>>> <mich...@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 <jamr...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
