[FFmpeg-devel] lavc/hevc_parser: add 4 bytes startcode condition in hevc_find_frame_end

Submitted by Michael Niedermayer on Nov. 28, 2018, 6:14 p.m.

Details

Message ID 20181128181406.GU3343@michaelspb
State New
Headers show

Commit Message

Michael Niedermayer Nov. 28, 2018, 6:14 p.m.
On Tue, Nov 27, 2018 at 08:16:55PM +0800, Linjie Fu wrote:
> The startcode before VPS,SPS,PPS and the first NALU in an AU is 4 bytes.
> Blindly taking the startcode as 3 bytes will leave 0x00 in last packet
> and may lead to some warnings in parse_nal_units when s->flags is set to
> PARSER_FLAG_COMPLETE_FRAMES.
> 
> Add 4 bytes startcode condition in hevc_find_frame_end.
> Modify the code to print the buf_size like in H264 and reduce the duplication.
> 
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
>  libavcodec/hevc_parser.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

breaks
 make fate-hevc-bsf-mp4toannexb
TEST    hevc-bsf-mp4toannexb
Test hevc-bsf-mp4toannexb failed. Look at tests/data/fate/hevc-bsf-mp4toannexb.err for details.
make: *** [fate-hevc-bsf-mp4toannexb] Error 1

[...]

Comments

Linjie Fu Nov. 29, 2018, 4:14 a.m.
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> Of Michael Niedermayer
> Sent: Thursday, November 29, 2018 02:14
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/hevc_parser: add 4 bytes
> startcode condition in hevc_find_frame_end
> 
> On Tue, Nov 27, 2018 at 08:16:55PM +0800, Linjie Fu wrote:
> > The startcode before VPS,SPS,PPS and the first NALU in an AU is 4 bytes.
> > Blindly taking the startcode as 3 bytes will leave 0x00 in last packet
> > and may lead to some warnings in parse_nal_units when s->flags is set to
> > PARSER_FLAG_COMPLETE_FRAMES.
> >
> > Add 4 bytes startcode condition in hevc_find_frame_end.
> > Modify the code to print the buf_size like in H264 and reduce the
> duplication.
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> >  libavcodec/hevc_parser.c | 15 ++++++++++-----
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> breaks
>  make fate-hevc-bsf-mp4toannexb
> TEST    hevc-bsf-mp4toannexb
> --- -	2018-11-28 19:12:47.869732022 +0100
> +++ tests/data/fate/hevc-bsf-mp4toannexb	2018-11-28
> 19:12:47.864276885 +0100
> @@ -1 +1 @@
> -1873662a3af1848c37e4eb25722c8df9
> +73019329ed7f81c24f9af67c34c640c0
> Test hevc-bsf-mp4toannexb failed. Look at tests/data/fate/hevc-bsf-
> mp4toannexb.err for details.
> make: *** [fate-hevc-bsf-mp4toannexb] Error 1
> 

Investigate the root cause:

In fate test:
Step 1. ffmpeg -i hevc-conformance/WPP_A_ericsson_MAIN10_2.bit -c copy -flags +bitexact hevc-mp4.mov
Step 2. ffmpeg -i hevc-mp4_after.mov -c:v copy -fflags +bitexact -f hevc hevc.out
Step3. md5sum and compared with the reference

Compare the output file in Step 1 before and after the patch, and found it changed:
-rw-r--r-- 1 root     root     68460 11月 29 11:23 hevc-mp4_after.mov
-rw-r--r-- 1 root     root     68507 11月 29 11:31 hevc-mp4_before.mov

Compare the original file with two output files:
SUFFIX_SEI bit information in  the first frame for example :

WPP_A_ericsson_MAIN10_2.bit: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 }

hevc-mp4_before.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 00 }

hevc-mp4_after.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 }

Before applying the patch, the output file has an extra 0x00 at the end of SUFFIX_SEI (because of blindly taking startcode as 3 bytes and left one byte).
After applying the patch, the output file has the same bit info like the original file.
So, I think this patch works.

