Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Muhammad Faiz]

On Wed, Aug 2, 2017 at 8:57 PM, Ronald S. Bultje  wrote:
> Hi,
>
> On Wed, Aug 2, 2017 at 8:45 AM, Tobias Rapp  wrote:
>
>> On 02.08.2017 14:04, Ronald S. Bultje wrote:
>>
>>> On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp 
>>> wrote:
>>>
 On 01.08.2017 17:01, Muhammad Faiz wrote:

> Fix Ticket6519.
>
> Signed-off-by: Muhammad Faiz 
> ---
>  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 c3c204268f..dfd276e015 100644
> --- a/libavfilter/vf_ssim.c
> +++ b/libavfilter/vf_ssim.c
> @@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
>  for (i = 0; i < s->nb_components; i++)
>  s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
> sum;
>
> -s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) *
> (1 + (desc->comp[0].depth > 8)));
> +s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
> sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
>  if (!s->temp)
>  return AVERROR(ENOMEM);
>  s->max = (1 << desc->comp[0].depth) - 1;
>
>
> I confirm that the command doesn't crash anymore and the reports of
 "invalid read/write" in Valgrind are gone. However there are still some
 "use of uninitialized value" reports remaining. Maybe use
 av_mallocz_array?

>>>
>>>
>>> Wait wait, before we do that, which values are uninitialized and what are
>>> they used for?
>>>
>>
>> Reads into s->temp seem to use uninitialized values on x86-64 when
>> vf_ssim.asm routines are used (-cpuflags all), see attached Valgrind
>> report. When vf_ssim.asm is not used (-cpuflags 0) no "use of uninitialized
>> value" is reported.
>>
>> The difference of the reported SSIM scores between my asm and non-asm test
>> runs was <= 10^-6.
>
>
> Which function is causing the warnings? Isit dsp->ssim_4x4_line() or
> dsp->ssim_end_line()? (My gut feeling tells me it's ssim_4x4_line(), but I
> don't understand why, since the result should be unused in
> ssim_end_line().) It's really important to understand the source and
> implication of these warnings before we silence them - they may be causing
> actual inaccuracies in the result, which we don't want.

Because ssim_4x4_line never reads sums, I think the warnings are
caused by ssim_end_line.

Looking at the asm code:
ssim_4x4_line writes to sums 32-bytes per loop, while ssim_end_line
reads sum0/sum1 64-bytes per loop + 16-bytes overread

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Muhammad Faiz]

On Wed, Aug 2, 2017 at 6:48 PM, Tobias Rapp  wrote:
> On 02.08.2017 12:31, Muhammad Faiz wrote:
>>
>> On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp 
>> wrote:
>>>
>>> On 01.08.2017 17:01, Muhammad Faiz wrote:


 Fix Ticket6519.

 Signed-off-by: Muhammad Faiz 
 ---
  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 c3c204268f..dfd276e015 100644
 --- a/libavfilter/vf_ssim.c
 +++ b/libavfilter/vf_ssim.c
 @@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
  for (i = 0; i < s->nb_components; i++)
  s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
 sum;

 -s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) *
 (1
 + (desc->comp[0].depth > 8)));
 +s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
 sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
  if (!s->temp)
  return AVERROR(ENOMEM);
  s->max = (1 << desc->comp[0].depth) - 1;

>>>
>>> I confirm that the command doesn't crash anymore and the reports of
>>> "invalid
>>> read/write" in Valgrind are gone. However there are still some "use of
>>> uninitialized value" reports remaining. Maybe use av_mallocz_array?
>>
>>
>> Changed locally with av_mallocz_array.
>
>
> LGTM then. Thanks for the fix.

Seems that this doesn't fix all cases. It fails with width=344.
Will post new patch.

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Ronald S. Bultje]

Hi,

On Wed, Aug 2, 2017 at 8:45 AM, Tobias Rapp  wrote:

> On 02.08.2017 14:04, Ronald S. Bultje wrote:
>
>> On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp 
>> wrote:
>>
>>> On 01.08.2017 17:01, Muhammad Faiz wrote:
>>>
 Fix Ticket6519.

 Signed-off-by: Muhammad Faiz 
 ---
  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 c3c204268f..dfd276e015 100644
 --- a/libavfilter/vf_ssim.c
 +++ b/libavfilter/vf_ssim.c
 @@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
  for (i = 0; i < s->nb_components; i++)
  s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
 sum;

 -s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) *
 (1 + (desc->comp[0].depth > 8)));
 +s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
 sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
  if (!s->temp)
  return AVERROR(ENOMEM);
  s->max = (1 << desc->comp[0].depth) - 1;


 I confirm that the command doesn't crash anymore and the reports of
>>> "invalid read/write" in Valgrind are gone. However there are still some
>>> "use of uninitialized value" reports remaining. Maybe use
>>> av_mallocz_array?
>>>
>>
>>
>> Wait wait, before we do that, which values are uninitialized and what are
>> they used for?
>>
>
> Reads into s->temp seem to use uninitialized values on x86-64 when
> vf_ssim.asm routines are used (-cpuflags all), see attached Valgrind
> report. When vf_ssim.asm is not used (-cpuflags 0) no "use of uninitialized
> value" is reported.
>
> The difference of the reported SSIM scores between my asm and non-asm test
> runs was <= 10^-6.


