diff mbox series

[FFmpeg-devel] avformat/img2dec: skip DHT segment in jpeg_probe()

Message ID 20200401085220.293934-1-matthieu.bouron@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/img2dec: skip DHT segment in jpeg_probe()
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Matthieu Bouron April 1, 2020, 8:52 a.m. UTC
---
 libavformat/img2dec.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Derek Buitenhuis April 1, 2020, 12:04 p.m. UTC | #1
On 01/04/2020 09:52, Matthieu Bouron wrote:
> ---
>  libavformat/img2dec.c | 1 +
>  1 file changed, 1 insertion(+)

Looks correct, but a commit message explaining why would be good.

- Derek
Matthieu Bouron April 1, 2020, 1:28 p.m. UTC | #2
On Wed, Apr 01, 2020 at 01:04:58PM +0100, Derek Buitenhuis wrote:
> On 01/04/2020 09:52, Matthieu Bouron wrote:
> > ---
> >  libavformat/img2dec.c | 1 +
> >  1 file changed, 1 insertion(+)
> 
> Looks correct, but a commit message explaining why would be good.
> 

Would "Skips the DHT segment in jpeg_probe() to avoid looking for JPEG
markers in the DHT segment data" be OK ?

> - Derek
> _______________________________________________
> 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".
Carl Eugen Hoyos April 1, 2020, 1:38 p.m. UTC | #3
Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
<matthieu.bouron@gmail.com>:
>
> ---
>  libavformat/img2dec.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index 40f3e3d499e..93cd51c1932 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
>                  return 0;
>              state = EOI;
>              break;
> +        case DHT:
>          case DQT:
>          case APP0:
>          case APP1:

Please provide a sample file.

Thank you, Carl Eugen
Matthieu Bouron April 1, 2020, 1:47 p.m. UTC | #4
On Wed, Apr 01, 2020 at 03:38:45PM +0200, Carl Eugen Hoyos wrote:
> Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
> <matthieu.bouron@gmail.com>:
> >
> > ---
> >  libavformat/img2dec.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > index 40f3e3d499e..93cd51c1932 100644
> > --- a/libavformat/img2dec.c
> > +++ b/libavformat/img2dec.c
> > @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
> >                  return 0;
> >              state = EOI;
> >              break;
> > +        case DHT:
> >          case DQT:
> >          case APP0:
> >          case APP1:
> 
> Please provide a sample file.

This patch does not fix a particular issue. IMHO, we should skip the DHT
segment just like the other segments (and avoids parsing the data it
contains) unless there is a particular reason for it ?

> 
> Thank you, Carl Eugen
> _______________________________________________
> 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".
Derek Buitenhuis April 1, 2020, 1:58 p.m. UTC | #5
On 01/04/2020 14:28, Matthieu Bouron wrote:
> Would "Skips the DHT segment in jpeg_probe() to avoid looking for JPEG
> markers in the DHT segment data" be OK ?

Sounds OK.

- Derek
Carl Eugen Hoyos April 1, 2020, 2:09 p.m. UTC | #6
Am Mi., 1. Apr. 2020 um 15:48 Uhr schrieb Matthieu Bouron
<matthieu.bouron@gmail.com>:
>
> On Wed, Apr 01, 2020 at 03:38:45PM +0200, Carl Eugen Hoyos wrote:
> > Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
> > <matthieu.bouron@gmail.com>:
> > >
> > > ---
> > >  libavformat/img2dec.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > > index 40f3e3d499e..93cd51c1932 100644
> > > --- a/libavformat/img2dec.c
> > > +++ b/libavformat/img2dec.c
> > > @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
> > >                  return 0;
> > >              state = EOI;
> > >              break;
> > > +        case DHT:
> > >          case DQT:
> > >          case APP0:
> > >          case APP1:
> >
> > Please provide a sample file.
>
> This patch does not fix a particular issue. IMHO, we should skip the DHT
> segment just like the other segments (and avoids parsing the data it
> contains) unless there is a particular reason for it ?

I am (somewhat strongly) against patches that do not fix issues but
Michael will hopefully comment.

