[FFmpeg-devel] [PATCH] opus_pvq_search: Restore the proper use of conditional define and simplify the function name suffix handling

[Rostislav Pehlivanov]

On 19 August 2017 at 16:10, Ivan Kalvachev <ikalvachev at gmail.com> wrote:

> Using named define properly documents the code paths.
> It also avoids passing additional numbered arguments through
> multiple levels of macro templates.
>
> The suffix handling is done by concatenation, like in
> other asm functions and avoid having two separate
> "cglobal" defines.
>
> ---
> I have to point few things out.
>
> commit f386dd70acdc81d42d6bcb885d2889634cdf45b7
> "opus_pvq_search: only use rsqrtps approximation on CPUs with avx"
> was rushed hack job:
>

Well, your original patch was unpolished, so there's that.


>
> 3. The changes were numerous and intrusive, obfuscating code,
> while the same result could have been achieved
> with a single new line.
>
>
I disagree. Its completely clear and unobfuscated.


> I did request revert of this commit on irc and in ffmpeg-cvs-log mail,
> I did request a patch to be sent to ffmpeg-dev-eng, to discuss the
> best way to make the change.
>
> I did oppose the follow up commit 3c99523a2864af729a8576c3fffe81
> fb884fa0d5,
> that tried to fix the compilation, by redoing the intrusive changes once
> again,
> and making new changes that affect the C code too. (Thus making revert
> a lot more messy.)
>
> I find it extremely offensive that I as author of the code and FFmpeg
> developer
> was totally ignored, without providing any technical reasons and with
> something that sums up to "I like my code more".
>
> This breaks good faith, COC and the written rules.
>
>
As the maintainer of the encoder I have the last word on what goes on. I
could have instead amended your original patch but that would probably have
been even more offensive, hence I chose to have separate patches to fix the
problems.

On the other hand, the patch you've included is nothing more than a case of
"I like my code more". All you do is you add back a macro which doesn't
(and did not in the first place) even follow the code standard. It has
indentation, and it shouldn't.

The only way I'm going to accept this patch is if you send a V2 of the
patch which correctly removes the ident on the macro. And does away with
your pointless protests.