[FFmpeg-devel] avformat/amr: reduce raw amr false positive detection

Am Mi., 2. Okt. 2019 um 20:19 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavformat/amr.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/libavformat/amr.c b/libavformat/amr.c
> index 42840a50a3..600cb1b0f0 100644
> --- a/libavformat/amr.c
> +++ b/libavformat/amr.c
> @@ -190,9 +190,12 @@ static int amrnb_probe(const AVProbeData *p)
>                  if (b[++i] != last)
>                      break;
>              }
> -            if (size > 0) {
> -                valid++;
> -                i += size;
> +            while (size--) {
> +                if (b[i])
> +                    valid++;
> +                else
> +                    invalid++;
> +                i++;
>              }
>          } else {
>              valid = 0;
> @@ -246,9 +249,12 @@ static int amrwb_probe(const AVProbeData *p)
>                  if (b[++i] != last)
>                      break;
>              }
> -            if (size > 0) {
> -                valid++;
> -                i += size;
> +            while (size--) {
> +                if (b[i])
> +                    valid++;
> +                else
> +                    invalid++;
> +                i++;
>              }
>          } else {
>              valid = 0;

The changes to amrwb are unneeded for the file you provided.
(A "PGP Secret Sub-key" according to file.)

I encoded a few amr files and "00 00" seems common and not
invalid.

The following works here but I believe the main issue is that
our amr decoder happily decodes the "PGP Secret Sub-key"
without any error messages so I wonder if the detection is
wrong at all.



Carl Eugen

Am Mi., 2. Okt. 2019 um 22:18 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>
> Am Mi., 2. Okt. 2019 um 20:19 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
> >
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavformat/amr.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/amr.c b/libavformat/amr.c
> > index 42840a50a3..600cb1b0f0 100644
> > --- a/libavformat/amr.c
> > +++ b/libavformat/amr.c
> > @@ -190,9 +190,12 @@ static int amrnb_probe(const AVProbeData *p)
> >                  if (b[++i] != last)
> >                      break;
> >              }
> > -            if (size > 0) {
> > -                valid++;
> > -                i += size;
> > +            while (size--) {
> > +                if (b[i])
> > +                    valid++;
> > +                else
> > +                    invalid++;
> > +                i++;
> >              }
> >          } else {
> >              valid = 0;
> > @@ -246,9 +249,12 @@ static int amrwb_probe(const AVProbeData *p)
> >                  if (b[++i] != last)
> >                      break;
> >              }
> > -            if (size > 0) {
> > -                valid++;
> > -                i += size;
> > +            while (size--) {
> > +                if (b[i])
> > +                    valid++;
> > +                else
> > +                    invalid++;
> > +                i++;
> >              }
> >          } else {
> >              valid = 0;
>
> The changes to amrwb are unneeded for the file you provided.
> (A "PGP Secret Sub-key" according to file.)
>
> I encoded a few amr files and "00 00" seems common and not
> invalid.
>
> The following works here but I believe the main issue is that
> our amr decoder happily decodes the "PGP Secret Sub-key"
> without any error messages so I wonder if the detection is
> wrong at all.
>
> diff --git a/libavformat/amr.c b/libavformat/amr.c
> index 42840a50a3..2645af95c2 100644
> --- a/libavformat/amr.c
> +++ b/libavformat/amr.c
> @@ -186,8 +186,18 @@ static int amrnb_probe(const AVProbeData *p)
>          if (mode < 9 && (b[i] & 0x4) == 0x4) {
>              int last = b[i];
>              int size = amrnb_packed_size[mode];
> +            int changes = 0, repeats = 0;
>              while (size--) {
>                  if (b[++i] != last)
> +                    changes++;
> +                else
> +                    repeats++;
> +                last = b[i];
> +                if (repeats >= 2) {
> +                    i += size;
> +                    size = 0;
> +                    break;
> +                } else if (changes > 4)
>                      break;
>              }
>              if (size > 0) {
> @@ -200,7 +210,7 @@ static int amrnb_probe(const AVProbeData *p)
>              i++;
>          }
>      }
> -    if (valid > 100 && valid >> 4 > invalid)
> +    if (valid > 120 && valid >> 4 > invalid)

Another idea is to require different modes, that should be simpler if you
believe that the sample should not get detected.

Carl Eugen

Am Mi., 2. Okt. 2019 um 22:27 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:

> Another idea is to require different modes

This would work for amr-wb but not amr-nb:
Silence encoded with libopencore_amrnb looks
similar to the provided sample that decodes to
noise.

Carl Eugen

On 10/2/19, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am Mi., 2. Okt. 2019 um 22:27 Uhr schrieb Carl Eugen Hoyos
> <ceffmpeg@gmail.com>:
>
>> Another idea is to require different modes
>
> This would work for amr-wb but not amr-nb:
> Silence encoded with libopencore_amrnb looks
> similar to the provided sample that decodes to
> noise.

Nope. Please fix it.

>
> Carl Eugen
> _______________________________________________
> 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".