Thanks,
- Linjie
Michael Niedermayer Nov. 29, 2018, 11 p.m.
On Thu, Nov 29, 2018 at 04:14:29AM +0000, Fu, Linjie wrote:
> > -----Original Message-----
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
> > Of Michael Niedermayer
> > Sent: Thursday, November 29, 2018 02:14
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] lavc/hevc_parser: add 4 bytes
> > startcode condition in hevc_find_frame_end
> > 
> > On Tue, Nov 27, 2018 at 08:16:55PM +0800, Linjie Fu wrote:
> > > The startcode before VPS,SPS,PPS and the first NALU in an AU is 4 bytes.
> > > Blindly taking the startcode as 3 bytes will leave 0x00 in last packet
> > > and may lead to some warnings in parse_nal_units when s->flags is set to
> > > PARSER_FLAG_COMPLETE_FRAMES.
> > >
> > > Add 4 bytes startcode condition in hevc_find_frame_end.
> > > Modify the code to print the buf_size like in H264 and reduce the
> > duplication.
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > ---
> > >  libavcodec/hevc_parser.c | 15 ++++++++++-----
> > >  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > breaks
> >  make fate-hevc-bsf-mp4toannexb
> > TEST    hevc-bsf-mp4toannexb
> > --- -	2018-11-28 19:12:47.869732022 +0100
> > +++ tests/data/fate/hevc-bsf-mp4toannexb	2018-11-28
> > 19:12:47.864276885 +0100
> > @@ -1 +1 @@
> > -1873662a3af1848c37e4eb25722c8df9
> > +73019329ed7f81c24f9af67c34c640c0
> > Test hevc-bsf-mp4toannexb failed. Look at tests/data/fate/hevc-bsf-
> > mp4toannexb.err for details.
> > make: *** [fate-hevc-bsf-mp4toannexb] Error 1
> > 
> 
> Investigate the root cause:
> 
> In fate test:
> Step 1. ffmpeg -i hevc-conformance/WPP_A_ericsson_MAIN10_2.bit -c copy -flags +bitexact hevc-mp4.mov
> Step 2. ffmpeg -i hevc-mp4_after.mov -c:v copy -fflags +bitexact -f hevc hevc.out
> Step3. md5sum and compared with the reference
> 
> Compare the output file in Step 1 before and after the patch, and found it changed:
> -rw-r--r-- 1 root     root     68460 11月 29 11:23 hevc-mp4_after.mov
> -rw-r--r-- 1 root     root     68507 11月 29 11:31 hevc-mp4_before.mov
> 
> Compare the original file with two output files:
> SUFFIX_SEI bit information in  the first frame for example :
> 
> WPP_A_ericsson_MAIN10_2.bit: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 }
> 
> hevc-mp4_before.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 00 }
> 
> hevc-mp4_after.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 }
> 
> Before applying the patch, the output file has an extra 0x00 at the end of SUFFIX_SEI (because of blindly taking startcode as 3 bytes and left one byte).
> After applying the patch, the output file has the same bit info like the original file.
> So, I think this patch works.

if the patch break make fate then it doesnt work.
If a change cause by a patch is correct it needs to update the 
test / its checksum
This is also important so that anyone looking at the patch knows which tests
change and anyone testing knows how to detect a test failure vs a passed test


[...]
Linjie Fu Dec. 7, 2018, 11:39 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Michael Niedermayer

> Sent: Friday, November 30, 2018 07:01

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH] lavc/hevc_parser: add 4 bytes

> startcode condition in hevc_find_frame_end

> 

> On Thu, Nov 29, 2018 at 04:14:29AM +0000, Fu, Linjie wrote:

> > > -----Original Message-----

> > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > > Of Michael Niedermayer

> > > Sent: Thursday, November 29, 2018 02:14

> > > To: FFmpeg development discussions and patches <ffmpeg-

> > > devel@ffmpeg.org>

> > > Subject: Re: [FFmpeg-devel] [PATCH] lavc/hevc_parser: add 4 bytes

> > > startcode condition in hevc_find_frame_end

> > >

> > > On Tue, Nov 27, 2018 at 08:16:55PM +0800, Linjie Fu wrote:

> > > > The startcode before VPS,SPS,PPS and the first NALU in an AU is 4 bytes.

