Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Carl Eugen Hoyos]

Michael Bradshaw  gmail.com> writes:

> > +return AVPROBE_SCORE_MAX / 4 + 1;
> 
> A score of 26 seems low to me, but maybe that's just me.

Increased the score and pushed.

Thank you, Carl Eugen

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

[FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Carl Eugen Hoyos]

Hi!

Attached patch improves ico probing, previously mpeg-2 frames could be 
detected.

Please comment, Carl Eugen
diff --git a/libavformat/icodec.c b/libavformat/icodec.c
index 22e2099..c3e87ea 100644
--- a/libavformat/icodec.c
+++ b/libavformat/icodec.c
@@ -44,8 +44,12 @@ typedef struct {
 
 static int probe(AVProbeData *p)
 {
-if (AV_RL16(p->buf) == 0 && AV_RL16(p->buf + 2) == 1 && AV_RL16(p->buf + 
4))
-return AVPROBE_SCORE_MAX / 4;
+if (   AV_RL16(p->buf) == 0
+&& AV_RL16(p->buf + 2) == 1
+&& AV_RL16(p->buf + 4)
+&& AV_RL16(p->buf + 10) == 1
+&& !p->buf[13])
+return AVPROBE_SCORE_MAX / 4 + 1;
 return 0;
 }
 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Michael Niedermayer]

On Tue, Jan 12, 2016 at 01:49:37PM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> On Tuesday 12 January 2016 01:33:53 pm Carl Eugen Hoyos wrote:
> >
> > Attached patch improves ico probing, previously mpeg-2 frames could be
> > detected.
> 
> Wikipedia claims that planes can be 0, new patch attached.
> 
> Carl Eugen

>  icodec.c |8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 5587972d12000a679757c9b42947869eb1cee0f5  patchico.diff
> diff --git a/libavformat/icodec.c b/libavformat/icodec.c
> index 22e2099..4d6e6dc 100644

tools/probetest 256 4096
testing size=1
testing size=2
testing size=4
testing size=8
Failure of ico probing code with score=26 type=1 p=EA6 size=8
testing size=16

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus


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

Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Carl Eugen Hoyos]

Hi!

On Tuesday 12 January 2016 01:33:53 pm Carl Eugen Hoyos wrote:
>
> Attached patch improves ico probing, previously mpeg-2 frames could be
> detected.

Wikipedia claims that planes can be 0, new patch attached.

Carl Eugen
diff --git a/libavformat/icodec.c b/libavformat/icodec.c
index 22e2099..4d6e6dc 100644
--- a/libavformat/icodec.c
+++ b/libavformat/icodec.c
@@ -44,8 +44,12 @@ typedef struct {
 
 static int probe(AVProbeData *p)
 {
-if (AV_RL16(p->buf) == 0 && AV_RL16(p->buf + 2) == 1 && AV_RL16(p->buf + 
4))
-return AVPROBE_SCORE_MAX / 4;
+if (   AV_RL16(p->buf) == 0
+&& AV_RL16(p->buf + 2) == 1
+&& AV_RL16(p->buf + 4)
+&& !(AV_RL16(p->buf + 10) & ~1)
+&& !p->buf[13])
+return AVPROBE_SCORE_MAX / 4 + 1;
 return 0;
 }
 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Carl Eugen Hoyos]

On Tuesday 12 January 2016 02:49:59 pm Michael Niedermayer wrote:
> On Tue, Jan 12, 2016 at 01:49:37PM +0100, Carl Eugen Hoyos wrote:
> > Hi!
> >
> > On Tuesday 12 January 2016 01:33:53 pm Carl Eugen Hoyos wrote:
> > > Attached patch improves ico probing, previously mpeg-2 frames could be
> > > detected.
> >
> > Wikipedia claims that planes can be 0, new patch attached.
> >
> > Carl Eugen
> >
> >  icodec.c |8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 5587972d12000a679757c9b42947869eb1cee0f5  patchico.diff
> > diff --git a/libavformat/icodec.c b/libavformat/icodec.c
> > index 22e2099..4d6e6dc 100644
>
> tools/probetest 256 4096
> testing size=1
> testing size=2
> testing size=4
> testing size=8
> Failure of ico probing code with score=26 type=1 p=EA6 size=8

