diff mbox

[FFmpeg-devel,2/2] avformat/mp3dec: Check for occurances of headers within frames during probing

Message ID 20191107212532.22861-2-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Nov. 7, 2019, 9:25 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Fixes misdetection of zYLx.wav

Co-Author: Michael Niedermayer <michael@niedermayer.cc>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mp3dec.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Hendrik Leppkes Nov. 7, 2019, 9:37 p.m. UTC | #1 
On Thu, Nov 7, 2019 at 10:34 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Fixes misdetection of zYLx.wav
>
> Co-Author: Michael Niedermayer <michael@niedermayer.cc>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mp3dec.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 6848415657..eb40362548 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -73,7 +73,7 @@ static int mp3_read_probe(const AVProbeData *p)
>      int frames, ret;
>      int framesizes, max_framesizes;
>      uint32_t header;
> -    const uint8_t *buf, *buf0, *buf2, *end;
> +    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
>
>      buf0 = p->buf;
>      end = p->buf + p->buf_size - sizeof(uint32_t);
> @@ -88,11 +88,19 @@ static int mp3_read_probe(const AVProbeData *p)
>          buf2 = buf;
>          for(framesizes = frames = 0; buf2 < end; frames++) {
>              MPADecodeHeader h;
> +            int header_emu = 0;
>
>              header = AV_RB32(buf2);
>              ret = avpriv_mpegaudio_decode_header(&h, header);
>              if (ret != 0 || end - buf2 < h.frame_size)
>                  break;
> +
> +            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {
> +                uint32_t next_sync = AV_RB32(buf3);
> +                header_emu += (next_sync & MP3_MASK) == (header & MP3_MASK);
> +            }
> +            if (header_emu > 2)
> +                break;
>              buf2 += h.frame_size;
>              framesizes += h.frame_size;
>          }

I still have the same question - how possible is it that the actual
audio data actually has these same bits? Is it actually impossible? Or
are we just trading detection flaws here?

-  Hendrik
Lance Wang Nov. 8, 2019, 1:44 a.m. UTC | #2 
On Thu, Nov 07, 2019 at 10:37:05PM +0100, Hendrik Leppkes wrote:
> On Thu, Nov 7, 2019 at 10:34 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Fixes misdetection of zYLx.wav
> >
> > Co-Author: Michael Niedermayer <michael@niedermayer.cc>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mp3dec.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 6848415657..eb40362548 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -73,7 +73,7 @@ static int mp3_read_probe(const AVProbeData *p)
> >      int frames, ret;
> >      int framesizes, max_framesizes;
> >      uint32_t header;
> > -    const uint8_t *buf, *buf0, *buf2, *end;
> > +    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
> >
> >      buf0 = p->buf;
> >      end = p->buf + p->buf_size - sizeof(uint32_t);
> > @@ -88,11 +88,19 @@ static int mp3_read_probe(const AVProbeData *p)
> >          buf2 = buf;
> >          for(framesizes = frames = 0; buf2 < end; frames++) {
> >              MPADecodeHeader h;
> > +            int header_emu = 0;
> >
> >              header = AV_RB32(buf2);
> >              ret = avpriv_mpegaudio_decode_header(&h, header);
> >              if (ret != 0 || end - buf2 < h.frame_size)
> >                  break;
> > +
> > +            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {
> > +                uint32_t next_sync = AV_RB32(buf3);
> > +                header_emu += (next_sync & MP3_MASK) == (header & MP3_MASK);
> > +            }
> > +            if (header_emu > 2)
> > +                break;
> >              buf2 += h.frame_size;
> >              framesizes += h.frame_size;
> >          }
> 
> I still have the same question - how possible is it that the actual
> audio data actually has these same bits? Is it actually impossible? Or
> are we just trading detection flaws here?

By the iso 11172-3 and 13818-3 specs, every mp3 audio frame has a header, below is the header bitstream
syntax:

header()
{
    syncword   12bits bslsf
    id         1bit    bslsf
    layer      2bit    bslsf
    protection_bit 1bit bslsf
    bitrate_index 4bits bslsf
    sampling_frequency 2bits bslsf
    padding_bit  1bit bslsf
    private_bit 1bit bslsf
    mode 2bits bslsf
    mode_extension 2bits bslsf
    copyright 1bit bslsf
    original/home 1bit bslsf
    emphasis 2bits bslsf
}

so if the header is masking with MP3_MASK(0xFFFE0CCF), it'll zero out these field:
protection_bit set to 0
bitrate_index set to 0
sampling_freqency set to 0
mode set to 0

So the header should be keep same pattern if the audio frame is encode with same id and layer. If not, it's
error or something else.


> 
> -  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".
Lance Wang Nov. 8, 2019, 7:11 a.m. UTC | #3 
On Thu, Nov 07, 2019 at 10:25:32PM +0100, Michael Niedermayer wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Fixes misdetection of zYLx.wav
> 
> Co-Author: Michael Niedermayer <michael@niedermayer.cc>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

why it's same?


> ---
>  libavformat/mp3dec.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 6848415657..eb40362548 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -73,7 +73,7 @@ static int mp3_read_probe(const AVProbeData *p)
>      int frames, ret;
>      int framesizes, max_framesizes;
>      uint32_t header;
> -    const uint8_t *buf, *buf0, *buf2, *end;
> +    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
>  
>      buf0 = p->buf;
>      end = p->buf + p->buf_size - sizeof(uint32_t);
> @@ -88,11 +88,19 @@ static int mp3_read_probe(const AVProbeData *p)
>          buf2 = buf;
>          for(framesizes = frames = 0; buf2 < end; frames++) {
>              MPADecodeHeader h;
> +            int header_emu = 0;
>  
>              header = AV_RB32(buf2);
>              ret = avpriv_mpegaudio_decode_header(&h, header);
>              if (ret != 0 || end - buf2 < h.frame_size)
>                  break;
> +
> +            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {

Sorry, please ignore my comments for the first patch, with patch2, it's necessary.
The patch looks good to me, I have tested and OK.

> +                uint32_t next_sync = AV_RB32(buf3);
> +                header_emu += (next_sync & MP3_MASK) == (header & MP3_MASK);
> +            }
> +            if (header_emu > 2)
> +                break;
>              buf2 += h.frame_size;
>              framesizes += h.frame_size;
>          }
> -- 
> 2.23.0
>
Michael Niedermayer Nov. 8, 2019, 10:27 p.m. UTC | #4 
On Thu, Nov 07, 2019 at 10:37:05PM +0100, Hendrik Leppkes wrote:
> On Thu, Nov 7, 2019 at 10:34 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Fixes misdetection of zYLx.wav
> >
> > Co-Author: Michael Niedermayer <michael@niedermayer.cc>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mp3dec.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > index 6848415657..eb40362548 100644
> > --- a/libavformat/mp3dec.c
> > +++ b/libavformat/mp3dec.c
> > @@ -73,7 +73,7 @@ static int mp3_read_probe(const AVProbeData *p)
> >      int frames, ret;
> >      int framesizes, max_framesizes;
> >      uint32_t header;
> > -    const uint8_t *buf, *buf0, *buf2, *end;
> > +    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
> >
> >      buf0 = p->buf;
> >      end = p->buf + p->buf_size - sizeof(uint32_t);
> > @@ -88,11 +88,19 @@ static int mp3_read_probe(const AVProbeData *p)
> >          buf2 = buf;
> >          for(framesizes = frames = 0; buf2 < end; frames++) {
> >              MPADecodeHeader h;
> > +            int header_emu = 0;
> >
> >              header = AV_RB32(buf2);
> >              ret = avpriv_mpegaudio_decode_header(&h, header);
> >              if (ret != 0 || end - buf2 < h.frame_size)
> >                  break;
> > +
> > +            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {
> > +                uint32_t next_sync = AV_RB32(buf3);
> > +                header_emu += (next_sync & MP3_MASK) == (header & MP3_MASK);
> > +            }
> > +            if (header_emu > 2)
> > +                break;
> >              buf2 += h.frame_size;
> >              framesizes += h.frame_size;
> >          }
> 
> I still have the same question - how possible is it that the actual
> audio data actually has these same bits? Is it actually impossible? Or
> are we just trading detection flaws here?

mp3 does not prevent header emulation unless my memory of the spec is
flawed. So it is possible that valid looking mp3 headers occur inside
valid mp3 frames.
This is thus a probabilistic test. 
Consider A1
    A stream that has few valid mp3 headers that in addition are spaced 
    correctly for mp3 will likely be mp3 or some mp3 in some othetr container
    but its very unlikely to be PCM