Or are you trying to improve probing speed?

Carl Eugen
Matthieu Bouron April 1, 2020, 2:24 p.m. UTC | #7
On Wed, Apr 01, 2020 at 04:09:08PM +0200, Carl Eugen Hoyos wrote:
> Am Mi., 1. Apr. 2020 um 15:48 Uhr schrieb Matthieu Bouron
> <matthieu.bouron@gmail.com>:
> >
> > On Wed, Apr 01, 2020 at 03:38:45PM +0200, Carl Eugen Hoyos wrote:
> > > Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
> > > <matthieu.bouron@gmail.com>:
> > > >
> > > > ---
> > > >  libavformat/img2dec.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > > > index 40f3e3d499e..93cd51c1932 100644
> > > > --- a/libavformat/img2dec.c
> > > > +++ b/libavformat/img2dec.c
> > > > @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
> > > >                  return 0;
> > > >              state = EOI;
> > > >              break;
> > > > +        case DHT:
> > > >          case DQT:
> > > >          case APP0:
> > > >          case APP1:
> > >
> > > Please provide a sample file.
> >
> > This patch does not fix a particular issue. IMHO, we should skip the DHT
> > segment just like the other segments (and avoids parsing the data it
> > contains) unless there is a particular reason for it ?
> 
> I am (somewhat strongly) against patches that do not fix issues but
> Michael will hopefully comment.
> 
> Or are you trying to improve probing speed?

I am trying to fix an issues with JPEG files containing MPF metadata
appended at the end.
Theses files makes the jpeg_probe function fails (returns 0) because the
MPF metadata contains JPEG images (so jpeg_probe() finds a SOI marker
after EOI). I will send a separate patch for that and provide a
sample.

This patch should improve the probing speed by little though.

> 
> Carl Eugen
> _______________________________________________
> 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".
Carl Eugen Hoyos April 1, 2020, 2:29 p.m. UTC | #8
Am Mi., 1. Apr. 2020 um 16:24 Uhr schrieb Matthieu Bouron
<matthieu.bouron@gmail.com>:
>
> On Wed, Apr 01, 2020 at 04:09:08PM +0200, Carl Eugen Hoyos wrote:
> > Am Mi., 1. Apr. 2020 um 15:48 Uhr schrieb Matthieu Bouron
> > <matthieu.bouron@gmail.com>:
> > >
> > > On Wed, Apr 01, 2020 at 03:38:45PM +0200, Carl Eugen Hoyos wrote:
> > > > Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
> > > > <matthieu.bouron@gmail.com>:
> > > > >
> > > > > ---
> > > > >  libavformat/img2dec.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > > > > index 40f3e3d499e..93cd51c1932 100644
> > > > > --- a/libavformat/img2dec.c
> > > > > +++ b/libavformat/img2dec.c
> > > > > @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
> > > > >                  return 0;
> > > > >              state = EOI;
> > > > >              break;
> > > > > +        case DHT:
> > > > >          case DQT:
> > > > >          case APP0:
> > > > >          case APP1:
> > > >
> > > > Please provide a sample file.
> > >
> > > This patch does not fix a particular issue. IMHO, we should skip the DHT
> > > segment just like the other segments (and avoids parsing the data it
> > > contains) unless there is a particular reason for it ?
> >
> > I am (somewhat strongly) against patches that do not fix issues but
> > Michael will hopefully comment.
> >
> > Or are you trying to improve probing speed?
>
> I am trying to fix an issues with JPEG files containing MPF metadata
> appended at the end.
> Theses files makes the jpeg_probe function fails (returns 0) because the
> MPF metadata contains JPEG images (so jpeg_probe() finds a SOI marker
> after EOI). I will send a separate patch for that and provide a
> sample.

Are these valid jpeg files?

> This patch should improve the probing speed by little though.

Then please measure the speedup and mention it in the
commit message.

