| Message ID | 20200401085220.293934-1-matthieu.bouron@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/img2dec: skip DHT segment in jpeg_probe() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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".
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
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".
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
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
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".
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
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".
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 --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: