[FFmpeg-devel] [PATCH] avformat/img2dec: return immediately in jpeg_probe() when EOI is found
Matthieu Bouron
matthieu.bouron at gmail.com
Wed Apr 1 22:39:24 EEST 2020
On Wed, Apr 01, 2020 at 09:10:19PM +0200, Carl Eugen Hoyos wrote:
> Am Mi., 1. Apr. 2020 um 21:07 Uhr schrieb Matthieu Bouron
> <matthieu.bouron at gmail.com>:
> >
> > On Wed, Apr 01, 2020 at 07:59:46PM +0200, Carl Eugen Hoyos wrote:
> > > Am Mi., 1. Apr. 2020 um 19:54 Uhr schrieb Matthieu Bouron
> > > <matthieu.bouron at gmail.com>:
> > > >
> > > > On Wed, Apr 01, 2020 at 07:30:55PM +0200, Carl Eugen Hoyos wrote:
> > > > > Am Mi., 1. Apr. 2020 um 19:15 Uhr schrieb Matthieu Bouron
> > > > > <matthieu.bouron at gmail.com>:
> > > > > >
> > > > > > On Wed, Apr 01, 2020 at 06:55:42PM +0200, Carl Eugen Hoyos wrote:
> > > > > > > Am Mi., 1. Apr. 2020 um 18:35 Uhr schrieb Matthieu Bouron
> > > > > > > <matthieu.bouron at gmail.com>:
> > > > > > > >
> > > > > > > > On Wed, Apr 01, 2020 at 06:29:03PM +0200, Carl Eugen Hoyos wrote:
> > > > > > > > > Am Mi., 1. Apr. 2020 um 18:01 Uhr schrieb Matthieu Bouron
> > > > > > > > > <matthieu.bouron at gmail.com>:
> > > > > > > > > >
> > > > > > > > > > Fixes probing of JPEG files containing MPF metadata appended at the end
> > > > > > > > > > of the file.
> > > > > > > > > >
> > > > > > > > > > The MPF metadata chunk can contains multiple JPEG images (thumbnails)
> > > > > > > > > > which makes the jpeg_probe fails (return 0) because it finds a SOI
> > > > > > > > > > marker after EOI.
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > This patch fixes probing of JPEG files containing MPF metadata [1] appended
> > > > > > > > > > at the end of the file.
> > > > > > > > > >
> > > > > > > > > > Such files can be produced by GoPro camera (which produces JPEG files
> > > > > > > > > > with MPF metadata).
> > > > > > > > > >
> > > > > > > > > > You can find a sample here:
> > > > > > > > > > https://0x5c.me/gopro_jpg_mpf_probe_fail
> > > > > > > > >
> > > > > > > > > The sample works fine here with FFmpeg 4.2: Is there a regression?
> > > > > > > >
> > > > > > > > This sample does not work on FFmpeg 4.2 and master if you set a big
> > > > > > > > enought formatprobesize (which matches the file size of the sample):
> > > > > > >
> > > > > > > Inlined is a patch that fixes detection but I wonder if this shouldn't be
> > > > > > > detected as mjpeg instead
> > > > > > >
> > > > > > > Carl Eugen
> > > > > > >
> > > > > > > diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> > > > > > > index 40f3e3d499..4e63b3494e 100644
> > > > > > > --- a/libavformat/img2dec.c
> > > > > > > +++ b/libavformat/img2dec.c
> > > > > > > @@ -737,20 +737,22 @@ static int j2k_probe(const AVProbeData *p)
> > > > > > > static int jpeg_probe(const AVProbeData *p)
> > > > > > > {
> > > > > > > const uint8_t *b = p->buf;
> > > > > > > - int i, state = SOI;
> > > > > > > + int i, state = SOI, MPF = 0;
> > > > > > >
> > > > > > > if (AV_RB16(b) != 0xFFD8 ||
> > > > > > > AV_RB32(b) == 0xFFD8FFF7)
> > > > > > > return 0;
> > > > > > >
> > > > > > > b += 2;
> > > > > > > - for (i = 0; i < p->buf_size - 3; i++) {
> > > > > > > + for (i = 0; i < p->buf_size - 8; i++) {
> > > > > > > int c;
> > > > > > > if (b[i] != 0xFF)
> > > > > > > continue;
> > > > > > > c = b[i + 1];
> > > > > > > switch (c) {
> > > > > > > case SOI:
> > > > > > > + if (state == EOI && MPF)
> > > > > > > + return AVPROBE_SCORE_EXTENSION + 1;
> > > > > >
> > > > > > Your patch might work
> > > > >
> > > > > You could test it.
> > > > >
> > > > > > but the code is trying to find JPEG markers in the
> > > > > > MPF metadata until we reach the first EOI in the MPF JPEG thumbnails,
> > > > >
> > > >
> > > > I meant SOI sorry.
> > > >
> > > > > No, and I am quite sure that I tested this sufficiently.
> > > >
> > > > My point is, we are trying to parse way more data than necessary (the data
> > > > we parse might not be jpeg at this point): the code looks for a marker and
> > > > handles it, if the marker is unknown it repeats this operation for the
> > > > next byte until it reaches the end of the buffer or it finds SOI in the
> > > > EOI+MPF state (with your patch).
> > > >
> > > > Why cant' we end the processing at EOI ? If there is a particular reason
> > > > for that ?
> > >
> > > You claim (that's how I understood it) was that a valid MPF jpg contains a
> > > second (smaller) jpeg immediately after the big jpeg (that contains
> > > an additional jpg in its APP2 data). I test if this is the case.
> > > If you tell me a valid MPF jpg can contain any data after EOI, my
> > > test if of course not correct.
> >
> > The MPF metadata contained in the APP2 segment can reference multiple
> > images. For each image, an offset and a size is given.
> > The multiple images are stored after the main JPEG chunk. So the EOI of
> > the main JPEG chunk might be followed by the SOI of the first referenced
> > image (and this is the case in the sample I provided). So your patch do
> > not introduce any unnecessary processing if there is no padding between
> > EOI and SOI.
> >
> > I wrongly remembered how MPF worked and assumed there was MPF data between
> > the SOI and EOI. Last time I worked on this was two years ago and I am
> > trying to upstream the patches I made back then.
> >
> > Sorry to ask again but, why can't we stop parsing at EOI in the SOS state
> > ?
>
> Sorry for the repeated answer: It would break mjpeg detection.
>
> We can of course stop at EOI for MPF but if every valid MPF file either ends
> at EOI or contains another jpeg image, we are still good.
Ok, I think I understand why it would break the mjpeg detection now. This
is why jpeg_probe() returns 0 if it finds SOI to give a chance to the
mjpeg probe to find something (and because the mjpeg demuxer is supposed
to handle multiple JPEG chunks). Is this correct ?
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
--
Matthieu B.
More information about the ffmpeg-devel
mailing list