[FFmpeg-devel] [PATCH 2/2 v2] avcodec/mjpegdec: postpone calling ff_get_buffer() until the SOS marker
Michael Niedermayer
michael at niedermayer.cc
Sun May 2 17:01:57 EEST 2021
On Sat, May 01, 2021 at 12:18:02PM -0300, James Almer wrote:
> On 5/1/2021 12:06 PM, James Almer wrote:
> > On 5/1/2021 11:30 AM, James Almer wrote:
> > > On 5/1/2021 11:17 AM, Michael Niedermayer wrote:
> > > > On Sat, May 01, 2021 at 09:41:55AM -0300, James Almer wrote:
> > > > > On 5/1/2021 4:01 AM, Michael Niedermayer wrote:
> > > > > > On Thu, Apr 22, 2021 at 02:02:33PM -0300, James Almer wrote:
> > > > > > > With JPEG-LS PAL8 samples, the JPEG-LS extension
> > > > > > > parameters signaled with
> > > > > > > the LSE marker show up after SOF but before SOS. For
> > > > > > > those, the pixel format
> > > > > > > chosen by get_format() in SOF is GRAY8, and then
> > > > > > > replaced by PAL8 in LSE.
> > > > > > > This has not been an issue given both pixel formats
> > > > > > > allocate the second data
> > > > > > > plane for the palette, but after the upcoming soname
> > > > > > > bump, GRAY8 will no longer
> > > > > > > do that. This will result in segfauls when
> > > > > > > ff_jpegls_decode_lse() attempts to
> > > > > > > write the palette on a buffer originally allocated as a GRAY8 one.
> > > > > > >
> > > > > > > Work around this by calling ff_get_buffer() after
> > > > > > > the actual pixel format is
> > > > > > > known.
> > > > > >
> > > > > > What if the LSE occurs after the SOS ?
> > > > > > What if there are 2 LSE ?
> > > > > > It seems allowed by the specification
> > > > > >
> > > > > > "The LSE marker segment may be present where tables or
> > > > > > miscellaneous marker segments may appear. If tables
> > > > > > specified
> > > > > > in this marker segment for a given table ID appear
> > > > > > more than once, each specification shall replace the
> > > > > > previous
> > > > > > specification."
> > > > > >
> > > > > > Maybe iam missing something but a implemenattion of this would seem to
> > > > > > require scanning through the image, finding all LSE and
> > > > > > checking if they all
> > > > > > are compatible with the PAL8 format before allocating the image in SOF
> > > > > >
> > > > > > The implemenattion here which delays allocation slightly seems to make
> > > > > > the code much more unstable allowing the frame
> > > > > > configuration or allocation
> > > > > > to be partly done, double done, redone, without the matching partner.
> > > > > > Also it does not seem to implement the related
> > > > > > specification completely.
> > > > >
> > > > > Well, it was a hack to replace a previous hack (allocate a
> > > > > gray buffer then
> > > > > silently treat it as PAL8 once an LSE showed up). It's no
> > > > > wonder it may not
> > > > > perfectly follow the spec and consider all potential cases.
> > > >
> > > > yes and i wouldnt mind but the new hack is leading to crashes ...
> > > > and the fixes to the crashes all in all start looking a bit ugly
> > > > and iam not
> > > > sure if any more remain. So id like to take a bit a step back
> > > > here and see
> > > > if there isnt a easier/cleaner solution.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > A complete implemenattion may reqire RGB24 or 48 to be
> > > > > > selected in some cases
> > > > > > like when each scan uses its own palette, which unless
> > > > > > iam missing something
> > > > > > seems allowed.
> > > > > > But then maybe iam missing something, in which case please correct me!
> > > > >
> > > > > You're probably not wrong, since unlike me you actually know
> > > > > this code and
> > > > > format. I tried to workaround an issue and it seemed to work
> > > > > and not break
> > > > > any file anyone here could test.
> > > > >
> > > > > If you think it's not good enough then just revert it and
> > > > > maybe allocate the
> > > > > data[1] pointer for the palette with av_buffer_alloc(),
> > > > > get_buffer2() custom
> > > > > implementations be damned. But i don't think the previous
> > > > > code even did half
> > > > > the things you listed above.
> > > >
> > > > how can i reproduce the issue you where trying to fix with this patch ?
> > >
> > > It's the crash you reported in
> > > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-April/279350.html
> > >
> > > The explanation of why it started crashing is in c8197f73e6.
> > > Basically, since GRAY8 no longer unnecessarily allocates a fixed
> > > palette, the old hack of changing pix_fmt from GRAY8 to PAL8 after
> > > having called ff_get_buffer() with the former, editing the palette
> > > plane buffer, and have things magically work is not possible
> > > anymore, because no palette buffer was ever allocated.
> > > ff_get_buffer() is meant to be called once the actual pix_fmt is
> > > known to correctly allocate all plane buffers.
> >
> > Another possible solution instead of my suggestion above (revert and
> > then manually allocate a palette plane buffer, which can potentially
> > piss off custom get_buffer2() implementations), is to revert and then
> > initially set the pix_fmt in ff_mjpeg_decode_sof() to PAL8 instead of
> > GRAY8 for jpegls, and replace it with the latter if no LSE is ever
> > parsed (or if one is parsed and it doesn't contain a palette).
> > It's also hacky, basically the inverse of what it used to be, but at
> > least it ensures the palette plane is allocated by get_buffer2(), which
> > will ultimately be ignored and freed.
>
> Which is to say, revert fb5e2d7112 c8197f73e6 and try the something like
> the following:
>
> > $ git diff
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > index 7c66ff8637..70e674758d 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -681,10 +681,8 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
> > } else if (s->nb_components != 1) {
> > av_log(s->avctx, AV_LOG_ERROR, "Unsupported number of components %d\n", s->nb_components);
> > return AVERROR_PATCHWELCOME;
> > - } else if (s->palette_index && s->bits <= 8)
> > + } else if (s->bits <= 8)
> > s->avctx->pix_fmt = AV_PIX_FMT_PAL8;
> > - else if (s->bits <= 8)
> > - s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > else
> > s->avctx->pix_fmt = AV_PIX_FMT_GRAY16;
> > }
> > @@ -733,6 +731,13 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
> > for (i = 0; i < 4; i++)
> > s->linesize[i] = s->picture_ptr->linesize[i] << s->interlaced;
> >
> > + if (s->ls && !s->palette_index && s->bits <= 8) {
> > + // The LSE marker will change this back to PAL8 if there's a palette.
> > + // By calling ff_get_buffer() with PAL8 pix_fmt we ensured a plane for
> > + // it was allocated.
> > + s->avctx->pix_fmt = s->picture_ptr->format = AV_PIX_FMT_GRAY8;
> > + avpriv_set_systematic_pal2((uint32_t *)s->picture_ptr->data[1], s->picture_ptr->format);
> > + }
> > ff_dlog(s->avctx, "%d %d %d %d %d %d\n",
> > s->width, s->height, s->linesize[0], s->linesize[1],
> > s->interlaced, s->avctx->height);
>
> fate-jpegls passes, so even if also hacky, it may be better than postponing
> the call to ff_get_buffer().
ive posted a patchset with a simple solution, which i hope doesnt do hacky
things with teh API.
note, in case the 2 reverted patches contain anything thats usefull outside
the new fix these parts of course should be reapplied or it should be told
to me to remove them from the revert
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210502/c2deb2cf/attachment.sig>
More information about the ffmpeg-devel
mailing list