Consider A2
    A stream that has few valid mp3 headers that are not spaced 
    correctly for mp3 is unlikely to be raw mp3, whatever that may be
Consider B
    A stream that has many valid mp3 headers, so that there are maybe
    100 headers within each frame.
    
For the last (B) case having headers match at the location of frame sizes
is a significantly weaker hint toward valid mp3 because there are many
its not unlikely to match something, so the previous seperation we
had between A1 and A2 is no longer possibly at B and thus something
should reduce the score for mp3 in the B case

The patch does that by considering frames with 3 or more headers
inside to be not matching the frame size of the previous.

We use an == check instead of a avpriv_mpegaudio_decode_header() call
because its simpler and faster and works for the case we have

There are many possible ways to achieve similar effects, for example
we could reduce the score based on the "density" of headers within
frames, this was just the simplest i saw and was already mostly implemented
by Limin. But surely it can be done differently

Also last thought, probing is fundamentally a probabilistic thing

A more radically different approuch would be to test for more of the mp3
syntax after the header but i suspect that would be much more complex
especially considering that we need to do this for mp1 and mp2 too if
its supposed to help for such cases

[...]
Paul B Mahol Nov. 16, 2019, 1:57 p.m. UTC | #5 
lgtm

On 11/7/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
>
> Fixes misdetection of zYLx.wav
>
> Co-Author: Michael Niedermayer <michael@niedermayer.cc>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mp3dec.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 6848415657..eb40362548 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -73,7 +73,7 @@ static int mp3_read_probe(const AVProbeData *p)
>      int frames, ret;
>      int framesizes, max_framesizes;
>      uint32_t header;
> -    const uint8_t *buf, *buf0, *buf2, *end;
> +    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
>
>      buf0 = p->buf;
>      end = p->buf + p->buf_size - sizeof(uint32_t);
> @@ -88,11 +88,19 @@ static int mp3_read_probe(const AVProbeData *p)
>          buf2 = buf;
>          for(framesizes = frames = 0; buf2 < end; frames++) {
>              MPADecodeHeader h;
> +            int header_emu = 0;
>
>              header = AV_RB32(buf2);
>              ret = avpriv_mpegaudio_decode_header(&h, header);
>              if (ret != 0 || end - buf2 < h.frame_size)
>                  break;
> +
> +            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {
> +                uint32_t next_sync = AV_RB32(buf3);
> +                header_emu += (next_sync & MP3_MASK) == (header &
> MP3_MASK);
> +            }
> +            if (header_emu > 2)
> +                break;
>              buf2 += h.frame_size;
>              framesizes += h.frame_size;
>          }
> --
> 2.23.0
>
> _______________________________________________
> 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 Nov. 16, 2019, 10:38 p.m. UTC | #6 
On Sat, Nov 16, 2019 at 02:57:47PM +0100, Paul B Mahol wrote:
> lgtm

will apply

thx

[...]
diff mbox

Patch

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index 6848415657..eb40362548 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -73,7 +73,7 @@  static int mp3_read_probe(const AVProbeData *p)
     int frames, ret;
     int framesizes, max_framesizes;
     uint32_t header;
-    const uint8_t *buf, *buf0, *buf2, *end;
+    const uint8_t *buf, *buf0, *buf2, *buf3, *end;
 
     buf0 = p->buf;
     end = p->buf + p->buf_size - sizeof(uint32_t);
@@ -88,11 +88,19 @@  static int mp3_read_probe(const AVProbeData *p)
         buf2 = buf;
         for(framesizes = frames = 0; buf2 < end; frames++) {
             MPADecodeHeader h;
+            int header_emu = 0;
 
             header = AV_RB32(buf2);
             ret = avpriv_mpegaudio_decode_header(&h, header);
             if (ret != 0 || end - buf2 < h.frame_size)
                 break;
+
+            for (buf3 = buf2 + 4; buf3 < buf2 + h.frame_size; buf3++) {
+                uint32_t next_sync = AV_RB32(buf3);
+                header_emu += (next_sync & MP3_MASK) == (header & MP3_MASK);
+            }
+            if (header_emu > 2)
+                break;
             buf2 += h.frame_size;
             framesizes += h.frame_size;
         }