[FFmpeg-devel] [PATCH] avformat/img2dec: return immediately in jpeg_probe() when EOI is found

[Carl Eugen Hoyos]

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.

Carl Eugen