[FFmpeg-devel] [PATCH, v2] lavc/hevc_parser: add 4 bytes startcode condition and update FATE

[Mark Thompson]

On 07/12/2018 11:33, 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.
> 
> Update the FATE checksum for fate-hevc-bsf-mp4toannexb:
> The ref output has redundant 0x00 at the end of the SUFFIX_SEI:
> { 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 }
> 
> To verify the output of FATE:
> ffmpeg -i hevc-conformance/WPP_A_ericsson_MAIN10_2.bit -c copy -flags \
>         +bitexact hevc-mp4.mov
> ffmpeg -i hevc-mp4.mov -c:v copy -fflags +bitexact -f hevc hevc.out
> 
> Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> ---
> [v2]: Update FATE checksum
> 
>  libavcodec/hevc_parser.c | 15 ++++++++++-----
>  tests/fate/hevc.mak      |  2 +-
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/hevc_parser.c b/libavcodec/hevc_parser.c
> index 369d1338d0..aa216e3c8d 100644
> --- a/libavcodec/hevc_parser.c
> +++ b/libavcodec/hevc_parser.c
> @@ -32,6 +32,7 @@
>  #include "parser.h"
>  
>  #define START_CODE 0x000001 ///< start_code_prefix_one_3bytes
> +#define START_CODE_4 0x00000001 ///< start_code_4bytes

(There is no syntax element called "start_code_4bytes".)

>  
>  #define IS_IRAP_NAL(nal) (nal->type >= 16 && nal->type <= 23)
>  #define IS_IDR_NAL(nal) (nal->type == HEVC_NAL_IDR_W_RADL || nal->type == HEVC_NAL_IDR_N_LP)
> @@ -239,7 +240,7 @@ static int parse_nal_units(AVCodecParserContext *s, const uint8_t *buf,
>          }
>      }
>      /* didn't find a picture! */
> -    av_log(avctx, AV_LOG_ERROR, "missing picture in access unit\n");
> +    av_log(avctx, AV_LOG_ERROR, "missing picture in access unit with size %d\n", buf_size);
>      return -1;
>  }
>  
> @@ -267,8 +268,7 @@ static int hevc_find_frame_end(AVCodecParserContext *s, const uint8_t *buf,
>          if ((nut >= HEVC_NAL_VPS && nut <= HEVC_NAL_EOB_NUT) || nut == HEVC_NAL_SEI_PREFIX ||
>              (nut >= 41 && nut <= 44) || (nut >= 48 && nut <= 55)) {
>              if (pc->frame_start_found) {
> -                pc->frame_start_found = 0;
> -                return i - 5;
> +                goto found;
>              }
>          } else if (nut <= HEVC_NAL_RASL_R ||
>                     (nut >= HEVC_NAL_BLA_W_LP && nut <= HEVC_NAL_CRA_NUT)) {
> @@ -277,14 +277,19 @@ static int hevc_find_frame_end(AVCodecParserContext *s, const uint8_t *buf,
>                  if (!pc->frame_start_found) {
>                      pc->frame_start_found = 1;
>                  } else { // First slice of next frame found
> -                    pc->frame_start_found = 0;
> -                    return i - 5;
> +                    goto found;
>                  }
>              }
>          }
>      }
>  
>      return END_NOT_FOUND;
> +
> +found:
> +    pc->frame_start_found = 0;
> +    if (((pc->state64 >> 3 * 8) & 0xFFFFFFFF) == START_CODE_4)
> +        return i - 6;
> +    return i - 5;
>  }

Maybe?  Special-casing four bytes doesn't make me feel good about this, given that leading_zero_8bits can be included as many times as you like at the beginning of a NAL unit.

If there are a large number of zeroes between two frames, which packet do you want them to end up in?  This changes it from "all at the end of the first frame" to "all except the last one at the end of the first frame", and it's not immediately obvious to me why that would be the right choice.  Why not, for example, put all of the zeroes into the second frame?

>  static int hevc_parse(AVCodecParserContext *s, AVCodecContext *avctx,
> diff --git a/tests/fate/hevc.mak b/tests/fate/hevc.mak
> index db3ea19340..ab8da53535 100644
> --- a/tests/fate/hevc.mak
> +++ b/tests/fate/hevc.mak
> @@ -238,7 +238,7 @@ FATE_HEVC-$(call ALLYES, HEVC_DEMUXER MOV_DEMUXER HEVC_MP4TOANNEXB_BSF MOV_MUXER
>  fate-hevc-bsf-mp4toannexb: tests/data/hevc-mp4.mov
>  fate-hevc-bsf-mp4toannexb: CMD = md5 -i $(TARGET_PATH)/tests/data/hevc-mp4.mov -c:v copy -fflags +bitexact -f hevc
>  fate-hevc-bsf-mp4toannexb: CMP = oneline
> -fate-hevc-bsf-mp4toannexb: REF = 1873662a3af1848c37e4eb25722c8df9
> +fate-hevc-bsf-mp4toannexb: REF = 73019329ed7f81c24f9af67c34c640c0
>  
>  fate-hevc-skiploopfilter: CMD = framemd5 -skip_loop_filter nokey -i $(TARGET_SAMPLES)/hevc-conformance/SAO_D_Samsung_5.bit -sws_flags bitexact
>  FATE_HEVC += fate-hevc-skiploopfilter
> 

Thanks,

- Mark