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

2015-10-20 Thread wm4 
On Tue, 20 Oct 2015 02:12:00 +0200
Michael Niedermayer  wrote:

> On Mon, Oct 19, 2015 at 11:12:03PM +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. The
> > set of compared header bits is the same as the libavcodec mp3 parser
> > uses for similar purposes.
> > ---
> > Now compares less header bits, as requested.
> > ---
> >  libavformat/mp3dec.c | 28 +---
> >  1 file changed, 21 insertions(+), 7 deletions(-)  
> 
> LGTM
> 
> thanks
> 
> [...]

Pushed, thanks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-19 Thread wm4 
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. The
set of compared header bits is the same as the libavcodec mp3 parser
uses for similar purposes.
---
Now compares less header bits, as requested.
---
 libavformat/mp3dec.c | 28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
index d3080d7..32ca00c 100644
--- a/libavformat/mp3dec.c
+++ b/libavformat/mp3dec.c
@@ -42,6 +42,9 @@
 
 #define XING_TOC_COUNT 100
 
+#define SAME_HEADER_MASK \
+   (0xffe0 | (3 << 17) | (3 << 10) | (3 << 19))
+
 typedef struct {
 AVClass *class;
 int64_t filesize;
@@ -54,7 +57,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 +377,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, );
+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, ) >= 0 &&
+(header & SAME_HEADER_MASK) == (header2 & SAME_HEADER_MASK))
+{
+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;
+}
 }
 avio_seek(s->pb, off, SEEK_SET);
 }
@@ -420,7 +432,7 @@ static int mp3_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 
 #define SEEK_WINDOW 4096
 
-static int check(AVIOContext *pb, int64_t pos)
+static int check(AVIOContext *pb, int64_t pos, uint32_t *ret_header)
 {
 int64_t ret = avio_seek(pb, pos, SEEK_SET);
 unsigned header;
@@ -434,6 +446,8 @@ static int check(AVIOContext *pb, int64_t pos)
 if (avpriv_mpegaudio_decode_header(, header) == 1)
 return -1;
 
+if (ret_header)
+*ret_header = header;
 return sd.frame_size;
 }
 
@@ -461,7 +475,7 @@ static int64_t mp3_sync(AVFormatContext *s, int64_t 
target_pos, int flags)
 continue;
 
 for(j=0; jpb, pos);
+ret = check(s->pb, pos, NULL);
 if(ret < 0)
 break;
 if ((target_pos - pos)*dir <= 0 && abs(MIN_VALID/2-j) < score) {
-- 
2.6.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-19 Thread Michael Niedermayer 
On Mon, Oct 19, 2015 at 11:12:03PM +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. The
> set of compared header bits is the same as the libavcodec mp3 parser
> uses for similar purposes.
> ---
> Now compares less header bits, as requested.
> ---
>  libavformat/mp3dec.c | 28 +---
>  1 file changed, 21 insertions(+), 7 deletions(-)

LGTM

thanks

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-17 Thread wm4 
On Sat, 17 Oct 2015 14:02:09 +0200
Michael Niedermayer  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, );
> > +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, ) >= 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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-17 Thread Michael Niedermayer 
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, );
> +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, ) >= 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

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

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-17 Thread 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  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, );
> > > +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, ) >= 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.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2015-10-16 Thread wm4 
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, );
+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, ) >= 0 &&
+(header & 0xFFFEFCF0) == (header2 & 0xFFFEFCF0))
+{
+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;
+}
 }
 avio_seek(s->pb, off, SEEK_SET);
 }
@@ -420,7 +429,7 @@ static int mp3_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 
 #define SEEK_WINDOW 4096
 
-static int check(AVIOContext *pb, int64_t pos)
+static int check(AVIOContext *pb, int64_t pos, uint32_t *ret_header)
 {
 int64_t ret = avio_seek(pb, pos, SEEK_SET);
 unsigned header;
@@ -434,6 +443,8 @@ static int check(AVIOContext *pb, int64_t pos)
 if (avpriv_mpegaudio_decode_header(, header) == 1)
 return -1;
 
+if (ret_header)
+*ret_header = header;
 return sd.frame_size;
 }
 
@@ -461,7 +472,7 @@ static int64_t mp3_sync(AVFormatContext *s, int64_t 
target_pos, int flags)
 continue;
 
 for(j=0; jpb, pos);
+ret = check(s->pb, pos, NULL);
 if(ret < 0)
 break;
 if ((target_pos - pos)*dir <= 0 && abs(MIN_VALID/2-j) < score) {
-- 
2.5.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel