Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)

[Michael Niedermayer]

On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
> This is likely more precise and conveys the intent better.
> 
> Signed-off-by: Ganesh Ajjanagadde 
> ---
>  libavfilter/vf_ssim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
> index ce1e3db..6b2a8d7 100644
> --- a/libavfilter/vf_ssim.c
> +++ b/libavfilter/vf_ssim.c
> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>  
>  static double ssim_db(double ssim, double weight)
>  {
> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
> +return 10 * (log10(weight) - log10(weight - ssim));

LGTM

note, this can be simplified further but thats maybe off topic in
relation to switching to log10

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)

[Ganesh Ajjanagadde]

On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
 wrote:
> On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>> This is likely more precise and conveys the intent better.
>>
>> Signed-off-by: Ganesh Ajjanagadde 
>> ---
>>  libavfilter/vf_ssim.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>> index ce1e3db..6b2a8d7 100644
>> --- a/libavfilter/vf_ssim.c
>> +++ b/libavfilter/vf_ssim.c
>> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>>
>>  static double ssim_db(double ssim, double weight)
>>  {
>> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>> +return 10 * (log10(weight) - log10(weight - ssim));
>
> LGTM
>
> note, this can be simplified further but thats maybe off topic in
> relation to switching to log10

I did note that you can rewrite as log10(weight/(weight-ssim)), but
avoided it deliberately as I did not know what people want with
respect to it. Since you brought it up and think it may be good, I
will change it.

I personally don't consider it too off topic, and prefer this over
sending a separate patch for the simplification and dealing with
another wm4 rant about it.
Will push all later, once everything is reviewed and ok'ed.

>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> You can kill me, but you cannot change the truth.
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)

[Ganesh Ajjanagadde]

On Thu, Oct 29, 2015 at 2:33 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde 
> wrote:
>
>> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
>>  wrote:
>> > On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>> >> This is likely more precise and conveys the intent better.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde 
>> >> ---
>> >>  libavfilter/vf_ssim.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>> >> index ce1e3db..6b2a8d7 100644
>> >> --- a/libavfilter/vf_ssim.c
>> >> +++ b/libavfilter/vf_ssim.c
>> >> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>> >>
>> >>  static double ssim_db(double ssim, double weight)
>> >>  {
>> >> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>> >> +return 10 * (log10(weight) - log10(weight - ssim));
>> >
>> > LGTM
>> >
>> > note, this can be simplified further but thats maybe off topic in
>> > relation to switching to log10
>>
>> I did note that you can rewrite as log10(weight/(weight-ssim)), but
>> avoided it deliberately as I did not know what people want with
>> respect to it. Since you brought it up and think it may be good, I
>> will change it.
>>
>> I personally don't consider it too off topic, and prefer this over
>> sending a separate patch for the simplification and dealing with
>> another wm4 rant about it.
>> Will push all later, once everything is reviewed and ok'ed.
>
>
> Now, now, let's stay friendly and professional...

I referred to it as a "rant" because that is what it was, and I want
to make it sufficiently clear that the reason patches I send often
result in a ton of noise is often not technical, but simply a
knee-jerk reaction and associated bickering over it.

I do not enjoy dealing with such things, but I can almost surely
guarantee that is what would happen if I send a separate patch for it.
I (and most people here) want work to be done at minimal noise cost,
and hence I gave my rationale for changing without sending a separate
patch.

Your point is taken though, and I will attempt to refrain from such things.

>
> Ronald
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)

[Ronald S. Bultje]

Hi,

On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde 
wrote:

> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
>  wrote:
> > On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
> >> This is likely more precise and conveys the intent better.
> >>
> >> Signed-off-by: Ganesh Ajjanagadde 
> >> ---
> >>  libavfilter/vf_ssim.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
> >> index ce1e3db..6b2a8d7 100644
> >> --- a/libavfilter/vf_ssim.c
> >> +++ b/libavfilter/vf_ssim.c
> >> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
> >>
> >>  static double ssim_db(double ssim, double weight)
> >>  {
> >> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
> >> +return 10 * (log10(weight) - log10(weight - ssim));
> >
> > LGTM
> >
> > note, this can be simplified further but thats maybe off topic in
> > relation to switching to log10
>
> I did note that you can rewrite as log10(weight/(weight-ssim)), but
> avoided it deliberately as I did not know what people want with
> respect to it. Since you brought it up and think it may be good, I
> will change it.
>
> I personally don't consider it too off topic, and prefer this over
> sending a separate patch for the simplification and dealing with
> another wm4 rant about it.
> Will push all later, once everything is reviewed and ok'ed.


Now, now, let's stay friendly and professional...

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 10/11] avfilter/vf_ssim: use log10 instead of log()/log(10)

[Ganesh Ajjanagadde]

On Thu, Oct 29, 2015 at 2:07 PM, Ganesh Ajjanagadde  wrote:
> On Thu, Oct 29, 2015 at 1:37 PM, Michael Niedermayer
>  wrote:
>> On Thu, Oct 29, 2015 at 12:20:08AM -0400, Ganesh Ajjanagadde wrote:
>>> This is likely more precise and conveys the intent better.
>>>
>>> Signed-off-by: Ganesh Ajjanagadde 
>>> ---
>>>  libavfilter/vf_ssim.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
>>> index ce1e3db..6b2a8d7 100644
>>> --- a/libavfilter/vf_ssim.c
>>> +++ b/libavfilter/vf_ssim.c
>>> @@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
>>>
>>>  static double ssim_db(double ssim, double weight)
>>>  {
>>> -return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
>>> +return 10 * (log10(weight) - log10(weight - ssim));
>>
>> LGTM
>>
>> note, this can be simplified further but thats maybe off topic in
>> relation to switching to log10
>
> I did note that you can rewrite as log10(weight/(weight-ssim)), but
> avoided it deliberately as I did not know what people want with
> respect to it. Since you brought it up and think it may be good, I
> will change it.
>
> I personally don't consider it too off topic, and prefer this over
> sending a separate patch for the simplification and dealing with
> another wm4 rant about it.
> Will push all later, once everything is reviewed and ok'ed.

pushed with the further simplification, thanks.

>
>>
>> [...]
>> --
>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> You can kill me, but you cannot change the truth.
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ganesh Ajjanagadde]

This is likely more precise and conveys the intent better.

Signed-off-by: Ganesh Ajjanagadde 
---
 libavfilter/vf_ssim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavfilter/vf_ssim.c b/libavfilter/vf_ssim.c
index ce1e3db..6b2a8d7 100644
--- a/libavfilter/vf_ssim.c
+++ b/libavfilter/vf_ssim.c
@@ -176,7 +176,7 @@ static float ssim_plane(SSIMDSPContext *dsp,
 
 static double ssim_db(double ssim, double weight)
 {
-return 10 * (log(weight) / log(10) - log(weight - ssim) / log(10));
+return 10 * (log10(weight) - log10(weight - ssim));
 }
 
 static AVFrame *do_ssim(AVFilterContext *ctx, AVFrame *main,
-- 
2.6.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel