[FFmpeg-devel,v1] avformat/utils: Fixes misdetection of zYLx.wav

Submitted by lance.lmwang@gmail.com on Nov. 6, 2019, 9 a.m.

Details

Message ID 20191106090045.21696-1-lance.lmwang@gmail.com
State New
Headers show

Commit Message

lance.lmwang@gmail.com Nov. 6, 2019, 9 a.m.
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/utils.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Carl Eugen Hoyos Nov. 6, 2019, 10:18 a.m.
Am Mi., 6. Nov. 2019 um 10:01 Uhr schrieb <lance.lmwang@gmail.com>:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/utils.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 8196442dd1..8381498e2b 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
>                 fmt->name, score);
>          for (i = 0; fmt_id_type[i].name; i++) {
>              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> -                    st->codecpar->sample_rate)
> +                if (st->codecpar->sample_rate)
>                      continue;

I believe the correct fix is to change the mp3 probe function so that it
does not return a score of 51 for the pcm pattern of ff ff e8 ff ff ff e8 ff...

Carl Eugen
Hendrik Leppkes Nov. 6, 2019, 10:30 a.m.
On Wed, Nov 6, 2019 at 10:01 AM <lance.lmwang@gmail.com> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/utils.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 8196442dd1..8381498e2b 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
>                 fmt->name, score);
>          for (i = 0; fmt_id_type[i].name; i++) {
>              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> -                    st->codecpar->sample_rate)
> +                if (st->codecpar->sample_rate)
>                      continue;
>                  if (st->request_probe > score &&
>                      st->codecpar->codec_id != fmt_id_type[i].id)

This change seems wrong, as you are practically rejecting probing of
any audio now, since those would get a sample rate.

- Hendrik
lance.lmwang@gmail.com Nov. 6, 2019, 10:51 a.m.
On Wed, Nov 06, 2019 at 11:18:10AM +0100, Carl Eugen Hoyos wrote:
> Am Mi., 6. Nov. 2019 um 10:01 Uhr schrieb <lance.lmwang@gmail.com>:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/utils.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 8196442dd1..8381498e2b 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> >                 fmt->name, score);
> >          for (i = 0; fmt_id_type[i].name; i++) {
> >              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> > -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> > -                    st->codecpar->sample_rate)
> > +                if (st->codecpar->sample_rate)
> >                      continue;
> 
> I believe the correct fix is to change the mp3 probe function so that it
> does not return a score of 51 for the pcm pattern of ff ff e8 ff ff ff e8 ff...

It has been detected as pcm and have get the valid audio sample rate, why we detect it
again here?  
I have run fate testing with the patch and haven't find any broken condition.


> 
> 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".
lance.lmwang@gmail.com Nov. 6, 2019, 11:01 a.m.
On Wed, Nov 06, 2019 at 11:30:41AM +0100, Hendrik Leppkes wrote:
> On Wed, Nov 6, 2019 at 10:01 AM <lance.lmwang@gmail.com> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/utils.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 8196442dd1..8381498e2b 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> >                 fmt->name, score);
> >          for (i = 0; fmt_id_type[i].name; i++) {
> >              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> > -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> > -                    st->codecpar->sample_rate)
> > +                if (st->codecpar->sample_rate)
> >                      continue;
> >                  if (st->request_probe > score &&
> >                      st->codecpar->codec_id != fmt_id_type[i].id)
> 
> This change seems wrong, as you are practically rejecting probing of
> any audio now, since those would get a sample rate.

By the old logic, I assume sample_rate > 0 means the audio has been detected, 
so don't probe the data again. The old code is for video only I think.
If have condition is broken, it should be add to fate testing.

> 
> - Hendrik
> _______________________________________________
> 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".
Carl Eugen Hoyos Nov. 6, 2019, 7:21 p.m.
Am Mi., 6. Nov. 2019 um 12:21 Uhr schrieb Limin Wang <lance.lmwang@gmail.com>:
>
> On Wed, Nov 06, 2019 at 11:18:10AM +0100, Carl Eugen Hoyos wrote:
> > Am Mi., 6. Nov. 2019 um 10:01 Uhr schrieb <lance.lmwang@gmail.com>:
> > >
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavformat/utils.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 8196442dd1..8381498e2b 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> > >                 fmt->name, score);
> > >          for (i = 0; fmt_id_type[i].name; i++) {
> > >              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> > > -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> > > -                    st->codecpar->sample_rate)
> > > +                if (st->codecpar->sample_rate)
> > >                      continue;
> >
> > I believe the correct fix is to change the mp3 probe function so that it
> > does not return a score of 51 for the pcm pattern of ff ff e8 ff ff ff e8 ff...
>
> It has been detected as pcm

