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

[Ganesh Ajjanagadde]

On Thu, Oct 29, 2015 at 8:13 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> 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.

Sometimes it is not possible to convince a reviewer, even when the
evidence has been laid out and is right in front of them. I still
recall your concerns with fabs/FFABS, where you simply did not look at
my random number benchmark, or Clement's asm, or the libc link. Even
if you did, your email did not reflect that. Furthermore, you did not
post saying that your objection had been withdrawn. I can quote
relevant links here, but doubt it is necessary. How can I guess your
mind? What do you propose me to do then?

Please note that this is not with respect to the patch series above,
but is a general comment.

> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel