diff mbox

[FFmpeg-devel,v4,2/4] avfilter/vf_framerate: if metadata lavfi.scd.mafd exists, we'll use it first

Message ID 20191008152839.30375-2-lance.lmwang@gmail.com
State New
Headers show

Commit Message

Lance Wang Oct. 8, 2019, 3:28 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Reviewed-by: Paul B Mahol <onemda@gmail.com>
Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavfilter/vf_framerate.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Michael Niedermayer Oct. 10, 2019, 11:52 a.m. UTC | #1 
On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Reviewed-by: Paul B Mahol <onemda@gmail.com>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavfilter/vf_framerate.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> index 6c8d01c..8d16998 100644
> --- a/libavfilter/vf_framerate.c
> +++ b/libavfilter/vf_framerate.c
> @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
>  
>      if (crnt->height == next->height &&
>          crnt->width  == next->width) {
> +        AVDictionaryEntry *e_mafd = NULL;
>          uint64_t sad;
> -        double mafd, diff;
> +        double mafd = HUGE_VAL, diff;
> +        char *tail = NULL;
>  
>          ff_dlog(ctx, "get_scene_score() process\n");
> -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> -        emms_c();
> -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> +        if (e_mafd)
> +            mafd = strtod(e_mafd->value, &tail);
> +        if (*tail || mafd == HUGE_VAL) {
> +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> +            emms_c();
> +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> +        }

while this is not introduced by the patch, i would suggest to seperate
emms_c() from functions using floating point, This combination does have
some potential for emms and the compilers optimizer hitting each other

thx


[...]
Lance Wang Oct. 10, 2019, 2:11 p.m. UTC | #2 
On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavfilter/vf_framerate.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > index 6c8d01c..8d16998 100644
> > --- a/libavfilter/vf_framerate.c
> > +++ b/libavfilter/vf_framerate.c
> > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> >  
> >      if (crnt->height == next->height &&
> >          crnt->width  == next->width) {
> > +        AVDictionaryEntry *e_mafd = NULL;
> >          uint64_t sad;
> > -        double mafd, diff;
> > +        double mafd = HUGE_VAL, diff;
> > +        char *tail = NULL;
> >  
> >          ff_dlog(ctx, "get_scene_score() process\n");
> > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > -        emms_c();
> > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > +        if (e_mafd)
> > +            mafd = strtod(e_mafd->value, &tail);
> > +        if (*tail || mafd == HUGE_VAL) {
> > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > +            emms_c();
> > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > +        }
> 
> while this is not introduced by the patch, i would suggest to seperate
> emms_c() from functions using floating point, This combination does have
> some potential for emms and the compilers optimizer hitting each other

Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
existing codebase? I can help to submit patch to fix it, there're at least five filters
are using the same way.

> 
> thx
> 
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Old school: Use the lowest level language in which you can solve the problem
>             conveniently.
> New school: Use the highest level language in which the latest supercomputer
>             can solve the problem without the user falling asleep waiting.



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Oct. 11, 2019, 6:13 p.m. UTC | #3 
On Thu, Oct 10, 2019 at 10:11:10PM +0800, Limin Wang wrote:
> On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> > On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavfilter/vf_framerate.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > > index 6c8d01c..8d16998 100644
> > > --- a/libavfilter/vf_framerate.c
> > > +++ b/libavfilter/vf_framerate.c
> > > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> > >  
> > >      if (crnt->height == next->height &&
> > >          crnt->width  == next->width) {
> > > +        AVDictionaryEntry *e_mafd = NULL;
> > >          uint64_t sad;
> > > -        double mafd, diff;
> > > +        double mafd = HUGE_VAL, diff;
> > > +        char *tail = NULL;
> > >  
> > >          ff_dlog(ctx, "get_scene_score() process\n");
> > > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > -        emms_c();
> > > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > > +        if (e_mafd)
> > > +            mafd = strtod(e_mafd->value, &tail);
> > > +        if (*tail || mafd == HUGE_VAL) {
> > > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > +            emms_c();
> > > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > +        }
> > 
> > while this is not introduced by the patch, i would suggest to seperate
> > emms_c() from functions using floating point, This combination does have
> > some potential for emms and the compilers optimizer hitting each other
> 
> Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
> existing codebase? I can help to submit patch to fix it, there're at least five filters
> are using the same way.

emms clears the float/MMX state of the cpu
if the compiler uses the FPU at the same time and is unaware of that
clearing it could result in problems.
so i just always tried to keep emms away from floating point usage to minimize 
the chances of this.

also it was rather slow on some CPUs to mix mmx and float code 

thx
[...]
Lance Wang Oct. 12, 2019, 1:39 a.m. UTC | #4 
On Fri, Oct 11, 2019 at 08:13:02PM +0200, Michael Niedermayer wrote:
> On Thu, Oct 10, 2019 at 10:11:10PM +0800, Limin Wang wrote:
> > On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> > > On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > 
> > > > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavfilter/vf_framerate.c | 15 +++++++++++----
> > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > > > index 6c8d01c..8d16998 100644
> > > > --- a/libavfilter/vf_framerate.c
> > > > +++ b/libavfilter/vf_framerate.c
> > > > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> > > >  
> > > >      if (crnt->height == next->height &&
> > > >          crnt->width  == next->width) {
> > > > +        AVDictionaryEntry *e_mafd = NULL;
> > > >          uint64_t sad;
> > > > -        double mafd, diff;
> > > > +        double mafd = HUGE_VAL, diff;
> > > > +        char *tail = NULL;
> > > >  
> > > >          ff_dlog(ctx, "get_scene_score() process\n");
> > > > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > -        emms_c();
> > > > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > > > +        if (e_mafd)
> > > > +            mafd = strtod(e_mafd->value, &tail);
> > > > +        if (*tail || mafd == HUGE_VAL) {
> > > > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > +            emms_c();
> > > > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > +        }
> > > 
> > > while this is not introduced by the patch, i would suggest to seperate
> > > emms_c() from functions using floating point, This combination does have
> > > some potential for emms and the compilers optimizer hitting each other
> > 
> > Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
> > existing codebase? I can help to submit patch to fix it, there're at least five filters
> > are using the same way.
> 
> emms clears the float/MMX state of the cpu
> if the compiler uses the FPU at the same time and is unaware of that
> clearing it could result in problems.
> so i just always tried to keep emms away from floating point usage to minimize 
> the chances of this.
> 
> also it was rather slow on some CPUs to mix mmx and float code 

OK, how about to scale mafd with 1000000 to avoid the float point use? 

> 
> thx
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> When you are offended at any man's fault, turn to yourself and study your
> own failings. Then you will forget your anger. -- Epictetus



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer Oct. 12, 2019, 9:41 p.m. UTC | #5 
On Sat, Oct 12, 2019 at 09:39:14AM +0800, Limin Wang wrote:
> On Fri, Oct 11, 2019 at 08:13:02PM +0200, Michael Niedermayer wrote:
> > On Thu, Oct 10, 2019 at 10:11:10PM +0800, Limin Wang wrote:
> > > On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> > > > On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > 
> > > > > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > ---
> > > > >  libavfilter/vf_framerate.c | 15 +++++++++++----
> > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > > > > index 6c8d01c..8d16998 100644
> > > > > --- a/libavfilter/vf_framerate.c
> > > > > +++ b/libavfilter/vf_framerate.c
> > > > > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> > > > >  
> > > > >      if (crnt->height == next->height &&
> > > > >          crnt->width  == next->width) {
> > > > > +        AVDictionaryEntry *e_mafd = NULL;
> > > > >          uint64_t sad;
> > > > > -        double mafd, diff;
> > > > > +        double mafd = HUGE_VAL, diff;
> > > > > +        char *tail = NULL;
> > > > >  
> > > > >          ff_dlog(ctx, "get_scene_score() process\n");
> > > > > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > -        emms_c();
> > > > > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > > > > +        if (e_mafd)
> > > > > +            mafd = strtod(e_mafd->value, &tail);
> > > > > +        if (*tail || mafd == HUGE_VAL) {
> > > > > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > +            emms_c();
> > > > > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > +        }
> > > > 
> > > > while this is not introduced by the patch, i would suggest to seperate
> > > > emms_c() from functions using floating point, This combination does have
> > > > some potential for emms and the compilers optimizer hitting each other
> > > 
> > > Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
> > > existing codebase? I can help to submit patch to fix it, there're at least five filters
> > > are using the same way.
> > 
> > emms clears the float/MMX state of the cpu
> > if the compiler uses the FPU at the same time and is unaware of that
> > clearing it could result in problems.
> > so i just always tried to keep emms away from floating point usage to minimize 
> > the chances of this.
> > 
> > also it was rather slow on some CPUs to mix mmx and float code 
> 
> OK, how about to scale mafd with 1000000 to avoid the float point use? 

For the use case of whole frame compare / scene detection, i dont see why
the function leaves the FPU state in need of a seperate EMMS
If this is never used with finer granularity then its probably best to
move the emms into the x86 specific code. 

Thanks
 
[...]
Lance Wang Oct. 12, 2019, 10:49 p.m. UTC | #6 
On Sat, Oct 12, 2019 at 11:41:53PM +0200, Michael Niedermayer wrote:
> On Sat, Oct 12, 2019 at 09:39:14AM +0800, Limin Wang wrote:
> > On Fri, Oct 11, 2019 at 08:13:02PM +0200, Michael Niedermayer wrote:
> > > On Thu, Oct 10, 2019 at 10:11:10PM +0800, Limin Wang wrote:
> > > > On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> > > > > On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > 
> > > > > > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > ---
> > > > > >  libavfilter/vf_framerate.c | 15 +++++++++++----
> > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > > > > > index 6c8d01c..8d16998 100644
> > > > > > --- a/libavfilter/vf_framerate.c
> > > > > > +++ b/libavfilter/vf_framerate.c
> > > > > > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> > > > > >  
> > > > > >      if (crnt->height == next->height &&
> > > > > >          crnt->width  == next->width) {
> > > > > > +        AVDictionaryEntry *e_mafd = NULL;
> > > > > >          uint64_t sad;
> > > > > > -        double mafd, diff;
> > > > > > +        double mafd = HUGE_VAL, diff;
> > > > > > +        char *tail = NULL;
> > > > > >  
> > > > > >          ff_dlog(ctx, "get_scene_score() process\n");
> > > > > > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > > -        emms_c();
> > > > > > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > > > > > +        if (e_mafd)
> > > > > > +            mafd = strtod(e_mafd->value, &tail);
> > > > > > +        if (*tail || mafd == HUGE_VAL) {
> > > > > > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > > +            emms_c();
> > > > > > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > > +        }
> > > > > 
> > > > > while this is not introduced by the patch, i would suggest to seperate
> > > > > emms_c() from functions using floating point, This combination does have
> > > > > some potential for emms and the compilers optimizer hitting each other
> > > > 
> > > > Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
> > > > existing codebase? I can help to submit patch to fix it, there're at least five filters
> > > > are using the same way.
> > > 
> > > emms clears the float/MMX state of the cpu
> > > if the compiler uses the FPU at the same time and is unaware of that
> > > clearing it could result in problems.
> > > so i just always tried to keep emms away from floating point usage to minimize 
> > > the chances of this.
> > > 
> > > also it was rather slow on some CPUs to mix mmx and float code 
> > 
> > OK, how about to scale mafd with 1000000 to avoid the float point use? 
> 
> For the use case of whole frame compare / scene detection, i dont see why
> the function leaves the FPU state in need of a seperate EMMS
> If this is never used with finer granularity then its probably best to
> move the emms into the x86 specific code. 

For I'm not familar with this related assemable code, maybe other expert developer 
can help change it later.

> 
> Thanks
>  
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Observe your enemies, for they first find out your faults. -- Antisthenes



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lance Wang Oct. 23, 2019, 10:22 a.m. UTC | #7 
On Sat, Oct 12, 2019 at 11:41:53PM +0200, Michael Niedermayer wrote:
> On Sat, Oct 12, 2019 at 09:39:14AM +0800, Limin Wang wrote:
> > On Fri, Oct 11, 2019 at 08:13:02PM +0200, Michael Niedermayer wrote:
> > > On Thu, Oct 10, 2019 at 10:11:10PM +0800, Limin Wang wrote:
> > > > On Thu, Oct 10, 2019 at 01:52:56PM +0200, Michael Niedermayer wrote:
> > > > > On Tue, Oct 08, 2019 at 11:28:37PM +0800, lance.lmwang@gmail.com wrote:
> > > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > 
> > > > > > Reviewed-by: Paul B Mahol <onemda@gmail.com>
> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > > ---
> > > > > >  libavfilter/vf_framerate.c | 15 +++++++++++----
> > > > > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
> > > > > > index 6c8d01c..8d16998 100644
> > > > > > --- a/libavfilter/vf_framerate.c
> > > > > > +++ b/libavfilter/vf_framerate.c
> > > > > > @@ -71,13 +71,20 @@ static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
> > > > > >  
> > > > > >      if (crnt->height == next->height &&
> > > > > >          crnt->width  == next->width) {
> > > > > > +        AVDictionaryEntry *e_mafd = NULL;
> > > > > >          uint64_t sad;
> > > > > > -        double mafd, diff;
> > > > > > +        double mafd = HUGE_VAL, diff;
> > > > > > +        char *tail = NULL;
> > > > > >  
> > > > > >          ff_dlog(ctx, "get_scene_score() process\n");
> > > > > > -        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > > -        emms_c();
> > > > > > -        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > > +        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
> > > > > > +        if (e_mafd)
> > > > > > +            mafd = strtod(e_mafd->value, &tail);
> > > > > > +        if (*tail || mafd == HUGE_VAL) {
> > > > > > +            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
> > > > > > +            emms_c();
> > > > > > +            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
> > > > > > +        }
> > > > > 
> > > > > while this is not introduced by the patch, i would suggest to seperate
> > > > > emms_c() from functions using floating point, This combination does have
> > > > > some potential for emms and the compilers optimizer hitting each other
> > > > 
> > > > Michael, I haven't clear about what to seperate the emms_c? any reference usage in 
> > > > existing codebase? I can help to submit patch to fix it, there're at least five filters
> > > > are using the same way.
> > > 
> > > emms clears the float/MMX state of the cpu
> > > if the compiler uses the FPU at the same time and is unaware of that
> > > clearing it could result in problems.
> > > so i just always tried to keep emms away from floating point usage to minimize 
> > > the chances of this.
> > > 
> > > also it was rather slow on some CPUs to mix mmx and float code 
> > 
> > OK, how about to scale mafd with 1000000 to avoid the float point use? 
> 
> For the use case of whole frame compare / scene detection, i dont see why
> the function leaves the FPU state in need of a seperate EMMS
> If this is never used with finer granularity then its probably best to
> move the emms into the x86 specific code. 

Michael, please give comments whether the sct filter can be merged. Your comment of the 
emms has exist in the current codebase already and not related to my patchset.


> 
> Thanks
>  
> [...]
> 
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> Observe your enemies, for they first find out your faults. -- Antisthenes



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox

Patch

diff --git a/libavfilter/vf_framerate.c b/libavfilter/vf_framerate.c
index 6c8d01c..8d16998 100644
--- a/libavfilter/vf_framerate.c
+++ b/libavfilter/vf_framerate.c
@@ -71,13 +71,20 @@  static double get_scene_score(AVFilterContext *ctx, AVFrame *crnt, AVFrame *next
 
     if (crnt->height == next->height &&
         crnt->width  == next->width) {
+        AVDictionaryEntry *e_mafd = NULL;
         uint64_t sad;
-        double mafd, diff;
+        double mafd = HUGE_VAL, diff;
+        char *tail = NULL;
 
         ff_dlog(ctx, "get_scene_score() process\n");
-        s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
-        emms_c();
-        mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
+        e_mafd = av_dict_get(next->metadata, "lavfi.scd.mafd", NULL, AV_DICT_MATCH_CASE);
+        if (e_mafd)
+            mafd = strtod(e_mafd->value, &tail);
+        if (*tail || mafd == HUGE_VAL) {
+            s->sad(crnt->data[0], crnt->linesize[0], next->data[0], next->linesize[0], crnt->width, crnt->height, &sad);
+            emms_c();
+            mafd = (double)sad * 100.0 / (crnt->width * crnt->height) / (1 << s->bitdepth);
+        }
         diff = fabs(mafd - s->prev_mafd);
         ret  = av_clipf(FFMIN(mafd, diff), 0, 100.0);
         s->prev_mafd = mafd;