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

[Muhammad Faiz]

On Wed, Aug 2, 2017 at 8:57 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Wed, Aug 2, 2017 at 8:45 AM, Tobias Rapp <t.rapp at noa-archive.com> wrote:
>
>> On 02.08.2017 14:04, Ronald S. Bultje wrote:
>>
>>> On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp <t.rapp at noa-archive.com>
>>> wrote:
>>>
>>>> On 01.08.2017 17:01, Muhammad Faiz wrote:
>>>>
>>>>> Fix Ticket6519.
>>>>>
>>>>> Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
>>>>> ---
>>>>>  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