> > > > Blindly taking the startcode as 3 bytes will leave 0x00 in last packet

> > > > and may lead to some warnings in parse_nal_units when s->flags is set

> to

> > > > PARSER_FLAG_COMPLETE_FRAMES.

> > > >

> > > > Add 4 bytes startcode condition in hevc_find_frame_end.

> > > > Modify the code to print the buf_size like in H264 and reduce the

> > > duplication.

> > > >

> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>

> > > > ---

> > > >  libavcodec/hevc_parser.c | 15 ++++++++++-----

> > > >  1 file changed, 10 insertions(+), 5 deletions(-)

> > >

> > > breaks

> > >  make fate-hevc-bsf-mp4toannexb

> > > TEST    hevc-bsf-mp4toannexb

> > > --- -	2018-11-28 19:12:47.869732022 +0100

> > > +++ tests/data/fate/hevc-bsf-mp4toannexb	2018-11-28

> > > 19:12:47.864276885 +0100

> > > @@ -1 +1 @@

> > > -1873662a3af1848c37e4eb25722c8df9

> > > +73019329ed7f81c24f9af67c34c640c0

> > > Test hevc-bsf-mp4toannexb failed. Look at tests/data/fate/hevc-bsf-

> > > mp4toannexb.err for details.

> > > make: *** [fate-hevc-bsf-mp4toannexb] Error 1

> > >

> >

> > Investigate the root cause:

> >

> > In fate test:

> > Step 1. ffmpeg -i hevc-conformance/WPP_A_ericsson_MAIN10_2.bit -c

> copy -flags +bitexact hevc-mp4.mov

> > Step 2. ffmpeg -i hevc-mp4_after.mov -c:v copy -fflags +bitexact -f hevc

> hevc.out

> > Step3. md5sum and compared with the reference

> >

> > Compare the output file in Step 1 before and after the patch, and found it

> changed:

> > -rw-r--r-- 1 root     root     68460 11月 29 11:23 hevc-mp4_after.mov

> > -rw-r--r-- 1 root     root     68507 11月 29 11:31 hevc-mp4_before.mov

> >

> > Compare the original file with two output files:

> > SUFFIX_SEI bit information in  the first frame for example :

> >

> > WPP_A_ericsson_MAIN10_2.bit: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09

> 38 9a 79 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0

> 8f 66 ee 95 f9 e2 b9 ba 23 9a e8 80 }

> >

> > hevc-mp4_before.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79

> 41 c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee

> 95 f9 e2 b9 ba 23 9a e8 80 00 }

> >

> > hevc-mp4_after.mov: { 50 01 84 31 00 19 39 77 d0 7b 3f bd 1e 09 38 9a 79 41

> c0 16 11 da 18 aa 0a db 2b 19 fa 47 3f 0f 67 4a 91 9c a1 12 72 67 d6 f0 8f 66 ee 95

> f9 e2 b9 ba 23 9a e8 80 }

> >

> > Before applying the patch, the output file has an extra 0x00 at the end of

> SUFFIX_SEI (because of blindly taking startcode as 3 bytes and left one byte).

> > After applying the patch, the output file has the same bit info like the

> original file.

> > So, I think this patch works.

> 

> if the patch break make fate then it doesnt work.

> If a change cause by a patch is correct it needs to update the

> test / its checksum

> This is also important so that anyone looking at the patch knows which tests

> change and anyone testing knows how to detect a test failure vs a passed

> test


Thanks, sent a [v2] patch with the updated FATE checksum.
---
Linjie

> [...]

> --

> Michael     GnuPG fingerprint:

> 9FF2128B147EF6730BADF133611EC787040B0FAB

> 

> Those who are best at talking, realize last or never when they are wrong.

Patch hide | download patch | download mbox

--- -	2018-11-28 19:12:47.869732022 +0100
+++ tests/data/fate/hevc-bsf-mp4toannexb	2018-11-28 19:12:47.864276885 +0100
@@ -1 +1 @@ 
-1873662a3af1848c37e4eb25722c8df9
+73019329ed7f81c24f9af67c34c640c0