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

Patch attached.

Main changes from v1 are switching to from int to size_t/ptrdiff_t
where relevant and removing av_image_fill_pointers_from_sizes()

Some things to note:
 - The av_image_fill_planes_sizes uses size_t/ptrdiff_t for buffer
sizes, but as mentioned during the v1 review, this leads to some
inconvenient conversions and range checks when using it with existing
functions. We could keep the return type as int to alleviate this, but
that seems like it would somewhat defeat the purpose of moving to
these types.
 - SIZE_MAX is used in several places in libavutil, so I used
PTRDIFF_MAX, but I could not see any mention of these being allowed.
Subject: [PATCH 1/3] libavutil/imgutils: add utility to get plane sizes

This utility helps avoid undefined behavior when doing things like
checking how much memory we need to allocate for an image before we have
allocated a buffer.

Signed-off-by: Brian Kim <bkkim@google.com>
---
 doc/APIchanges       |  3 ++
 libavutil/frame.c    | 15 ++++++---
 libavutil/imgutils.c | 74 ++++++++++++++++++++++++++++++++------------
 libavutil/imgutils.h | 12 +++++++
 libavutil/version.h  |  2 +-
 5 files changed, 82 insertions(+), 24 deletions(-)

Patch attached.

There was some discussion on the v1 thread on whether it was
acceptable to break code that was relying on UB, so this patch will
probably want to get delayed until a major version bump to avoid
breaking places that were relying on av_image_fill_pointers()
populating data when the input ptr is null

On 7/8/2020 10:54 PM, Brian Kim wrote:
> Patch attached.
> 
> There was some discussion on the v1 thread on whether it was
> acceptable to break code that was relying on UB, so this patch will
> probably want to get delayed until a major version bump to avoid
> breaking places that were relying on av_image_fill_pointers()
> populating data when the input ptr is null

> From 2c269118523de0911f17a4b560b016c34fc3002f Mon Sep 17 00:00:00 2001
> From: Brian Kim <bkkim@google.com>
> Date: Tue, 7 Jul 2020 11:42:35 -0700
> Subject: [PATCH 3/3] libavutil/imgutils: check for non-null buffer in
>  av_image_fill_pointers
> 
> We were previously always filling data by adding offsets to ptr, which
> was undefined behavior when ptr was NULL.
> 
> Signed-off-by: Brian Kim <bkkim@google.com>
> ---
>  libavutil/imgutils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 082229cfaf..3898c5e771 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -155,6 +155,9 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
>      ptrdiff_t ret, linesizes1[4];
>      size_t size[4];
>  
> +    if (!ptr)
> +        return AVERROR(EINVAL);

No, check !ptr immediately after the memset() call (to zero the data
array), and if it's NULL then return the value from
av_image_fill_plane_sizes().

No reason to break calls with ptr == NULL since calling it to get the
total size of the buffer you want to allocate is a valid scenario where
the contents of *data[4] are meaningless and thus ignored. You just want
to fix the UB of adding offsets to NULL.

This will still need to wait until a major bump nonetheless.

> +
>      for (i = 0; i < 4; i++)
>          linesizes1[i] = linesizes[i];
>  
> -- 
> 2.27.0.383.g050319c2ae-goog
>

On 7/8/2020 10:50 PM, Brian Kim wrote:
> Patch attached.
> 
> Main changes from v1 are switching to from int to size_t/ptrdiff_t
> where relevant and removing av_image_fill_pointers_from_sizes()
> 
> Some things to note:
>  - The av_image_fill_planes_sizes uses size_t/ptrdiff_t for buffer
> sizes, but as mentioned during the v1 review, this leads to some
> inconvenient conversions and range checks when using it with existing
> functions. We could keep the return type as int to alleviate this, but
> that seems like it would somewhat defeat the purpose of moving to
> these types.
>  - SIZE_MAX is used in several places in libavutil, so I used
> PTRDIFF_MAX, but I could not see any mention of these being allowed.

