| Message ID | 20170801150137.8844-1-mfcc64@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01.08.2017 17:01, Muhammad Faiz wrote: > Fix Ticket6519. > > Signed-off-by: Muhammad Faiz <mfcc64@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? Regards, Tobias
On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 01.08.2017 17:01, Muhammad Faiz wrote: >> >> Fix Ticket6519. >> >> Signed-off-by: Muhammad Faiz <mfcc64@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? Changed locally with av_mallocz_array. Thank's.
On 02.08.2017 12:31, Muhammad Faiz wrote: > On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp <t.rapp@noa-archive.com> wrote: >> On 01.08.2017 17:01, Muhammad Faiz wrote: >>> >>> Fix Ticket6519. >>> >>> Signed-off-by: Muhammad Faiz <mfcc64@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? > > Changed locally with av_mallocz_array. LGTM then. Thanks for the fix. Regards, Tobias
Hi, On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 01.08.2017 17:01, Muhammad Faiz wrote: > >> Fix Ticket6519. >> >> Signed-off-by: Muhammad Faiz <mfcc64@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? Ronald
On 02.08.2017 14:04, Ronald S. Bultje wrote: > Hi, > > On Wed, Aug 2, 2017 at 3:10 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > >> On 01.08.2017 17:01, Muhammad Faiz wrote: >> >>> Fix Ticket6519. >>> >>> Signed-off-by: Muhammad Faiz <mfcc64@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. Regards, Tobias
Hi, On Wed, Aug 2, 2017 at 8:45 AM, Tobias Rapp <t.rapp@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@noa-archive.com> >> wrote: >> >>> On 01.08.2017 17:01, Muhammad Faiz wrote: >>> >>>> Fix Ticket6519. >>>> >>>> Signed-off-by: Muhammad Faiz <mfcc64@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. Ronald
On Wed, Aug 2, 2017 at 6:48 PM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > On 02.08.2017 12:31, Muhammad Faiz wrote: >> >> On Wed, Aug 2, 2017 at 2:10 PM, Tobias Rapp <t.rapp@noa-archive.com> >> wrote: >>> >>> On 01.08.2017 17:01, Muhammad Faiz wrote: >>>> >>>> >>>> Fix Ticket6519. >>>> >>>> Signed-off-by: Muhammad Faiz <mfcc64@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? >> >> >> 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.
On Wed, Aug 2, 2017 at 8:57 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Wed, Aug 2, 2017 at 8:45 AM, Tobias Rapp <t.rapp@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@noa-archive.com> >>> wrote: >>> >>>> On 01.08.2017 17:01, Muhammad Faiz wrote: >>>> >>>>> Fix Ticket6519. >>>>> >>>>> Signed-off-by: Muhammad Faiz <mfcc64@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
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;
Fix Ticket6519. Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> --- libavfilter/vf_ssim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)