| Message ID | CAGKSLywdGwJ=B1fa_1Ns5Ac-ZaxecB7PUFuiTYnNFA9=+NYF-A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Patch JPEG2000 Parser: Fix parsing of tile-part header | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Wed, Jul 21, 2021 at 7:02 PM Shaun Simpson <shauns2029@gmail.com> wrote: > > Please find my patch attached. > > libavcodec/jpeg2000_parser: Fix parsing of tile-part header, and frames > where the end of frame marker is at the end of the buffer. > > Fixes playback of Sol Levante MXF JPEG2000 file: > VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf > > http://download.opencontent.netflix.com.s3.amazonaws.com/SolLevante/imf/SolLevante_IMF_DolbyVision_PQP3D65_UHD_24fps.zip > > Bug Summary: > When playing back VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf, I noticed > a regression between release 4.2.4 and release 4.3.2. Release 4.3.2 skips > some frames. This can be seen in the first 15 seconds of the video. The > Netflix logo should fade out. > > This regression is in the master branch and was introduced by commit > d09c356 which adds a JPEG2000 parser. I have also tested this patch against > my 3 other JPEG2000 samples. > > The patches are against master branch 13ec662 and fix playback of this file. > First of all, thank you for poking at the j2k parser. I have seen similar complaints from multiple people handling J2K in MXF, and after linking these patches to such people they seem to fix the issue. It also seems like it would make sense to have a minimal sample of such bit stream type in our FATE test set so we can figure out if the parsing of such streams fails in the future. > 1. Fixed a off by one error when calculating the number of bytes to skip. > > 2. Added a 'continue' to the Tile part header reading, to skip the reaming > paring code during a header read. > > 3. To deal with the end of frame tag being at the very end of the buffer, > return the frame size during the current loop iteration. > As someone who unfortunately lacks the context for JPEG2000 bit streams, can you give any pointers towards helping with review? Also others may consider this as a flag that if there is JPEG2000 bit stream knowledge about, you are free to poke at this one :) . It seems to fix decoding properly. > Is the PARSER_FLAG_COMPLETE_FRAMES flag supposed to be set for the Netflix > sample? The files playback correctly if the flag is set, as each buffer > contains a complete frame, but it is not set for the test case above. > It seems to be set if the AVStream's internal->need_parsing is set to AVSTREAM_PARSE_HEADERS , which is the default behavior in avformat/mxfdec. Jan
Hi Jan, I am glad these patches help, thank you for looking into this. > It also seems like it would make sense to have a minimal sample of > such bit stream type in our FATE test set so we can figure out if the > parsing of such streams fails in the future. Yes, I have been looking at samples for FATE as they are required, but the size of these files is problematic. If I try and use FFmpeg to sample the beginning of the Sol Levante, the issue "goes away". The best I could do was to use "dd" to truncate the file for my testing. I will look at using Da Vinci Resolve to create a sample, what sample size should I be considering for FATE? > As someone who unfortunately lacks the context for JPEG2000 bit > streams, can you give any pointers towards helping with review? Yes, I would appreciate a second pair of eyes. I used following document: T-REC-T.800-201511-S!!PDF-E.pdf https://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-T.800-201511-S!!PDF-E&type=items > Also others may consider this as a flag that if there is JPEG2000 bit > stream knowledge about, you are free to poke at this one :) . It seems > to fix decoding properly. Yes, I am willing. I can't guarantee my response times but I would like to get J2K playback working as robustly as possible. > It seems to be set if the AVStream's internal->need_parsing is set to > AVSTREAM_PARSE_HEADERS , which is the default behavior in > avformat/mxfdec. Thank you for this information. Shaun On Sat, Jul 24, 2021 at 7:01 PM Jan Ekström <jeebjp@gmail.com> wrote: > On Wed, Jul 21, 2021 at 7:02 PM Shaun Simpson <shauns2029@gmail.com> > wrote: > > > > Please find my patch attached. > > > > libavcodec/jpeg2000_parser: Fix parsing of tile-part header, and frames > > where the end of frame marker is at the end of the buffer. > > > > Fixes playback of Sol Levante MXF JPEG2000 file: > > VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf > > > > > http://download.opencontent.netflix.com.s3.amazonaws.com/SolLevante/imf/SolLevante_IMF_DolbyVision_PQP3D65_UHD_24fps.zip > > > > Bug Summary: > > When playing back VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf, I > noticed > > a regression between release 4.2.4 and release 4.3.2. Release 4.3.2 skips > > some frames. This can be seen in the first 15 seconds of the video. The > > Netflix logo should fade out. > > > > This regression is in the master branch and was introduced by commit > > d09c356 which adds a JPEG2000 parser. I have also tested this patch > against > > my 3 other JPEG2000 samples. > > > > The patches are against master branch 13ec662 and fix playback of this > file. > > > > First of all, thank you for poking at the j2k parser. I have seen > similar complaints from multiple people handling J2K in MXF, and after > linking these patches to such people they seem to fix the issue. > > It also seems like it would make sense to have a minimal sample of > such bit stream type in our FATE test set so we can figure out if the > parsing of such streams fails in the future. > > > 1. Fixed a off by one error when calculating the number of bytes to skip. > > > > 2. Added a 'continue' to the Tile part header reading, to skip the > reaming > > paring code during a header read. > > > > 3. To deal with the end of frame tag being at the very end of the buffer, > > return the frame size during the current loop iteration. > > > > As someone who unfortunately lacks the context for JPEG2000 bit > streams, can you give any pointers towards helping with review? > > Also others may consider this as a flag that if there is JPEG2000 bit > stream knowledge about, you are free to poke at this one :) . It seems > to fix decoding properly. > > > Is the PARSER_FLAG_COMPLETE_FRAMES flag supposed to be set for the > Netflix > > sample? The files playback correctly if the flag is set, as each buffer > > contains a complete frame, but it is not set for the test case above. > > > > It seems to be set if the AVStream's internal->need_parsing is set to > AVSTREAM_PARSE_HEADERS , which is the default behavior in > avformat/mxfdec. > > Jan > _______________________________________________ > 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". >
diff --git a/libavcodec/jpeg2000_parser.c b/libavcodec/jpeg2000_parser.c index 123197fdca..b332a067e5 100644 --- a/libavcodec/jpeg2000_parser.c +++ b/libavcodec/jpeg2000_parser.c @@ -23,7 +23,6 @@ * @file * JPEG2000 parser. */ - #include "parser.h" /* Whether frame is jp2 file or codestream @@ -42,7 +41,6 @@ typedef struct JPEG2000ParserContext { uint8_t fheader_read; // are we reading uint8_t reading_file_header; uint8_t skipped_codestream; - uint8_t codestream_frame_end; uint8_t read_tp; uint8_t in_codestream; } JPEG2000ParserContext; @@ -57,7 +55,6 @@ static inline void reset_context(JPEG2000ParserContext *m) m->ft = 0; m->skipped_codestream = 0; m->fheader_read = 0; - m->codestream_frame_end = 0; m->skip_bytes = 0; m->read_tp = 0; m->in_codestream = 0; @@ -100,16 +97,13 @@ static int find_frame_end(JPEG2000ParserContext *m, const uint8_t *buf, int buf_ m->skip_bytes--; continue; } - if (m->codestream_frame_end) { - reset_context(m); - return i; - } if (m->read_tp) { // Find out how many bytes inside Tile part codestream to skip. if (m->read_tp == 1) { - m->skip_bytes = (state64 & 0xFFFFFFFF) - 10 > 0? - (state64 & 0xFFFFFFFF) - 10 : 0; + m->skip_bytes = (state64 & 0xFFFFFFFF) - 9 > 0? + (state64 & 0xFFFFFFFF) - 9 : 0; } m->read_tp--; + continue; } if (m->fheader_read) { if (m->fheader_read == 1) { @@ -141,7 +135,8 @@ static int find_frame_end(JPEG2000ParserContext *m, const uint8_t *buf, int buf_ if (pc->frame_start_found && m->ft == jp2_file) { m->skipped_codestream = 1; } else if (pc->frame_start_found && m->ft == j2k_cstream) { - m->codestream_frame_end = 1; + reset_context(m); + return i + 1; // End of frame detected, return frame size. } m->in_codestream = 0; } else if (m->in_codestream && (state & 0xFFFF) == 0xFF90) { // Are we in tile part header?
Please find my patch attached. libavcodec/jpeg2000_parser: Fix parsing of tile-part header, and frames where the end of frame marker is at the end of the buffer. Fixes playback of Sol Levante MXF JPEG2000 file: VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf http://download.opencontent.netflix.com.s3.amazonaws.com/SolLevante/imf/SolLevante_IMF_DolbyVision_PQP3D65_UHD_24fps.zip Bug Summary: When playing back VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf, I noticed a regression between release 4.2.4 and release 4.3.2. Release 4.3.2 skips some frames. This can be seen in the first 15 seconds of the video. The Netflix logo should fade out. This regression is in the master branch and was introduced by commit d09c356 which adds a JPEG2000 parser. I have also tested this patch against my 3 other JPEG2000 samples. The patches are against master branch 13ec662 and fix playback of this file. 1. Fixed a off by one error when calculating the number of bytes to skip. 2. Added a 'continue' to the Tile part header reading, to skip the reaming paring code during a header read. 3. To deal with the end of frame tag being at the very end of the buffer, return the frame size during the current loop iteration. Is the PARSER_FLAG_COMPLETE_FRAMES flag supposed to be set for the Netflix sample? The files playback correctly if the flag is set, as each buffer contains a complete frame, but it is not set for the test case above. Thank you, Shaun Simpson Subject: [PATCH 1/2] libavcodec/jpeg2000_parser: Fix parsing of tile-part header, and frames were the end of frame marker is at the end of the buffer. Fixes playback of Sol Levante MXF JPEG2000 file: VIDEO_e4da5fcd-5ffc-4713-bcdd-95ea579d790b.mxf http://download.opencontent.netflix.com.s3.amazonaws.com/SolLevante/imf/SolLevante_IMF_DolbyVision_PQP3D65_UHD_24fps.zip Signed-off-by: Shaun Simpson <shauns2029@gmail.com> --- libavcodec/jpeg2000_parser.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)