[FFmpeg-devel] [PATCH 2/2 v2] avcodec/mjpegdec: postpone calling ff_get_buffer() until the SOS marker

[Michael Niedermayer]

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 ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20210501/7d3592d2/attachment.sig>