Thank you, Carl Eugen
Matthieu Bouron April 1, 2020, 4:22 p.m. UTC | #9
On Wed, Apr 01, 2020 at 04:29:16PM +0200, Carl Eugen Hoyos wrote:
> Am Mi., 1. Apr. 2020 um 16:24 Uhr schrieb Matthieu Bouron
> <matthieu.bouron@gmail.com>:
> >
> > On Wed, Apr 01, 2020 at 04:09:08PM +0200, Carl Eugen Hoyos wrote:
> > > Am Mi., 1. Apr. 2020 um 15:48 Uhr schrieb Matthieu Bouron
> > > <matthieu.bouron@gmail.com>:
> > > >
> > > > On Wed, Apr 01, 2020 at 03:38:45PM +0200, Carl Eugen Hoyos wrote:
> > > > > Am Mi., 1. Apr. 2020 um 10:57 Uhr schrieb Matthieu Bouron
> > > > > <matthieu.bouron@gmail.com>:
> > > > > >
> > > > > > ---
> > > > > >  libavformat/img2dec.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > > > > > index 40f3e3d499e..93cd51c1932 100644
> > > > > > --- a/libavformat/img2dec.c
> > > > > > +++ b/libavformat/img2dec.c
> > > > > > @@ -775,6 +775,7 @@ static int jpeg_probe(const AVProbeData *p)
> > > > > >                  return 0;
> > > > > >              state = EOI;
> > > > > >              break;
> > > > > > +        case DHT:
> > > > > >          case DQT:
> > > > > >          case APP0:
> > > > > >          case APP1:
> > > > >
> > > > > Please provide a sample file.
> > > >
> > > > This patch does not fix a particular issue. IMHO, we should skip the DHT
> > > > segment just like the other segments (and avoids parsing the data it
> > > > contains) unless there is a particular reason for it ?
> > >
> > > I am (somewhat strongly) against patches that do not fix issues but
> > > Michael will hopefully comment.
> > >
> > > Or are you trying to improve probing speed?
> >
> > I am trying to fix an issues with JPEG files containing MPF metadata
> > appended at the end.
> > Theses files makes the jpeg_probe function fails (returns 0) because the
> > MPF metadata contains JPEG images (so jpeg_probe() finds a SOI marker
> > after EOI). I will send a separate patch for that and provide a
> > sample.
> 
> Are these valid jpeg files?

They are valid JPEGs with MPF metadata
(https://exiftool.org/TagNames/MPF.html).

See
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/259489.html


> 
> > This patch should improve the probing speed by little though.
> 
> Then please measure the speedup and mention it in the
> commit message.

I tried to measure the speed gain using perf record ffmpeg -i [...]
without success (i can't get a meaningful difference between the different
runs which makes sense as the DHT segment size from the file I test against is
419 bytes).

> 
> Thank you, Carl Eugen
> _______________________________________________
> 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 April 1, 2020, 11:23 p.m. UTC | #10
On Wed, Apr 01, 2020 at 03:28:06PM +0200, Matthieu Bouron wrote:
> On Wed, Apr 01, 2020 at 01:04:58PM +0100, Derek Buitenhuis wrote:
> > On 01/04/2020 09:52, Matthieu Bouron wrote:
> > > ---
> > >  libavformat/img2dec.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > 
> > Looks correct, but a commit message explaining why would be good.
> > 
> 
> Would "Skips the DHT segment in jpeg_probe() to avoid looking for JPEG
> markers in the DHT segment data" be OK ?

i suspect that startcodes in a valid DHT are quite rare. It seems they
arent disallowed by the spec, just that a valid table with such values
would be a bit odd. In the bits array it would imply that basically 
all huffman codes are equal length.
So iam not sure this patch will affect any real files, but its more
proper to skip the DHT unless iam missing something.
So with a clear commit message patch should be ok

thx



[...]
diff mbox series

Patch

diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
index 40f3e3d499e..93cd51c1932 100644
--- a/libavformat/img2dec.c
+++ b/libavformat/img2dec.c
@@ -775,6 +775,7 @@  static int jpeg_probe(const AVProbeData *p)
                 return 0;
             state = EOI;
             break;
+        case DHT:
         case DQT:
         case APP0:
         case APP1: