Message ID | 20191107212532.22861-2-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
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
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".
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 >
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 [...]
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".
On Sat, Nov 16, 2019 at 02:57:47PM +0100, Paul B Mahol wrote:
> lgtm
will apply
thx
[...]
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; }