[FFmpeg-devel] [PATCH] avformat/mp3dec: improve junk skipping heuristic

[Michael Niedermayer]

On Sat, Oct 17, 2015 at 04:18:14PM +0200, wm4 wrote:
> On Sat, 17 Oct 2015 14:02:09 +0200
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Fri, Oct 16, 2015 at 08:04:08PM +0200, wm4 wrote:
> > > Commit 2b3e9bbfb529e6bde238aeb511b55ebe461664c8 caused problems for a
> > > certain API user:
> > > 
> > > https://code.google.com/p/chromium/issues/detail?id=537725
> > > https://code.google.com/p/chromium/issues/detail?id=542032
> > > 
> > > The problem seems rather arbitrary, because if there's junk, anything
> > > can happen. In this case, the imperfect junk skipping just caused it to
> > > read different junk, from what I can see.
> > > 
> > > We can improve the accuracy of junk detection by a lot by checking if 2
> > > consecutive frames use the same configuration. While in theory it might
> > > be completely fine for the 1st frame to have a different format than the
> > > 2nd frame, it's exceedingly unlikely, and I can't think of a legitimate
> > > use-case.
> > > 
> > > This is approximately the same mpg123 does for junk skipping, and the
> > > set of compared header bits is exactly the same.
> > > ---
> > > The reason Chromium had so much problems with this is that they don't
> > > allow mid-stream format changes at all, and the junk triggered format
> > > changes.
> > > 
> > > (Would be nice if Google paid me for this.)
> > > ---
> > >  libavformat/mp3dec.c | 25 ++++++++++++++++++-------
> > >  1 file changed, 18 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> > > index d3080d7..e7b6234 100644
> > > --- a/libavformat/mp3dec.c
> > > +++ b/libavformat/mp3dec.c
> > > @@ -54,7 +54,7 @@ typedef struct {
> > >      int is_cbr;
> > >  } MP3DecContext;
> > >  
> > > -static int check(AVIOContext *pb, int64_t pos);
> > > +static int check(AVIOContext *pb, int64_t pos, uint32_t *header);
> > >  
> > >  /* mp3 read */
> > >  
> > > @@ -374,12 +374,21 @@ static int mp3_read_header(AVFormatContext *s)
> > >  
> > >      off = avio_tell(s->pb);
> > >      for (i = 0; i < 64 * 1024; i++) {
> > > +        uint32_t header, header2;
> > > +        int frame_size;
> > >          if (!(i&1023))
> > >              ffio_ensure_seekback(s->pb, i + 1024 + 4);
> > > -        if (check(s->pb, off + i) >= 0) {
> > > -            av_log(s, AV_LOG_INFO, "Skipping %d bytes of junk at %"PRId64".\n", i, off);
> > > -            avio_seek(s->pb, off + i, SEEK_SET);
> > > -            break;
> > > +        frame_size = check(s->pb, off + i, &header);
> > > +        if (frame_size > 0) {
> > > +            avio_seek(s->pb, off, SEEK_SET);
> > > +            ffio_ensure_seekback(s->pb, i + 1024 + frame_size + 4);
> > > +            if (check(s->pb, off + i + frame_size, &header2) >= 0 &&
> > > +                (header & 0xFFFEFCF0) == (header2 & 0xFFFEFCF0))
> > 
> > This also requires the bitrate and stereo type to match
> > teh bitrate can change in VBR, the stereo coding type can also change
> > i think
> > 
> > [...]
> 
> What do you suggest?

does checking only the remaining parts/bits work ?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151017/1d2664f5/attachment.sig>