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

[Carl Eugen Hoyos]

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.

> > Shouldn't there be a mpf demuxer that also provides the attachments?
>
> I already have a patch to parse the MPF metadata. The only thing left to
> do is to expose the attachments (it currently only exposes those as
> file offsets in stream metadata). I will send the patch later on.

But this parsing cannot output the image(s) after the main image as
attachments, or do I misunderstand?

Carl Eugen