| Message ID | 20210422170233.41884-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
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. 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! thx [...]
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. > > 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. > > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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 [...]
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. > > thx > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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. > >> >> thx >> >> [...] >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >
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().
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 [...]
diff --git a/libavcodec/jpeglsdec.c b/libavcodec/jpeglsdec.c index e17de09e9f..4ab0cb5bd8 100644 --- a/libavcodec/jpeglsdec.c +++ b/libavcodec/jpeglsdec.c @@ -108,9 +108,8 @@ int ff_jpegls_decode_lse(MJpegDecodeContext *s) if (s->palette_index > maxtab) return AVERROR_INVALIDDATA; - if ((s->avctx->pix_fmt == AV_PIX_FMT_GRAY8 || s->avctx->pix_fmt == AV_PIX_FMT_PAL8) && - (s->picture_ptr->format == AV_PIX_FMT_GRAY8 || s->picture_ptr->format == AV_PIX_FMT_PAL8)) { - uint32_t *pal = (uint32_t *)s->picture_ptr->data[1]; + if (s->avctx->pix_fmt == AV_PIX_FMT_GRAY8 || s->avctx->pix_fmt == AV_PIX_FMT_PAL8) { + uint32_t *pal = s->palette; int shift = 0; if (s->avctx->bits_per_raw_sample > 0 && s->avctx->bits_per_raw_sample < 8) { @@ -118,7 +117,6 @@ int ff_jpegls_decode_lse(MJpegDecodeContext *s) shift = 8 - s->avctx->bits_per_raw_sample; } - s->picture_ptr->format = s->avctx->pix_fmt = AV_PIX_FMT_PAL8; for (i=s->palette_index; i<=maxtab; i++) { uint8_t k = i << shift; diff --git a/libavcodec/mjpegbdec.c b/libavcodec/mjpegbdec.c index 7666674908..890befb522 100644 --- a/libavcodec/mjpegbdec.c +++ b/libavcodec/mjpegbdec.c @@ -55,6 +55,7 @@ static int mjpegb_decode_frame(AVCodecContext *avctx, buf_ptr = buf; buf_end = buf + buf_size; + s->seen_sof = 0; s->got_picture = 0; s->adobe_transform = -1; diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index 7c7cc20af8..42e6170469 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -138,6 +138,7 @@ av_cold int ff_mjpeg_decode_init(AVCodecContext *avctx) s->buffer = NULL; s->start_code = -1; s->first_picture = 1; + s->seen_sof = 0; s->got_picture = 0; s->orig_height = avctx->coded_height; avctx->chroma_sample_location = AVCHROMA_LOC_CENTER; @@ -429,6 +430,7 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s) memcpy(s->h_count, h_count, sizeof(h_count)); memcpy(s->v_count, v_count, sizeof(v_count)); s->interlaced = 0; + s->seen_sof = 0; s->got_picture = 0; /* test interlaced mode */ @@ -681,11 +683,13 @@ 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) - s->avctx->pix_fmt = AV_PIX_FMT_PAL8; - else if (s->bits <= 8) - s->avctx->pix_fmt = AV_PIX_FMT_GRAY8; - else + } else if (s->bits <= 8) { + avpriv_set_systematic_pal2(s->palette, s->avctx->pix_fmt); + if (s->palette_index) + s->avctx->pix_fmt = AV_PIX_FMT_PAL8; + else + s->avctx->pix_fmt = AV_PIX_FMT_GRAY8; + } else s->avctx->pix_fmt = AV_PIX_FMT_GRAY16; } @@ -719,26 +723,13 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s) if (s->avctx->skip_frame == AVDISCARD_ALL) { s->picture_ptr->pict_type = AV_PICTURE_TYPE_I; s->picture_ptr->key_frame = 1; - s->got_picture = 1; + s->seen_sof = 1; return 0; } - - av_frame_unref(s->picture_ptr); - if (ff_get_buffer(s->avctx, s->picture_ptr, AV_GET_BUFFER_FLAG_REF) < 0) - return -1; - s->picture_ptr->pict_type = AV_PICTURE_TYPE_I; - s->picture_ptr->key_frame = 1; - s->got_picture = 1; - - for (i = 0; i < 4; i++) - s->linesize[i] = s->picture_ptr->linesize[i] << s->interlaced; - - 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); - } + s->seen_sof = 1; + if ((s->rgb && !s->lossless && !s->ls) || (!s->rgb && s->ls && s->nb_components > 1) || (s->avctx->pix_fmt == AV_PIX_FMT_PAL8 && !s->ls) @@ -764,18 +755,6 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s) memset(s->coefs_finished, 0, sizeof(s->coefs_finished)); } - if (s->avctx->hwaccel) { - s->hwaccel_picture_private = - av_mallocz(s->avctx->hwaccel->frame_priv_data_size); - if (!s->hwaccel_picture_private) - return AVERROR(ENOMEM); - - ret = s->avctx->hwaccel->start_frame(s->avctx, s->raw_image_buffer, - s->raw_image_buffer_size); - if (ret < 0) - return ret; - } - return 0; } @@ -1630,12 +1609,44 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext *s, const uint8_t *mb_bitmask, const int block_size = s->lossless ? 1 : 8; int ilv, prev_shift; - if (!s->got_picture) { + if (!s->seen_sof) { av_log(s->avctx, AV_LOG_WARNING, "Can not process SOS before SOF, skipping\n"); return -1; } + if (!s->got_picture || !s->interlaced || !(s->bottom_field == !s->interlace_polarity)) { + av_frame_unref(s->picture_ptr); + if (ff_get_buffer(s->avctx, s->picture_ptr, AV_GET_BUFFER_FLAG_REF) < 0) + return -1; + s->picture_ptr->pict_type = AV_PICTURE_TYPE_I; + s->picture_ptr->key_frame = 1; + + for (i = 0; i < 4; i++) + s->linesize[i] = s->picture_ptr->linesize[i] << s->interlaced; + + if (s->picture_ptr->format == AV_PIX_FMT_PAL8) + memcpy(s->picture_ptr->data[1], s->palette, sizeof(s->palette)); + + s->got_picture = 1; + + 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); + } + + if (s->avctx->hwaccel && !s->hwaccel_picture_private) { + s->hwaccel_picture_private = + av_mallocz(s->avctx->hwaccel->frame_priv_data_size); + if (!s->hwaccel_picture_private) + return AVERROR(ENOMEM); + + ret = s->avctx->hwaccel->start_frame(s->avctx, s->raw_image_buffer, + s->raw_image_buffer_size); + if (ret < 0) + return ret; + } + if (reference) { if (reference->width != s->picture_ptr->width || reference->height != s->picture_ptr->height || @@ -2561,6 +2572,7 @@ eoi_parser: break; } if (avctx->skip_frame == AVDISCARD_ALL) { + s->seen_sof = 0; s->got_picture = 0; ret = AVERROR(EAGAIN); goto the_end_no_picture; @@ -2574,6 +2586,7 @@ eoi_parser: } if ((ret = av_frame_ref(frame, s->picture_ptr)) < 0) return ret; + s->seen_sof = 0; s->got_picture = 0; frame->pkt_dts = s->pkt->dts; @@ -2634,6 +2647,7 @@ skip: av_log(avctx, AV_LOG_FATAL, "No JPEG data found in image\n"); return AVERROR_INVALIDDATA; fail: + s->seen_sof = 0; s->got_picture = 0; return ret; the_end: @@ -2924,6 +2938,7 @@ av_cold int ff_mjpeg_decode_end(AVCodecContext *avctx) static void decode_flush(AVCodecContext *avctx) { MJpegDecodeContext *s = avctx->priv_data; + s->seen_sof = 0; s->got_picture = 0; s->smv_next_frame = 0; diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h index 2400a179f1..71cacb0b27 100644 --- a/libavcodec/mjpegdec.h +++ b/libavcodec/mjpegdec.h @@ -109,6 +109,7 @@ typedef struct MJpegDecodeContext { int last_dc[MAX_COMPONENTS]; /* last DEQUANTIZED dc (XXX: am I right to do that ?) */ AVFrame *picture; /* picture structure */ AVFrame *picture_ptr; /* pointer to picture structure */ + int seen_sof; ///< we found a SOF. int got_picture; ///< we found a SOF and picture is valid, too. int linesize[MAX_COMPONENTS]; ///< linesize << interlaced int8_t *qscale_table; @@ -165,7 +166,9 @@ typedef struct MJpegDecodeContext { enum AVPixelFormat hwaccel_sw_pix_fmt; enum AVPixelFormat hwaccel_pix_fmt; void *hwaccel_picture_private; + struct JLSState *jls_state; + uint32_t palette[AVPALETTE_COUNT]; } MJpegDecodeContext; int ff_mjpeg_build_vlc(VLC *vlc, const uint8_t *bits_table, diff --git a/libavcodec/mxpegdec.c b/libavcodec/mxpegdec.c index 763ce5871d..617da52cf0 100644 --- a/libavcodec/mxpegdec.c +++ b/libavcodec/mxpegdec.c @@ -197,7 +197,7 @@ static int mxpeg_decode_frame(AVCodecContext *avctx, buf_end = buf + buf_size; jpg->got_picture = 0; s->got_mxm_bitmask = 0; - s->got_sof_data = !!s->got_sof_data; + jpg->seen_sof = s->got_sof_data = !!s->got_sof_data; while (buf_ptr < buf_end) { start_code = ff_mjpeg_find_marker(jpg, &buf_ptr, buf_end, &unescaped_buf_ptr, &unescaped_buf_size);
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. Signed-off-by: James Almer <jamrial@gmail.com> --- Now splitting the variable that signals that a frame was allocated and that SOF data was successfully parsed. libavcodec/jpeglsdec.c | 6 +-- libavcodec/mjpegbdec.c | 1 + libavcodec/mjpegdec.c | 83 +++++++++++++++++++++++++----------------- libavcodec/mjpegdec.h | 3 ++ libavcodec/mxpegdec.c | 2 +- 5 files changed, 56 insertions(+), 39 deletions(-)