Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On Wed, Apr 24, 2024 at 01:20:02PM +0200, Jerome Martinez wrote: > Hi, I'm bumping this patch proposal for avoiding a situation where FFmpeg > skips half the visual content when 2 jpeg2000 codestreams are present in one > frame. I re-reviewed this discussion and think I answered all concerns. I'm > hesitant with patch v3 because I consider that touching frame_worker_thread > for this feature is not so useful but I am ok with either patch v2 or v3. > Thanks much, > Jérôme I hope i applied the correct patch: libavcodec/avcodec.h | 14 ++ libavcodec/codec_par.c | 3 +++ libavcodec/codec_par.h | 5 + libavcodec/decode.c| 3 +++ libavcodec/defs.h | 7 +++ libavcodec/jpeg2000dec.c | 73 +++-- libavcodec/jpeg2000dec.h | 6 ++ libavcodec/pthread_frame.c | 3 +++ libavformat/mxfdec.c | 14 ++ 9 files changed, 118 insertions(+), 10 deletions(-) but that with: ./ffmpeg -i ~/tickets/1102/ntsc2.mxf -bitexact -vframes 1 /tmp/file.bmp segafaults: ==1031087== Invalid write of size 2 ==1031087==at 0x9D758F: jpeg2000_decode_tile (in ffmpeg/ffmpeg_g) ==1031087==by 0x7C3820: avcodec_default_execute2 (in ffmpeg/ffmpeg_g) ==1031087==by 0x9DCE6E: jpeg2000_decode_frame (in ffmpeg/ffmpeg_g) ==1031087==by 0xAF912F: frame_worker_thread (in ffmpeg/ffmpeg_g) ==1031087==by 0x4A00608: start_thread (pthread_create.c:477) ==1031087==by 0x8104352: clone (clone.S:95) ==1031087== Address 0x0 is not stack'd, malloc'd or (recently) free'd [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
ons 2024-04-24 klockan 13:20 +0200 skrev Jerome Martinez: > Hi, I'm bumping this patch proposal for avoiding a situation where > FFmpeg skips half the visual content when 2 jpeg2000 codestreams are > present in one frame. I re-reviewed this discussion and think I > answered > all concerns. I'm hesitant with patch v3 because I consider that > touching frame_worker_thread for this feature is not so useful but I > am > ok with either patch v2 or v3. Did you look at doing this further up, say in demux.c? Because adding codec-specific logic for this seems wrong, when J2K is far from the only codec that can be used like this in MXF.. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
Hi, I'm bumping this patch proposal for avoiding a situation where FFmpeg skips half the visual content when 2 jpeg2000 codestreams are present in one frame. I re-reviewed this discussion and think I answered all concerns. I'm hesitant with patch v3 because I consider that touching frame_worker_thread for this feature is not so useful but I am ok with either patch v2 or v3. Thanks much, Jérôme On 20/02/2024 16:07, Jerome Martinez wrote: Attached is an updated version of the patch proposal. About the idea to keep separate fields in the output AVFrame, I note from the discussion that it is commonly accepted that up to now it is expected that the AVPacket contains what is in the MXF element and that the AVFrame contains a frame and never a field, and additionally I see in e.g. mpeg12dec.c that the decoder interleaves separate fields: mb_y <<= field_pic; if (s2->picture_structure == PICT_BOTTOM_FIELD) mb_y++; And mpegvideo_parser.c creates a AVPacket with both fields in AVPacket even if they are separated, this patch keeps the AVPacket from e.g. MXF with both fields in it and does something similar to what do other decoders with separate fields in the output AVFRame. About the detection of the 2 separated fields in 1 packet in the MXF file (I2 mode), it is doable in the example file provided in the first patch proposal to catch it by checking the essence label but other files I have don't have the specific essence label (they use the "undefined" essence label, legit) so IMO we should not rely on it and there is actually no practical advantage in catching that from the container. In practice this new patch proposal is slightly more complex, with one recursive call to jpeg2000_decode_frame() when there are 2 codestreams in 1 AVPacket, but it has a negligible performance impact (few comparisons and bool checks) when there is only one codestream in the AVPacket (the currently only supported method, remind that currently FFmpeg completely discards the 2nd codestream present in the AVPacket) and it relies on the current jpeg2000_read_main_headers() function for catching the end of the first codestream (safer than the quick find of EOC/SOC in the first version). It also changes the jpeg2000_decode_frame return value to the count of bytes parsed, it seems that it was what is expected but in practice it was not doing that, fixing the return value could be a separate patch if preferred. Jérôme On 02/02/2024 16:55, Jerome Martinez wrote: Before this patch, the FFmpeg MXF parser correctly detects content with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding that the indicated height is the height of 1 field only so overwrites the frame size info with e.g. 720x243, and also completely discards the second frame, which lead to the decoding of only half of the stored content as "progressive" 720x243 flagged interlaced. Example file: https://www.digitizationguidelines.gov/guidelines/MXF_sampleFiles/RDD48-sample12-gf-jpeg2000-ntsc-4.2.zip Before this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 tbr, 29.97 tbn, 29.97 tbc After this patch: Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first (swapped)), 720x486, lossless, SAR 9:10 DAR 4:3, 29.97 fps, 29.97 tbr, 29.97 tbn ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
ons 2024-02-21 klockan 15:27 +0100 skrev Jerome Martinez: > On 21/02/2024 14:11, Tomas Härdin wrote: > > > mxfdec can detect cases where there will be two separate fields in > > one > > KLV, > > In practice the issue is not to detect I2 case in mxfdec (it saves us > only a little during probing of the first frame, but I could add such > signaling in a patch after this one because it is actually > independent > from the management of 2 fields in the decoder if we want to support > buggy files), the issue is to split per field in mxfdec. > > SMPTE ST 422 extracts: > "Users are cautioned that the code values for SOC and EOC are not > protected and can occur within the image size marker segment (SIZ), > quantization marker segment (QCD), comment marker segment (COM) and > other places." > "Decoders can derive the bytestream offsets of each field by > analysing > the code stream format within the essence element as described in > ISO/IEC 15444-1." > > Note that this MXF + jp2k spec hints that separating fields should be > done in the decoder, not the demuxer. We already have a j2k parser that could be pressed into service for this. But perhaps parsing is not necessary, see below. My main concern is that interlacing work the same for all codecs that mxfdec supports, expecially rawvideo. > It is impossible to split per field in a codec-neutral manner due to > lack of metadata for that in MXF, and doing that in mxfdec implies to > duplicate jp2k header parser code in mxfdec, and to add a similar > parser > per supported video format in the future. We do have a bit of duplicate parsing in mxfdec (and mxfenc) already, which I don't like. So I share your concern. > > and the decoder(s) can if I'm not mistaken be instructed to decode > > into an AVFrame with stride and offset set up for interlaced > > decoding. > > > I checked the MPEG-2 Video decoder and it does not do what you say I didn't say that it does, I said that we could do that. If we conform all wrappings to MIXED_FIELDS this is probably fine. I think it's been well established that FFmpeg is MIXED_FIELDS internally. > , it does what I do with this patch: > - mpegvideo_parser (so the parser of raw files, equivalent to mxfdec) > understands that the stream has 2 separate fields and puts them in 1 > single AVPacket. It could separate them put it doesn't. mpeg2 has a concept of fields that j2k doesn't. Again, we're not really talking about j2k here but MXF > > It should be possible to have ffmpeg set up the necessary plumbing > > for > > this. > > But is it how it works elsewhere in FFmpeg? Would such complex and > deep > modifications be accepted by others? Good question. I would propose something like the following: 1) detect the use of SEPARATE_FIELDS and set a flag in AVStream 2) allocate AVFrame for the size of the resulting *frame* 3a) if the codec is inherently interlaced, call the decoder once 3b) if the codec is not inherently interlaced, call the decoder twice, with appropriate stride, and keep track of the number of bytes decoded so far so we know what offset to start the second decode from The codecs for which 3b) applies include at least: * jpeg2000 * ffv1 * rawvideo * tiff /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
On 21/02/2024 14:11, Tomas Härdin wrote: mxfdec can detect cases where there will be two separate fields in one KLV, In practice the issue is not to detect I2 case in mxfdec (it saves us only a little during probing of the first frame, but I could add such signaling in a patch after this one because it is actually independent from the management of 2 fields in the decoder if we want to support buggy files), the issue is to split per field in mxfdec. SMPTE ST 422 extracts: "Users are cautioned that the code values for SOC and EOC are not protected and can occur within the image size marker segment (SIZ), quantization marker segment (QCD), comment marker segment (COM) and other places." "Decoders can derive the bytestream offsets of each field by analysing the code stream format within the essence element as described in ISO/IEC 15444-1." Note that this MXF + jp2k spec hints that separating fields should be done in the decoder, not the demuxer. It is impossible to split per field in a codec-neutral manner due to lack of metadata for that in MXF, and doing that in mxfdec implies to duplicate jp2k header parser code in mxfdec, and to add a similar parser per supported video format in the future. and the decoder(s) can if I'm not mistaken be instructed to decode into an AVFrame with stride and offset set up for interlaced decoding. I checked the MPEG-2 Video decoder and it does not do what you say, it does what I do with this patch: - mpegvideo_parser (so the parser of raw files, equivalent to mxfdec) understands that the stream has 2 separate fields and puts them in 1 single AVPacket. It could separate them put it doesn't. - mpeg12dec (equivalent to jpeg2000dec) understands that there are 2 separate fields and applies a stride and offset internally and outputs a frame in AVFrame, not a field. It could provide fields but it doesn't. I checked only what I know well, MPEG-2 Video, maybe it is done the way you indicate elsewhere, currently I see no example about the idea that stride and offset should be provided by the demuxer, would you mind to show some code in FFmpeg doing this way? And stride and offset in signaling would mean that the target decoder must support stride and offset in signaling (including jpeg2000dec so getting a part of my current patch anyway), so no more "universal" as far as I understand. Also: Other comments say that AVPacket is expected to receive the packet as it in the container so no split of packet. Other comments say that AVFrame is expected to receive a frame and never, at least for the moment, a field. I don't see how to respect theses 2 indications, while mpegvideo_parser.c + mpeg12dec.c are conforming to theses indications because they do as in this patch. My understanding of other comments is that my patch proposal is the right way to do it, I am lost here with contradictions in the indicated path for the support of such file. e.g. https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/321540.html It should be possible to have ffmpeg set up the necessary plumbing for this. But is it how it works elsewhere in FFmpeg? Would such complex and deep modifications be accepted by others? Please confirm that it would be accepted before I go so deep in the changes of FFmpeg code, or please indicate where such tip is already stored in codec context. And it is not only plumbing, it is implementing a jp2k parser in mxfdec for splitting fields, impossible otherwise to split the fields and it is NOT for all codec at once (which is impossible in practice due to the need to have codec dependent split of fields). And in practice, what it the inconvenience to do this way, with very few modifications and in a similar manner of what is done elsewhere in FFmpeg with separate fields? Especially with patch v2 which adds nearly (a couple of comparisons between integers) no performance impact to previously supported content. As a summary, IMO this patch is conform to what is done elsewhere in FFmpeg for separated fiels e.g. in mpeg12dec and is conform to what is hinted in the specification by checking the 2nd codestream offset in the decoder rather than in the demuxer. Jérôme ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
This is still J2K specific rather than handling it properly for all codecs all at once. mxfdec can detect cases where there will be two separate fields in one KLV, and the decoder(s) can if I'm not mistaken be instructed to decode into an AVFrame with stride and offset set up for interlaced decoding. It should be possible to have ffmpeg set up the necessary plumbing for this. /Tomas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
Attached is an updated version of the patch proposal.
About the idea to keep separate fields in the output AVFrame, I note
from the discussion that it is commonly accepted that up to now it is
expected that the AVPacket contains what is in the MXF element and that
the AVFrame contains a frame and never a field, and additionally I see
in e.g. mpeg12dec.c that the decoder interleaves separate fields:
mb_y <<= field_pic;
if (s2->picture_structure == PICT_BOTTOM_FIELD)
mb_y++;
And mpegvideo_parser.c creates a AVPacket with both fields in AVPacket
even if they are separated, this patch keeps the AVPacket from e.g. MXF
with both fields in it and does something similar to what do other
decoders with separate fields in the output AVFRame.
About the detection of the 2 separated fields in 1 packet in the MXF
file (I2 mode), it is doable in the example file provided in the first
patch proposal to catch it by checking the essence label but other files
I have don't have the specific essence label (they use the "undefined"
essence label, legit) so IMO we should not rely on it and there is
actually no practical advantage in catching that from the container.
In practice this new patch proposal is slightly more complex, with one
recursive call to jpeg2000_decode_frame() when there are 2 codestreams
in 1 AVPacket, but it has a negligible performance impact (few
comparisons and bool checks) when there is only one codestream in the
AVPacket (the currently only supported method, remind that currently
FFmpeg completely discards the 2nd codestream present in the AVPacket)
and it relies on the current jpeg2000_read_main_headers() function for
catching the end of the first codestream (safer than the quick find of
EOC/SOC in the first version).
It also changes the jpeg2000_decode_frame return value to the count of
bytes parsed, it seems that it was what is expected but in practice it
was not doing that, fixing the return value could be a separate patch if
preferred.
Jérôme
On 02/02/2024 16:55, Jerome Martinez wrote:
Before this patch, the FFmpeg MXF parser correctly detects content
with 2 fields in 1 AVPacket as e.g. interlaced 720x486 but the FFmpeg
JPEG 2000 decoder reads the JPEG 2000 SIZ header without understanding
that the indicated height is the height of 1 field only so overwrites
the frame size info with e.g. 720x243, and also completely discards
the second frame, which lead to the decoding of only half of the
stored content as "progressive" 720x243 flagged interlaced.
Example file:
https://www.digitizationguidelines.gov/guidelines/MXF_sampleFiles/RDD48-sample12-gf-jpeg2000-ntsc-4.2.zip
Before this patch:
Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first
(swapped)), 720x243, lossless, SAR 9:20 DAR 4:3, 29.97 tbr, 29.97 tbn,
29.97 tbc
After this patch:
Stream #0:0: Video: jpeg2000, yuv422p10le(bottom coded first
(swapped)), 720x486, lossless, SAR 9:10 DAR 4:3, 29.97 fps, 29.97 tbr,
29.97 tbn
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
From 6d47d60178f50091afc3b7193b752186603efc6a Mon Sep 17 00:00:00 2001
From: Jerome Martinez
Date: Tue, 20 Feb 2024 16:04:11 +0100
Subject: [PATCH] avcodec/jpeg2000dec: support of 2 fields in 1 AVPacket
---
libavcodec/jpeg2000dec.c | 78 ++--
libavcodec/jpeg2000dec.h | 5
2 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 691cfbd891..28a3e11020 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -194,6 +194,8 @@ static int get_siz(Jpeg2000DecoderContext *s)
int ret;
int o_dimx, o_dimy; //original image dimensions.
int dimx, dimy;
+int previous_width = s->width;
+int previous_height = s->height;
if (bytestream2_get_bytes_left(&s->g) < 36) {
av_log(s->avctx, AV_LOG_ERROR, "Insufficient space for SIZ\n");
@@ -211,7 +213,7 @@ static int get_siz(Jpeg2000DecoderContext *s)
s->tile_offset_y = bytestream2_get_be32u(&s->g); // YT0Siz
ncomponents = bytestream2_get_be16u(&s->g); // CSiz
-if (av_image_check_size2(s->width, s->height, s->avctx->max_pixels,
AV_PIX_FMT_NONE, 0, s->avctx)) {
+if (av_image_check_size2(s->width, s->height << (s->has_2_fields &&
s->height >= 0), s->avctx->max_pixels, AV_PIX_FMT_NONE, 0, s->avctx)) {
avpriv_request_sample(s->avctx, "Large Dimensions");
return AVERROR_PATCHWELCOME;
}
@@ -301,6 +303,20 @@ static int get_siz(Jpeg2000DecoderContext *s)
return AVERROR(ENOMEM);
}
+/* management of frames having 2 separate codestreams */
+if (s->has_2_fields) {
+s->height <<= 1;
+s->image_offset_y <<= 1;
+s->tile_offset_y <<= 1;
+
