diff mbox series

[FFmpeg-devel] Patch JPEG2000 Parser: Fix parsing of tile-part header

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
Related show

Checks

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

Commit Message

Shaun Simpson July 21, 2021, 3:30 p.m. UTC
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(-)

Comments

Jan Ekström July 24, 2021, 6:01 p.m. UTC | #1
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
Shaun Simpson July 25, 2021, 8:47 a.m. UTC | #2
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 mbox series

Patch

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?