[FFmpeg-devel] [PATCH] lavf/vf_freezedetect: improve for the freeze frame detection

[Marton Balint]

On Sun, 14 Jul 2019, Limin Wang wrote:

> On Sat, Jul 13, 2019 at 07:24:59PM +0200, Marton Balint wrote:
>> 
>> 
>> On Sat, 13 Jul 2019, lance.lmwang at gmail.com wrote:
>> 
>> >From: Limin Wang <lance.lmwang at gmail.com>
>> >
>> >I have samples failed to detect the freeze frame with the default -60dB
>> >noise(-40dB is OK to detect),
>> >after apply the patch, it's ok to detect.
>> >
>> >I run the testing with fate-suite sample for your testing:
>> >old: no freeze frame detect.
>> >
>> >with the patch:
>> >./ffmpeg -i fate-suite/svq3/Vertical400kbit.sorenson3.mov -vf
>> >freezedetect=n=-60dB -an -f null -
>> >[freezedetect @ 0x7fe18a604900] lavfi.freezedetect.freeze_start: 38.7667
>> >[freezedetect @ 0x7fe18a604900] lavfi.freezedetect.freeze_duration: 2.5
>> >[freezedetect @ 0x7fe18a604900] lavfi.freezedetect.freeze_end: 41.2667
>> >[freezedetect @ 0x7fe18a604900] lavfi.freezedetect.freeze_start: 41.2667
>> >
>> >Have run make fate testing although haven't find any freezedetect checking for
>> >the fate.
>> 
>> You are changing the way the score is calculated and not describing
>> why. Of course you will get different results.
>> 
>> >
>> >Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
>> >---
>> >libavfilter/vf_freezedetect.c | 22 ++++++++++++++++------
>> >1 file changed, 16 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/libavfilter/vf_freezedetect.c b/libavfilter/vf_freezedetect.c
>> >index cc086af..0288fb0 100644
>> >--- a/libavfilter/vf_freezedetect.c
>> >+++ b/libavfilter/vf_freezedetect.c
>> >@@ -38,7 +38,9 @@ typedef struct FreezeDetectContext {
>> >    ptrdiff_t height[4];
>> >    ff_scene_sad_fn sad;
>> >    int bitdepth;
>> >+    int nb_planes;
>> >    AVFrame *reference_frame;
>> >+    double prev_mafd;
>> >    int64_t n;
>> >    int64_t reference_n;
>> >    int frozen;
>> >@@ -102,13 +104,15 @@ static int config_input(AVFilterLink *inlink)
>> >    AVFilterContext *ctx = inlink->dst;
>> >    FreezeDetectContext *s = ctx->priv;
>> >    const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(inlink->format);
>> >+    int vsub = pix_desc->log2_chroma_h;
>> >
>> >    s->bitdepth = pix_desc->comp[0].depth;
>> >+    s->nb_planes = av_pix_fmt_count_planes(inlink->format);
>> 
>> This is not needed, for invalid planes we simply get 0 for linesize
>> and width.
>
> It's more general to get nb_planes instead of checking the linesize
> result. If have one plane, it's unneed to check 4 times.

Still looks a purely cosmetic change to me.

>
>> 
>> >
>> >-    for (int plane = 0; plane < 4; plane++) {
>> >+    for (int plane = 0; plane < s->nb_planes; plane++) {
>> >        ptrdiff_t line_size = av_image_get_linesize(inlink->format, inlink->w, plane);
>> >-        s->width[plane] = line_size >> (s->bitdepth > 8);
>> >-        s->height[plane] = inlink->h >> ((plane == 1 || plane == 2) ? pix_desc->log2_chroma_h : 0);
>> >+        s->width[plane] = line_size;
>> 
>> Why? Width is the width of the plane in pixels, simply setting
>> linesize does not seem right.
>
> the sad init function have set the bitdepth, so I think it's OK to set
> the width in bytes. Maybe it's my misunderstand for the code.

Later on width*height is used to determine the number of pixels for which 
we have counted the sum of differences.

>
>> 
>> >+        s->height[plane] = plane == 1 || plane == 2 ? AV_CEIL_RSHIFT(inlink->h, vsub) : inlink->h;
>> 
>> Mix of a cosmetic and functional change. This should do the same
>> with existing code and variables:
>> s->height[plane] = AV_CEIL_RSHIFT(inlink->h, ((plane == 1 || plane == 2) ? pix_desc->log2_chroma_h : 0)
>> 
>> >    }
>> >
>> >    s->sad = ff_scene_sad_get_fn(s->bitdepth == 8 ? 8 : 16);
>> >@@ -129,7 +133,9 @@ static int is_frozen(FreezeDetectContext *s, AVFrame *reference, AVFrame *frame)
>> >    uint64_t sad = 0;
>> >    uint64_t count = 0;
>> >    double mafd;
>> >-    for (int plane = 0; plane < 4; plane++) {
>> >+    double diff, cmp;
>> >+
>> >+    for (int plane = 0; plane < s->nb_planes; plane++) {
>> 
>> Unneeded.
>> 
>> >        if (s->width[plane]) {
>> >            uint64_t plane_sad;
>> >            s->sad(frame->data[plane], frame->linesize[plane],
>> >@@ -140,8 +146,12 @@ static int is_frozen(FreezeDetectContext *s, AVFrame *reference, AVFrame *frame)
>> >        }
>> >    }
>> >    emms_c();
>> >-    mafd = (double)sad / count / (1ULL << s->bitdepth);
>> >-    return (mafd <= s->noise);
>> >+    mafd = (double)sad /(count >> (s->bitdepth > 8));
>> 
>> Why? MAFD should be the mean difference normalized to [0..1].
> if the bitdeth is 16, it'll divide by 1UL << 16, it'll cause the mafd is
> very small. So I choose the scenecut way in the below.

The metric supposed to be indepentent of the bit depth. If you get 
different mafd for the same source expressed in 8bit, 10bit or 16bit pixel 
format (provided that you use the same subsampling), then you are doing 
something wrong.

And you can't reinvent MAFD according to your needs. It is the mean 
absolute difference of the whole image normalized to 0..1 in this case. If 
you are not calculating that, then it is not MAFD.

>
>> 
>> >+    diff = fabs(mafd - s->prev_mafd);
>> >+    cmp  = av_clipf(FFMIN(mafd, diff) / 100., 0, 1);
>> >+    s->prev_mafd = mafd;
>> 
>> Why? This is not scene change detection, MAFD change is not useful
>> for us. E.g. two frames alternating will be detected as frozen. Also
>> note that not always consecutive frames are compared, so involving
>> MAFD change seems even more bogus to me.
> Isn't possible to add one standard sampel into fate, it's useful to
> test the function. We can choose different noise setting to get the same
> result, so it's difficult to say whether it's covered.

Good idea, although one of the existing samples (maybe the one you tested 
with already) most probably should work as well. Fate would be the perfect 
place to test different bit depths and make sure you get the same results. 
Noise floor should be set up in a way so that changing it slightly make a 
real difference in detection, so score changes in code can be detected.

Regards,
Marton