[FFmpeg-devel] [PATCH 1/2] aptx: implement the aptX bluetooth codec

[James Almer]

On 11/9/2017 10:37 AM, Aurelien Jacobs wrote:
> On Thu, Nov 09, 2017 at 12:52:34AM +0000, Rostislav Pehlivanov wrote:
>> On 8 November 2017 at 22:41, Aurelien Jacobs <aurel at gnuage.org> wrote:
>>
>>> On Wed, Nov 08, 2017 at 06:26:03PM +0100, Michael Niedermayer wrote:
>>>> On Wed, Nov 08, 2017 at 02:06:09PM +0100, Aurelien Jacobs wrote:
>>>> [...]
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Compute the convolution of the signal with the coefficients, and
>>> reduce
>>>>> + * to 24 bits by applying the specified right shifting.
>>>>> + */
>>>>> +av_always_inline
>>>>> +static int32_t aptx_qmf_convolution(FilterSignal *signal,
>>>>> +                                    const int32_t coeffs[FILTER_TAPS],
>>>>> +                                    int shift)
>>>>> +{
>>>>> +    int32_t *sig = &signal->buffer[signal->pos];
>>>>> +    int64_t e = 0;
>>>>> +
>>>>
>>>>> +    for (int i = 0; i < FILTER_TAPS; i++)
>>>>
>>>> "for (int" is something we avoided as some comilers didnt like it,
>>>> iam not sure if this is still true but there are none in the codebase
>>>
>>> If you insist on this I will of course change it, but hey, we require
>>> a C99 compiler since a long time and we use so many features of C99
>>> that I really don't get why we couldn't use "for (int".
>>> I can't imagine that any relevant C compiler would not support this
>>> nowadays !
>>> What I propose is to get this in as is, and see if anyone encounter
>>> issue with any compiler. If any issue arise, I will of course send a
>>> patch to fix it.
>>>
>>> Here is an updated patch.
>>>
>>>
>> Send another patch because some people are beyond convincing and its really
>> pissing me off. Really. In particular maybe those compiler writers at
>> Microsoft who seem to think shipping something that doesn't support such a
>> simple yet important feature is important.
>> Or maybe users who don't want to update a 6 year old version of msvc.
>> Or maybe us because we support compiling with msvc at all when its clearly
>> _not_ a C compiler and this project is written in C.
> 
> Here you go.
> 
> Just for reference, splitting the déclaration out of the for loop added
> 19 lines in this file, which is a 2.3 % increase of the line count.
> (I'm not sure this file is representative of the rest of the code base)

If you combine lines like

+    int channel;
+    int ret;

and

+    int pos, channel, sample;
+    int ret;

You can reduce that a bit.

Don't send a new patch just for that. Only if there's some other change
requested. Whoever commits this can change it before pushing.