[...]

> From a47b3f3b5c2ed4a1caf9f0a3429dd425ce708bb1 Mon Sep 17 00:00:00 2001
> From: Brian Kim <bkkim@google.com>
> Date: Tue, 7 Jul 2020 11:36:19 -0700
> Subject: [PATCH 1/3] libavutil/imgutils: add utility to get plane sizes
> 
> This utility helps avoid undefined behavior when doing things like
> checking how much memory we need to allocate for an image before we have
> allocated a buffer.
> 
> Signed-off-by: Brian Kim <bkkim@google.com>
> ---
>  doc/APIchanges       |  3 ++
>  libavutil/frame.c    | 15 ++++++---
>  libavutil/imgutils.c | 74 ++++++++++++++++++++++++++++++++------------
>  libavutil/imgutils.h | 12 +++++++
>  libavutil/version.h  |  2 +-
>  5 files changed, 82 insertions(+), 24 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 1d6cc36b8c..44defd9ca8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-07-xx - xxxxxxxxxx - lavu 56.56.100 - imgutils.h
> +  Add av_image_fill_plane_sizes().
> +
>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
>    Add AV_PIX_FMT_X2RGB10.
>  
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9884eae054..b664dcfbe8 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c

Changes to frame.c should be in a separate commit as well.

> @@ -214,6 +214,8 @@ static int get_video_buffer(AVFrame *frame, int align)
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
>      int ret, i, padded_height;
>      int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);
> +    ptrdiff_t total_size, linesizes[4];
> +    size_t size[4];
>  
>      if (!desc)
>          return AVERROR(EINVAL);
> @@ -238,12 +240,17 @@ static int get_video_buffer(AVFrame *frame, int align)
>              frame->linesize[i] = FFALIGN(frame->linesize[i], align);
>      }
>  
> +    for (i = 0; i < 4; i++)
> +        linesizes[i] = frame->linesize[i];
> +
>      padded_height = FFALIGN(frame->height, 32);
> -    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
> -                                      NULL, frame->linesize)) < 0)
> -        return ret;
> +    if ((total_size = av_image_fill_plane_sizes(size, frame->format, padded_height,
> +                                                linesizes)) < 0)
> +        return total_size;
> +    if (total_size > INT_MAX - 4*plane_padding)
> +        return AVERROR(EINVAL);
>  
> -    frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding);
> +    frame->buf[0] = av_buffer_alloc(total_size + 4*plane_padding);
>      if (!frame->buf[0]) {
>          ret = AVERROR(ENOMEM);
>          goto fail;
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 7f9c1b632c..082229cfaf 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -108,26 +108,26 @@ int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int wi
>      return 0;
>  }
>  
> -int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height,
> -                           uint8_t *ptr, const int linesizes[4])
> +ptrdiff_t av_image_fill_plane_sizes(size_t size[4], enum AVPixelFormat pix_fmt,

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.

> +                                    int height, const ptrdiff_t linesizes[4])
>  {
> -    int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 };
> +    int i, has_plane[4] = { 0 };
> +    ptrdiff_t total_size;
>  
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
> -    memset(data     , 0, sizeof(data[0])*4);
> +    memset(size     , 0, sizeof(size[0])*4);
>  
>      if (!desc || desc->flags & AV_PIX_FMT_FLAG_HWACCEL)
>          return AVERROR(EINVAL);
>  
> -    data[0] = ptr;
> -    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.

Same for other similar calculations in the function.

>          return AVERROR(EINVAL);
>      size[0] = linesizes[0] * height;

[...]

On Wed, Jul 8, 2020 at 7:42 PM James Almer <jamrial@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.

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?

On 7/9/2020 3:20 PM, Brian Kim wrote:
> [...]
> 
> On Wed, Jul 8, 2020 at 7:42 PM James Almer <jamrial@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.

James Almer (12020-07-09):
> 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.

I find this is by far the best solution. Even more so since the total
size is not the main output of the function as specified by its name. It
is also more future-proof, and allows potential consistency for cases
that return a pointer.

I would be curious to know what you find ugly with this; I am frequently
baffled by similar judgements of yours. Would you try to analyze it?

Regards,

On 7/10/2020 9:03 AM, Nicolas George wrote:
> James Almer (12020-07-09):
>> 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.
> 
> I find this is by far the best solution. Even more so since the total
> size is not the main output of the function as specified by its name. It
> is also more future-proof, and allows potential consistency for cases
> that return a pointer.
> 
> I would be curious to know what you find ugly with this; I am frequently
> baffled by similar judgements of yours. Would you try to analyze it?
> 
> Regards,

It's adding an extra parameter to get a value that ultimately can be
derived from the output of another parameter. If you can use the >0 part
of the return value for that, then to me it sounds ideal.
It's not like the functions that allocate a struct, where if you return
the pointer instead of passing it as an output parameter you lose the
ability to signal errors. In those this method is nicer.

I'd very much prefer to keep this new function consistent with either
av_image_fill_linesizes() (Output array as parameter, no sum of sizes as
return value), or av_image_fill_pointers() (Output array as parameter,
sum of sizes as return value).

Also, we could schedule changing all linesize parameters to ptrfdiff_t
on the next bump.

James Almer (12020-07-10):
> It's adding an extra parameter to get a value that ultimately can be
> derived from the output of another parameter. If you can use the >0 part
> of the return value for that,

Yes. And for me, it is totally a good thing.

So why are you calling it ugly?

This discussion feels to me like "Why is planting trees a bad thing? -
It makes the neighborhood greener."

Ugly is using the same channel to carry to completely different pieces
of information. And extra parameters are not a bad thing.

Regards,

On 7/10/2020 11:29 AM, Nicolas George wrote:
> James Almer (12020-07-10):
>> It's adding an extra parameter to get a value that ultimately can be
>> derived from the output of another parameter. If you can use the >0 part
>> of the return value for that,
> 
> Yes. And for me, it is totally a good thing.

Then we have different opinions.

> 
> So why are you calling it ugly?

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.

> 
> This discussion feels to me like "Why is planting trees a bad thing? -
> It makes the neighborhood greener."
> 
> Ugly is using the same channel to carry to completely different pieces
> of information. And extra parameters are not a bad thing.

They are a good thing when they provide useful information that can't be
derived from other information, or at least not easily. So returning
pointers, plane sizes and line sizes in separate parameters, like
av_image_fill_pointers() does, is great. But the suggestion i made
earlier as an alternative to this is superfluous duplication.

So lets do

int av_image_fill_plane_sizes(size_t sizes[4], enum AVPixelFormat
pix_fmt, const ptrdiff_t linesizes[4]);

Making it consistent with av_image_fill_linesizes() (ideal approach
considering the point of the function is also to fill an array of sizes)
while also introducing size_t and ptrdiff_t to start the eventual type
migration for the rest of the module.

> 
> Regards,
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

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.

Regards,

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.

> 
> Regards,

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

thx

[...]

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.

Michael Niedermayer (12020-07-14):
> 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

I strongly agree.

If we use ints now, then the next time the same question comes this
function will be one more argument for using ints again. And the next
time, and the next time, all this making that much work for when we
cannot wait any longer to make the change, instead of that much less.

Consistency is not a end in itself, it is a means to an end. And it is
one of the weakest arguments. If there are no other reason for doing things
one way or another, then sure, by all means let us choose the way that
looks the same at the rest of the code. But if there is a reason, if one
way is more efficient, or more convenient, or more future proof, or more
compatible, etc., then we choose this way, and too bad for consistency.

Regards,

On 7/15/2020 4:06 AM, Nicolas George wrote:
> Michael Niedermayer (12020-07-14):
>> 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
> 
> I strongly agree.
> 
> If we use ints now, then the next time the same question comes this
> function will be one more argument for using ints again. And the next
> time, and the next time, all this making that much work for when we
> cannot wait any longer to make the change, instead of that much less.

