Discussion:
[FFmpeg-devel] [PATCH, v2] lavc/hevc_parser: add 4 bytes startcode condition and update FATE
Linjie Fu
2018-12-07 11:33:51 UTC
Permalink
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 <***@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

#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;
}

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
--
2.17.1
Mark Thompson
2018-12-09 17:39:49 UTC
Permalink
Post by Linjie Fu
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.
{ 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 }
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
---
[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".)
Post by Linjie Fu
#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;
+
+ 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?
Post by Linjie Fu
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
Fu, Linjie
2018-12-10 10:30:15 UTC
Permalink
-----Original Message-----
Of Mark Thompson
Sent: Monday, December 10, 2018 01:40
Subject: Re: [FFmpeg-devel] [PATCH, v2] lavc/hevc_parser: add 4 bytes
startcode condition and update FATE
Post by Linjie Fu
#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".)
Deleted.
Post by Linjie Fu
#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,
Post by Linjie Fu
}
}
/* 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);
Post by Linjie Fu
return -1;
}
@@ -267,8 +268,7 @@ static int
hevc_find_frame_end(AVCodecParserContext *s, const uint8_t *buf,
Post by Linjie Fu
if ((nut >= HEVC_NAL_VPS && nut <= HEVC_NAL_EOB_NUT) || nut ==
HEVC_NAL_SEI_PREFIX ||
Post by Linjie Fu
(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))
{
Post by Linjie Fu
@@ -277,14 +277,19 @@ static int
hevc_find_frame_end(AVCodecParserContext *s, const uint8_t *buf,
Post by Linjie Fu
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;
+
+ 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?
Modified and sent a new patch to find all leading_zero_8bits and put them into
the second frame.

Thanks
---
Linjie Fu

Loading...