[FFmpeg-devel] [PATCH] all: simplify qsort comparators, and add const-correctness

[Ganesh Ajjanagadde]

On Sat, Oct 24, 2015 at 9:32 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Sat, Oct 24, 2015 at 9:02 PM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> wrote:
>>
>> All the comparator API needs is > 0, < 0, or = 0 signalling: it does not
>> need +1, -1, 0. This avoids some useless branching.
>> [..]
>>
>> diff --git a/cmdutils_opencl.c b/cmdutils_opencl.c
>> index 61478e2..d9095b6 100644
>> --- a/cmdutils_opencl.c
>> +++ b/cmdutils_opencl.c
>> @@ -206,7 +206,7 @@ end:
>>
>>  static int compare_ocl_device_desc(const void *a, const void *b)
>>  {
>> -    return ((OpenCLDeviceBenchmark*)a)->runtime -
>> ((OpenCLDeviceBenchmark*)b)->runtime;
>> +    return ((const OpenCLDeviceBenchmark*)a)->runtime - ((const
>> OpenCLDeviceBenchmark*)b)->runtime;
>>  }
>
>
> OK.
>
>>
>> @@ -2578,8 +2578,7 @@ static InputStream *get_input_stream(OutputStream
>> *ost)
>>
>>  static int compare_int64(const void *a, const void *b)
>>  {
>> -    int64_t va = *(int64_t *)a, vb = *(int64_t *)b;
>> -    return va < vb ? -1 : va > vb ? +1 : 0;
>> +    return *(const int64_t *)a - *(const int64_t *)b;
>>  }
>
>
> What if the result doesn't fit in int? The input is not int.
>
>>
>> @@ -367,7 +367,7 @@ static int cmp_intervals(const void *a, const void *b)
>>      int64_t ts_diff = i1->start_ts - i2->start_ts;
>>      int ret;
>>
>> -    ret = ts_diff > 0 ? 1 : ts_diff < 0 ? -1 : 0;
>> +    ret = ts_diff;
>>      return ret == 0 ? i1->index - i2->index : ret;
>>  }
>
>
> Same here.
>
>>
>>  static int cmp_int(const void *p1, const void *p2)
>>  {
>> -    int left = *(const int *)p1;
>> -    int right = *(const int *)p2;
>> -
>> -    return ((left > right) - (left < right));
>> +    return *(const int *)p1 - *(const int *)p2;
>>  }
>
>
> OK.
>
>>
>> @@ -146,12 +146,9 @@ static int cmp_pkt_sub_ts_pos(const void *a, const
>> void *b)
>>  {
>>      const AVPacket *s1 = a;
>>      const AVPacket *s2 = b;
>> -    if (s1->pts == s2->pts) {
>> -        if (s1->pos == s2->pos)
>> -            return 0;
>> -        return s1->pos > s2->pos ? 1 : -1;
>> -    }
>> -    return s1->pts > s2->pts ? 1 : -1;
>> +    if (s1->pts == s2->pts)
>> +        return s1->pos - s2->pos;
>> +    return s1->pts - s2->pts;
>>  }
>
>
> pos is also int64_t.
>
>>
>> -static int cmp(const int *a, const int *b){
>> -    return *a - *b;
>> +static int cmp(const void *a, const void *b){
>> +    return *(const int *)a - *(const int *)b;
>>  }
>
>
> OK.
>
>>
>> -    qsort(remaining_tests + max_tests - num_tests, num_tests,
>> sizeof(remaining_tests[0]), (void*)cmp);
>> +    qsort(remaining_tests + max_tests - num_tests, num_tests,
>> sizeof(remaining_tests[0]), cmp);
>
>
> I thought all qsorts were changed to AV_QSORT?

Such a wholesale change was not liked by wm4 and Clement due to the
increase in binary size. I have a patch for one such instance that I
believe should be replaced: the performance hit of per frame calls to
qsort as opposed to AV_QSORT in avcodec has bigger impact than the
increase in binary size IMO. This has been ok'ed by Michael.

Thus, this change will be done gradually, with performance measurements.

>
> Ronald