I don't share the sentiment, since as i said an entire new set of
functions will have to be added for a module-wide type switch. The way
this function is designed here will not increase or reduce any future
work in any way whatsoever. It's meant to be a solution today for a
problem in the current state of the imgutils module.

For all we know, by the time ints start being a real issue or the need
to replace the current functions arise, the new set of functions might
have to be designed in a way this one wont be reusable. For example,
will AVPixelFormat have been replaced by then with an alternative that
removes the need to have 20 entries to cover all bitdepths, chroma
variants, endianness, and plane presence/order, for what's ultimately a
single format?

Sure, at first glance using the proper types here will seem like making
a step forward, but we may instead be getting ahead of ourselves for no
real gain. av_image_check_size() is truly future proof, as is
av_image_check_sar(), but most others aren't, and that includes this one
regardless of the data type we choose.

> 
> Consistency is not a end in itself, it is a means to an end. And it is
> one of the weakest arguments. If there are no other reason for doing things
> one way or another, then sure, by all means let us choose the way that
> looks the same at the rest of the code. But if there is a reason, if one
> way is more efficient, or more convenient, or more future proof, or more
> compatible, etc., then we choose this way, and too bad for consistency.
> 
> Regards,

Just wanted to check if there was any consensus on what we wanted to do
with these changes. Are we holding off until a future module wide change?

On Wed, Jul 15, 2020 at 8:09 AM James Almer <jamrial@gmail.com> wrote:

> On 7/15/2020 4:06 AM, Nicolas George wrote:
> > Michael Niedermayer (12020-07-14):
> >> 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
> >
> > I strongly agree.
> >
> > If we use ints now, then the next time the same question comes this
> > function will be one more argument for using ints again. And the next
> > time, and the next time, all this making that much work for when we
> > cannot wait any longer to make the change, instead of that much less.
>
> I don't share the sentiment, since as i said an entire new set of
> functions will have to be added for a module-wide type switch. The way
> this function is designed here will not increase or reduce any future
> work in any way whatsoever. It's meant to be a solution today for a
> problem in the current state of the imgutils module.
>
> For all we know, by the time ints start being a real issue or the need
> to replace the current functions arise, the new set of functions might
> have to be designed in a way this one wont be reusable. For example,
> will AVPixelFormat have been replaced by then with an alternative that
> removes the need to have 20 entries to cover all bitdepths, chroma
> variants, endianness, and plane presence/order, for what's ultimately a
> single format?
>
> Sure, at first glance using the proper types here will seem like making
> a step forward, but we may instead be getting ahead of ourselves for no
> real gain. av_image_check_size() is truly future proof, as is
> av_image_check_sar(), but most others aren't, and that includes this one
> regardless of the data type we choose.
>
> >
> > Consistency is not a end in itself, it is a means to an end. And it is
> > one of the weakest arguments. If there are no other reason for doing
> things
> > one way or another, then sure, by all means let us choose the way that
> > looks the same at the rest of the code. But if there is a reason, if one
> > way is more efficient, or more convenient, or more future proof, or more
> > compatible, etc., then we choose this way, and too bad for consistency.
> >
> > Regards,
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 7/20/2020 6:32 PM, Brian Kim wrote:
> Just wanted to check if there was any consensus on what we wanted to do
> with these changes. Are we holding off until a future module wide change?

No, i'll push v3 soon if my argumentation below was not enough to
convince Nicolas or Michael. My intention is to use ints for the new
function, not to postpone committing it in any form indefinitely.