Before the old patch, 32 bits were tested and return a value of 25, 
after the patch, 55 bits were tested and returned 26.
Imo, this just shows that probetest cannot see every possible issue.

Improved patch attached.

Carl Eugen
diff --git a/libavformat/icodec.c b/libavformat/icodec.c
index 22e2099..9cf3dca 100644
--- a/libavformat/icodec.c
+++ b/libavformat/icodec.c
@@ -27,6 +27,7 @@
 #include "libavutil/intreadwrite.h"
 #include "libavcodec/bytestream.h"
 #include "libavcodec/bmp.h"
+#include "libavcodec/png.h"
 #include "avformat.h"
 #include "internal.h"
 
@@ -44,9 +45,30 @@ typedef struct {
 
 static int probe(AVProbeData *p)
 {
-if (AV_RL16(p->buf) == 0 && AV_RL16(p->buf + 2) == 1 && AV_RL16(p->buf + 
4))
-return AVPROBE_SCORE_MAX / 4;
-return 0;
+unsigned i, frames = AV_RL16(p->buf + 4);
+
+if (AV_RL16(p->buf) || AV_RL16(p->buf + 2) != 1 || !frames)
+return 0;
+for (i = 0; i < frames; i++) {
+unsigned offset;
+if (AV_RL16(p->buf + 10 + i * 16) & ~1)
+return FFMIN(i, AVPROBE_SCORE_MAX / 4);
+if (p->buf[13 + i * 16])
+return FFMIN(i, AVPROBE_SCORE_MAX / 4);
+if (AV_RL32(p->buf + 14 + i * 16) < 40)
+return FFMIN(i, AVPROBE_SCORE_MAX / 4);
+offset = AV_RL32(p->buf + 18 + i * 16);
+if (offset < 22)
+return FFMIN(i, AVPROBE_SCORE_MAX / 4);
+if (offset + 8 > p->buf_size)
+return AVPROBE_SCORE_MAX / 4 + FFMIN(i, 1);
+if (p->buf[offset] != 40 && AV_RB64(p->buf + offset) != PNGSIG)
+return FFMIN(i, AVPROBE_SCORE_MAX / 4);
+if (i * 16 + 6 > p->buf_size)
+return AVPROBE_SCORE_MAX / 4;
+}
+
+return AVPROBE_SCORE_MAX / 4 + 1;
 }
 
 static int read_header(AVFormatContext *s)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Michael Niedermayer]