Which function is causing the warnings? Isit dsp->ssim_4x4_line() or
dsp->ssim_end_line()? (My gut feeling tells me it's ssim_4x4_line(), but I
don't understand why, since the result should be unused in
ssim_end_line().) It's really important to understand the source and
implication of these warnings before we silence them - they may be causing
actual inaccuracies in the result, which we don't want.

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Tobias Rapp]

On 02.08.2017 14:04, Ronald S. Bultje wrote:

Hi,

On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp  wrote:


On 01.08.2017 17:01, Muhammad Faiz wrote:


Fix Ticket6519.

Signed-off-by: Muhammad Faiz 
---
 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 c3c204268f..dfd276e015 100644
--- a/libavfilter/vf_ssim.c
+++ b/libavfilter/vf_ssim.c
@@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
 for (i = 0; i < s->nb_components; i++)
 s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
sum;

-s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) *
(1 + (desc->comp[0].depth > 8)));
+s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
 if (!s->temp)
 return AVERROR(ENOMEM);
 s->max = (1 << desc->comp[0].depth) - 1;



I confirm that the command doesn't crash anymore and the reports of
"invalid read/write" in Valgrind are gone. However there are still some
"use of uninitialized value" reports remaining. Maybe use av_mallocz_array?



Wait wait, before we do that, which values are uninitialized and what are
they used for?


Reads into s->temp seem to use uninitialized values on x86-64 when 
vf_ssim.asm routines are used (-cpuflags all), see attached Valgrind 
report. When vf_ssim.asm is not used (-cpuflags 0) no "use of 
uninitialized value" is reported.


The difference of the reported SSIM scores between my asm and non-asm 
test runs was <= 10^-6.


Regards,
Tobias


valgrind-fate-ssim-rgb_7c04be7b.log.gz
Description: application/gzip
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Ronald S. Bultje]

Hi,

On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp  wrote:

> On 01.08.2017 17:01, Muhammad Faiz wrote:
>
>> Fix Ticket6519.
>>
>> Signed-off-by: Muhammad Faiz 
>> ---
>>  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 c3c204268f..dfd276e015 100644
>> --- a/libavfilter/vf_ssim.c
>> +++ b/libavfilter/vf_ssim.c
>> @@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
>>  for (i = 0; i < s->nb_components; i++)
>>  s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
>> sum;
>>
>> -s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) *
>> (1 + (desc->comp[0].depth > 8)));
>> +s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
>> sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
>>  if (!s->temp)
>>  return AVERROR(ENOMEM);
>>  s->max = (1 << desc->comp[0].depth) - 1;
>>
>>
> I confirm that the command doesn't crash anymore and the reports of
> "invalid read/write" in Valgrind are gone. However there are still some
> "use of uninitialized value" reports remaining. Maybe use av_mallocz_array?


Wait wait, before we do that, which values are uninitialized and what are
they used for?

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Tobias Rapp]

On 02.08.2017 12:31, Muhammad Faiz wrote:

On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp  wrote:

On 01.08.2017 17:01, Muhammad Faiz wrote:


Fix Ticket6519.

Signed-off-by: Muhammad Faiz 
---
 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 c3c204268f..dfd276e015 100644
--- a/libavfilter/vf_ssim.c
+++ b/libavfilter/vf_ssim.c
@@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
 for (i = 0; i < s->nb_components; i++)
 s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
sum;

-s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) * (1
+ (desc->comp[0].depth > 8)));
+s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
 if (!s->temp)
 return AVERROR(ENOMEM);
 s->max = (1 << desc->comp[0].depth) - 1;



I confirm that the command doesn't crash anymore and the reports of "invalid
read/write" in Valgrind are gone. However there are still some "use of
uninitialized value" reports remaining. Maybe use av_mallocz_array?


Changed locally with av_mallocz_array.


LGTM then. Thanks for the fix.

Regards,
Tobias

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_ssim: align temp size

[Muhammad Faiz]

On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp  wrote:
> On 01.08.2017 17:01, Muhammad Faiz wrote:
>>
>> Fix Ticket6519.
>>
>> Signed-off-by: Muhammad Faiz 
>> ---
>>  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 c3c204268f..dfd276e015 100644
>> --- a/libavfilter/vf_ssim.c
>> +++ b/libavfilter/vf_ssim.c
>> @@ -402,7 +402,7 @@ static int config_input_ref(AVFilterLink *inlink)
>>  for (i = 0; i < s->nb_components; i++)
>>  s->coefs[i] = (double) s->planeheight[i] * s->planewidth[i] /
>> sum;
>>
>> -s->temp = av_malloc_array((2 * inlink->w + 12), sizeof(*s->temp) * (1
>> + (desc->comp[0].depth > 8)));
>> +s->temp = av_malloc_array(FFALIGN(2 * inlink->w + 12, 64),
>> sizeof(*s->temp) * (1 + (desc->comp[0].depth > 8)));
>>  if (!s->temp)
>>  return AVERROR(ENOMEM);
>>  s->max = (1 << desc->comp[0].depth) - 1;
>>
>
> I confirm that the command doesn't crash anymore and the reports of "invalid
> read/write" in Valgrind are gone. However there are still some "use of
> uninitialized value" reports remaining. Maybe use av_mallocz_array?

Changed locally with av_mallocz_array.

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