There is no pcm detection in FFmpeg (it is hard iiuc), and the content
does not get detected as pcm but mp3 / mp1.

> and have get the valid audio sample rate, why we detect it
> again here?

How else can you rule out dca audio?

> I have run fate testing with the patch and haven't find any broken condition.

fate cannot test for all possible code paths.

Carl Eugen
Carl Eugen Hoyos Nov. 6, 2019, 7:23 p.m.
Am Mi., 6. Nov. 2019 um 12:01 Uhr schrieb Limin Wang <lance.lmwang@gmail.com>:
>
> On Wed, Nov 06, 2019 at 11:30:41AM +0100, Hendrik Leppkes wrote:
> > On Wed, Nov 6, 2019 at 10:01 AM <lance.lmwang@gmail.com> wrote:
> > >
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > >
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavformat/utils.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > index 8196442dd1..8381498e2b 100644
> > > --- a/libavformat/utils.c
> > > +++ b/libavformat/utils.c
> > > @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> > >                 fmt->name, score);
> > >          for (i = 0; fmt_id_type[i].name; i++) {
> > >              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> > > -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> > > -                    st->codecpar->sample_rate)
> > > +                if (st->codecpar->sample_rate)
> > >                      continue;
> > >                  if (st->request_probe > score &&
> > >                      st->codecpar->codec_id != fmt_id_type[i].id)
> >
> > This change seems wrong, as you are practically rejecting probing of
> > any audio now, since those would get a sample rate.
>
> By the old logic, I assume sample_rate > 0 means the audio has been detected,
> so don't probe the data again.

If sample_rate > 0 then do not consider non-audio codecs for auto-detection.

Carl Eugen
lance.lmwang@gmail.com Nov. 7, 2019, 1:30 a.m.
On Wed, Nov 06, 2019 at 08:21:56PM +0100, Carl Eugen Hoyos wrote:
> Am Mi., 6. Nov. 2019 um 12:21 Uhr schrieb Limin Wang <lance.lmwang@gmail.com>:
> >
> > On Wed, Nov 06, 2019 at 11:18:10AM +0100, Carl Eugen Hoyos wrote:
> > > Am Mi., 6. Nov. 2019 um 10:01 Uhr schrieb <lance.lmwang@gmail.com>:
> > > >
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > >
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavformat/utils.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > > > index 8196442dd1..8381498e2b 100644
> > > > --- a/libavformat/utils.c
> > > > +++ b/libavformat/utils.c
> > > > @@ -367,8 +367,7 @@ static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
> > > >                 fmt->name, score);
> > > >          for (i = 0; fmt_id_type[i].name; i++) {
> > > >              if (!strcmp(fmt->name, fmt_id_type[i].name)) {
> > > > -                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
> > > > -                    st->codecpar->sample_rate)
> > > > +                if (st->codecpar->sample_rate)
> > > >                      continue;
> > >
> > > I believe the correct fix is to change the mp3 probe function so that it
> > > does not return a score of 51 for the pcm pattern of ff ff e8 ff ff ff e8 ff...
> >
> > It has been detected as pcm
> 
> There is no pcm detection in FFmpeg (it is hard iiuc), and the content
> does not get detected as pcm but mp3 / mp1.
It's detected as wav and get the pcm codec, but if it's AV_CODEC_ID_PCM_S16LE, it'll request
probe again, see my updated patch. I add a checking to prove it.

> 
> > and have get the valid audio sample rate, why we detect it
> > again here?
> 
> How else can you rule out dca audio?

got it, haven't used dca audio before.

> 
> > I have run fate testing with the patch and haven't find any broken condition.
> 
> fate cannot test for all possible code paths.


> 
> 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".

Patch hide | download patch | download mbox

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 8196442dd1..8381498e2b 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -367,8 +367,7 @@  static int set_codec_from_probe_data(AVFormatContext *s, AVStream *st,
                fmt->name, score);
         for (i = 0; fmt_id_type[i].name; i++) {
             if (!strcmp(fmt->name, fmt_id_type[i].name)) {
-                if (fmt_id_type[i].type != AVMEDIA_TYPE_AUDIO &&
-                    st->codecpar->sample_rate)
+                if (st->codecpar->sample_rate)
                     continue;
                 if (st->request_probe > score &&
                     st->codecpar->codec_id != fmt_id_type[i].id)