> 
> On Wed, Jul 15, 2020 at 8:09 AM James Almer <jamrial@gmail.com> wrote:
> 
>> On 7/15/2020 4:06 AM, Nicolas George wrote:
>>> Michael Niedermayer (12020-07-14):
>>>> 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
>>>
>>> I strongly agree.
>>>
>>> If we use ints now, then the next time the same question comes this
>>> function will be one more argument for using ints again. And the next
>>> time, and the next time, all this making that much work for when we
>>> cannot wait any longer to make the change, instead of that much less.
>>
>> I don't share the sentiment, since as i said an entire new set of
>> functions will have to be added for a module-wide type switch. The way
>> this function is designed here will not increase or reduce any future
>> work in any way whatsoever. It's meant to be a solution today for a
>> problem in the current state of the imgutils module.
>>
>> For all we know, by the time ints start being a real issue or the need
>> to replace the current functions arise, the new set of functions might
>> have to be designed in a way this one wont be reusable. For example,
>> will AVPixelFormat have been replaced by then with an alternative that
>> removes the need to have 20 entries to cover all bitdepths, chroma
>> variants, endianness, and plane presence/order, for what's ultimately a
>> single format?
>>
>> Sure, at first glance using the proper types here will seem like making
>> a step forward, but we may instead be getting ahead of ourselves for no
>> real gain. av_image_check_size() is truly future proof, as is
>> av_image_check_sar(), but most others aren't, and that includes this one
>> regardless of the data type we choose.
>>
>>>
>>> Consistency is not a end in itself, it is a means to an end. And it is
>>> one of the weakest arguments. If there are no other reason for doing
>> things
>>> one way or another, then sure, by all means let us choose the way that
>>> looks the same at the rest of the code. But if there is a reason, if one
>>> way is more efficient, or more convenient, or more future proof, or more
>>> compatible, etc., then we choose this way, and too bad for consistency.
>>>
>>> Regards,
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

James Almer (12020-07-20):
> No, i'll push v3 soon if my argumentation below was not enough to
> convince Nicolas or Michael. My intention is to use ints for the new
> function, not to postpone committing it in any form indefinitely.

Sorry, I missed you mail earlier, I only read your arguments nowish.

You are emphasizing that the "future-proof" argument is rather weak,
which I was already aware. And you are underplaying the fact that it
belongs to a trend to always delay necessary changes.

Anyway, even if all the arguments for using the proper types are all
very weak, there are several, and together I am still convinced they
exceed "consistency" easily.

consistently good > inconsistently good and bad > consistently bad
                                                ^
                                                |
             this is the discussion we are having

Or, as John Oliver pointed last Sunday: just because you reopened
restaurants does not mean you have to reopen schools too for the sake of
consistency.

Regards,

On 2020-07-22 11:56 +0200, Nicolas George wrote:
> James Almer (12020-07-20):
> > No, i'll push v3 soon if my argumentation below was not enough to
> > convince Nicolas or Michael. My intention is to use ints for the new
> > function, not to postpone committing it in any form indefinitely.
>
> Sorry, I missed you mail earlier, I only read your arguments nowish.
>
> You are emphasizing that the "future-proof" argument is rather weak,
> which I was already aware. And you are underplaying the fact that it
> belongs to a trend to always delay necessary changes.
>
> Anyway, even if all the arguments for using the proper types are all
> very weak, there are several, and together I am still convinced they
> exceed "consistency" easily.
>
> consistently good > inconsistently good and bad > consistently bad
>                                                 ^
>                                                 |
>              this is the discussion we are having

From what I have read so far this is not a simple black and white
decision.

I think it's a discussion about coarseness of change. Improving
everything at once will likely never happen in a good way. But
still there are more step sizes in between.

Here is a excerpt of what Michael wrote before:

> 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

Changing at a per function level is the finest possible way of
API adjustment. Changing a group of functions would be the next
coarser way to adjust the API, and that's what I think James
opts for.

I personally would be OK with that strategy. It emphasizes on
the value of API users. As an API user I would find it annoying
to have inconsistencies at a per function level, even in the
same group of functions that are tied together by name and
by header.

In the long term, I believe we should focus on even more
consistency among groups of functions and libraries and
even cut down on the number of API functions needed.


  Alexander