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

[James Almer]

On 7/9/2020 3:20 PM, Brian Kim wrote:
> [...]
> 
> On Wed, Jul 8, 2020 at 7:42 PM James Almer <jamrial at gmail.com> wrote:
>> The sum of all sizes should be size_t if each size is a size_t. But if
>> you do that you can't return error values, so i'm not sure what to
>> suggest other than just stick to ints for both (ptrdiff_t linesizes
>> should be ok).
>> I'd like to hear Michael's opinion about it.
>>
>> For that matter, as it is right now i think this would be the first
>> function that returns ptrdiff_t to signal AVERROR values.
> 
> Yeah, I wasn't sure what the best thing to do here would be since it
> seemed like switching back to ints for the outputs would make the
> ptrdiff_t linesizes less useful. We could have each size be a
> ptrdiff_t as well, but that seems a bit weird too.

Yes, the sizes (total or per plane) are for objects in memory, so they
should be either size_t, or int to be consistent with other functions in
this module.

> 
> There is one function in libavformat (ff_subtitles_read_line) that
> returns a negative error ptrdiff_t to signal errors.
> 
> [...]
> 
>>> -    if (linesizes[0] > (INT_MAX - 1024) / height)
>>> +    if (linesizes[0] > (PTRDIFF_MAX - 1024) / height)
>>
>> This check would need to ensure the calculation below fits in a size_t
>> instead.
> 
> If the return type is ptrdiff_t, then I guess we would need to check
> both. I had thought ptrdiff_t was the corresponding signed type for
> size_t (so it would be guaranteed to have a lower max), but I guess
> that isn't guaranteed?

Technically, the signed type for size_t is ssize_t, but afaik that's not
portable so we don't use it.
My point is, linesize[0] * height could fit in a size_t but not in a
ptrdiff_t, and you care about the former since that's the type for size[0].

I still think the best course of action here is to just stick with ints,
but i want to hear what other devs have to say about it.
Another option is to return the sum of all plane sizes in another
function parameter, and make the return value an int where 0 means
success and <0 failure, so:

int av_image_fill_plane_sizes(size_t *size, size_t planesizes[4], enum
AVPixelFormat pix_fmt, int height, const ptrdiff_t linesizes[4])

But it's also somewhat ugly and inconsistent with other functions in
this module.