[FFmpeg-devel] [PATCH v2 1/3] libavutil/imgutils: add utility to get plane sizes

[James Almer]

On 7/14/2020 5:49 PM, Michael Niedermayer wrote:
> On Tue, Jul 14, 2020 at 11:19:54AM -0300, James Almer wrote:
>> On 7/14/2020 9:47 AM, Nicolas George wrote:
>>> James Almer (12020-07-10):
>>>> Because my opinion and tastes are not yours. I already said why *i*
>>>> consider it ugly. It doesn't need to fit *your* conception of ugliness.
>>>
>>> If it is only a matter of taste, then it cannot count as an argument.
>>> But tastes are frequently a proxy for minor factors. If you can express
>>> the minor factors behind your tastes, they can count as arguments.
>>>
>>> Which is what I am trying to do about my tastes.
>>>
>>> One of these minor factors: there is frequently a "int ret" variable and
>>> a "return ret" at the end. If the return value conflates size with the
>>> error code, ret cannot be used, which means some kind of "if (size < 0)
>>> ret = size;" need to happen, but would easily be forgotten, initially or
>>> in refactoring.
>>>
>>>> int av_image_fill_plane_sizes(size_t sizes[4], enum AVPixelFormat
>>>> pix_fmt, const ptrdiff_t linesizes[4]);
>>>
>>> So the question is now: is the total size useful enough to warrant an
>>> extra return parameter or not? I have no advice on the question.
>>
>> Not for an extra parameter (Who doesn't love enterprise-style code full
>> of function calls filled with NULL arguments, right?), but yes for using
>> the otherwise undefined >0 scope of the return value, which is only
>> possible if we keep the sizes as int like the rest of the module, and to
>> be honest something i would very much like to do seeing almost every
>> other existing function can't be ported to size_t/ptrdiff_t.
>> A complete, consistent set of new functions would have to be introduced
>> for that purpose.
>>
>> I'd like to convince Michael about it, since he suggested these types,
>> but if i can't then this is the approach most consistent with existing
>> size array filling functions.
> 
> Let me phrase my suggestion in a more high level sense
> 
> Currently the functions use int for lots of cases where they should not
> ultimately we want the functions to use more correct types for linesize
> sizes and so on.
> 
> We need to add these function(s) because we fix a bug.
> Can we take a step toward more correct types while doing this in a
> way that makes sense ?
> 
> if so that should be done.
> 
> If not (as in its more mess than if its done later in some other way)
> then we should not try to do this now
> 
> The idea is just to take a step towards how these functions/API should look
> ideally. Its not to make some inconvenient mess

The issue is that most existing functions can't be ported to
ptrdiff_t/size_t with a mere type switch after a soname bump, which
means to make such a move we'd need to introduce an entire set of new
imgutils functions with a different design approach for this purpose.

This is why i consider it may be a better idea to define this new single
function in a consistent way with the existing API, including types and
signature, which is what the first iteration of this set attempted to
do, and leave the move to proper types for a rewrite of the module.
If not, the v3 posted yesterday is currently the best non-int approach.