On Tue, Mar 14, 2017 at 5:27 PM, James Almer <jamr...@gmail.com> wrote: > On 3/14/2017 6:16 PM, Vittorio Giovara wrote: >> 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. > > 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
