[FFmpeg-devel] [PATCH 01/11] ffmpeg: use log10 instead of log()/log(10)

[Hendrik Leppkes]

On Fri, Oct 30, 2015 at 1:05 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Thu, Oct 29, 2015 at 7:50 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> On Fri, Oct 30, 2015 at 12:08 AM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>>> On Thu, Oct 29, 2015 at 8:27 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> On Thu, Oct 29, 2015 at 8:04 AM, Michael Niedermayer <michael at niedermayer.cc
>>>>> wrote:
>>>>
>>>>> On Thu, Oct 29, 2015 at 12:19:59AM -0400, Ganesh Ajjanagadde wrote:
>>>>> > This is more concise and conveys the intent better.
>>>>> > Furthermore, it is likely more precise as well due to lack of floating
>>>>> > point division.
>>>>> >
>>>>> > Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>>>>
>>>>> patchset tested on linux32, inux64, mingw32, ming64, qemu-mips and
>>>>> qemu-arm
>>>>> fate passes on all, i could test on actual arm&mips hw if people think
>>>>> that is needed
>>>>
>>>>
>>>> I don't think that's needed.
>>>>
>>>> Is there some way we can confirm that each of these files that is changed
>>>> includes libavutil/libm.h for the compatibility macros in case they're
>>>> lacking on the target system?
>>>
>>> pushed as is; we have until the next release or a FATE failure to
>>> check the build on such obscure configurations. I will try to keep an
>>> eye out, but please report if you find failures.
>>>
>>
>> This is really not the right way to go. You have been notified of a
>> real-world portability concern with an easy fix (verify that all files
>> you use log2/etc in include libavutil/libm.h), and you decided to wait
>> until it blows up instead?
>> Not cool.
>
> Ok. Do a grep on ffmpeg.c: line 56 answers your point.
>
> Other patches have been acked by Michael, and were pushed.
>
> More generally, how is this problem "easy to verify"? It may be
> included indirectly, etc. Since you seem to think it is easy, go ahead
> and do whatever you want. Or enlighten mortals like me who fail to see
> it is easy with your wisdom. Or if it is "easy", why don't you do it
> for the commits that I just pushed.
>
> If FFmpeg waited until verification on every single config was done,
> we would be nowhere. You may think it is not cool, I could say the
> same about many things you have posted on this mailing list as well.
>
> If Michael thought this was not cool, I will immediately take action.
>

Its not about this patch especially, but all in the series, to which
Ronald's concerns apply equally.
If you get a review which voices concerns, these concerns need to be
addressed. Either by fixing the patch, or by convincing the reviewer
that no action is needed. And one positive review does not overrule a
negative one - not everyone knows all possible concerns equally well.

Pushing without addressing concerns is not OK at all.