On Tue, Jan 12, 2016 at 08:33:47AM -0800, Michael Bradshaw wrote:
> Overall it looks good. I thought it might overflow the buffer but with
> AVPROBE_PADDING_SIZE it doesn't.
> 
> On Tue, Jan 12, 2016 at 7:09 AM, Carl Eugen Hoyos  wrote:
> > diff --git a/libavformat/icodec.c b/libavformat/icodec.c
> > index 22e2099..9cf3dca 100644
> > --- a/libavformat/icodec.c
> > +++ b/libavformat/icodec.c
> > @@ -27,6 +27,7 @@
> >  #include "libavutil/intreadwrite.h"
> >  #include "libavcodec/bytestream.h"
> >  #include "libavcodec/bmp.h"
> > +#include "libavcodec/png.h"
> >  #include "avformat.h"
> >  #include "internal.h"
> >
> > @@ -44,9 +45,30 @@ typedef struct {
> >
> >  static int probe(AVProbeData *p)
> >  {
> > -if (AV_RL16(p->buf) == 0 && AV_RL16(p->buf + 2) == 1 && AV_RL16(p->buf 
> > + 4))
> > -return AVPROBE_SCORE_MAX / 4;
> > -return 0;
> > +unsigned i, frames = AV_RL16(p->buf + 4);
> > +
> > +if (AV_RL16(p->buf) || AV_RL16(p->buf + 2) != 1 || !frames)
> > +return 0;
> > +for (i = 0; i < frames; i++) {
> > +unsigned offset;
> > +if (AV_RL16(p->buf + 10 + i * 16) & ~1) // color planes
> > +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> > +if (p->buf[13 + i * 16])
> > +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> > +if (AV_RL32(p->buf + 14 + i * 16) < 40)  // size
> > +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> > +offset = AV_RL32(p->buf + 18 + i * 16);
> > +if (offset < 22)
> > +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> > +if (offset + 8 > p->buf_size)
> > +return AVPROBE_SCORE_MAX / 4 + FFMIN(i, 1);
> > +if (p->buf[offset] != 40 && AV_RB64(p->buf + offset) != PNGSIG)
> > +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> > +if (i * 16 + 6 > p->buf_size)
> > +return AVPROBE_SCORE_MAX / 4;
> > +}
> > +
> > +return AVPROBE_SCORE_MAX / 4 + 1;
> 
> A score of 26 seems low to me, but maybe that's just me.

same feeling
either way LGTM

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras


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

Re: [FFmpeg-devel] [PATCH]lavf/icodec: Improve probe function

[Michael Bradshaw]

Overall it looks good. I thought it might overflow the buffer but with
AVPROBE_PADDING_SIZE it doesn't.

On Tue, Jan 12, 2016 at 7:09 AM, Carl Eugen Hoyos  wrote:
> diff --git a/libavformat/icodec.c b/libavformat/icodec.c
> index 22e2099..9cf3dca 100644
> --- a/libavformat/icodec.c
> +++ b/libavformat/icodec.c
> @@ -27,6 +27,7 @@
>  #include "libavutil/intreadwrite.h"
>  #include "libavcodec/bytestream.h"
>  #include "libavcodec/bmp.h"
> +#include "libavcodec/png.h"
>  #include "avformat.h"
>  #include "internal.h"
>
> @@ -44,9 +45,30 @@ typedef struct {
>
>  static int probe(AVProbeData *p)
>  {
> -if (AV_RL16(p->buf) == 0 && AV_RL16(p->buf + 2) == 1 && AV_RL16(p->buf + 
> 4))
> -return AVPROBE_SCORE_MAX / 4;
> -return 0;
> +unsigned i, frames = AV_RL16(p->buf + 4);
> +
> +if (AV_RL16(p->buf) || AV_RL16(p->buf + 2) != 1 || !frames)
> +return 0;
> +for (i = 0; i < frames; i++) {
> +unsigned offset;
> +if (AV_RL16(p->buf + 10 + i * 16) & ~1) // color planes
> +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> +if (p->buf[13 + i * 16])
> +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> +if (AV_RL32(p->buf + 14 + i * 16) < 40)  // size
> +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> +offset = AV_RL32(p->buf + 18 + i * 16);
> +if (offset < 22)
> +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> +if (offset + 8 > p->buf_size)
> +return AVPROBE_SCORE_MAX / 4 + FFMIN(i, 1);
> +if (p->buf[offset] != 40 && AV_RB64(p->buf + offset) != PNGSIG)
> +return FFMIN(i, AVPROBE_SCORE_MAX / 4);
> +if (i * 16 + 6 > p->buf_size)
> +return AVPROBE_SCORE_MAX / 4;
> +}
> +
> +return AVPROBE_SCORE_MAX / 4 + 1;

A score of 26 seems low to me, but maybe that's just me.

>  }
>
>  static int read_header(AVFormatContext *s)

I checked all the various header bytes this would be checking and it
all looks good.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel