Re: [FFmpeg-devel] [PATCH] lavc/h264_levels: add MaxMBPS checking and update fate test.
> -Original Message-
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: 2019年3月7日 8:24
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] lavc/h264_levels: add MaxMBPS
> checking and update fate test.
>
> On 06/03/2019 08:21, Decai Lin wrote:
> > 1. add MaxMBPS checking for level idc setting to align with AVC spec
> >AnnexA table A-1/A-6 level limits.
> > 2. update h264 level fate test.
> >
> > Signed-off-by: Decai Lin
> > ---
> > libavcodec/h264_levels.c | 5 +
> > libavcodec/h264_levels.h | 1 +
> > libavcodec/h264_metadata_bsf.c | 14 +-
> > libavcodec/tests/h264_levels.c | 41
> > ++---
> > libavcodec/vaapi_encode_h264.c | 13 +
> > 5 files changed, 70 insertions(+), 4 deletions(-)
>
> Nice, this is a good idea. Some comments below.
>
> > diff --git a/libavcodec/h264_levels.c b/libavcodec/h264_levels.c index
> > 7a55116..e457dbe 100644
> > --- a/libavcodec/h264_levels.c
> > +++ b/libavcodec/h264_levels.c
> > @@ -89,11 +89,13 @@ const H264LevelDescriptor *ff_h264_get_level(int
> > level_idc,
> >
> > const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
> > int64_t bitrate,
> > + int framerate,
> > int width, int
> height,
> > int
> > max_dec_frame_buffering) {
> > int width_mbs = (width + 15) / 16;
> > int height_mbs = (height + 15) / 16;
> > +int64_t mb_processing_rate = (int64_t) (width_mbs * height_mbs *
> > + framerate);
>
> This doesn't work - if it overflows you've already hit undefined behaviour in
> the calculation before the cast.
>
> > int no_cs3f = !(profile_idc == 66 ||
> > profile_idc == 77 ||
> > profile_idc == 88); @@ -108,6 +110,9 @@ const
> > H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
> > if (bitrate > (int64_t)level->max_br *
> h264_get_br_factor(profile_idc))
> > continue;
> >
> > +if (mb_processing_rate > (int64_t)level->max_mbps)
> > +continue;
> > +
> > if (width_mbs * height_mbs > level->max_fs)
> > continue;
> > if (width_mbs * width_mbs > 8 * level->max_fs) diff --git
> > a/libavcodec/h264_levels.h b/libavcodec/h264_levels.h index
> > 4189fc6..0a0f410 100644
> > --- a/libavcodec/h264_levels.h
> > +++ b/libavcodec/h264_levels.h
> > @@ -46,6 +46,7 @@ const H264LevelDescriptor *ff_h264_get_level(int
> level_idc,
> > */
> > const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
> > int64_t bitrate,
> > + int framerate,
> > int width, int
> height,
> > int
> > max_dec_frame_buffering);
> >
> > diff --git a/libavcodec/h264_metadata_bsf.c
> > b/libavcodec/h264_metadata_bsf.c index a17987a..61c2711 100644
> > --- a/libavcodec/h264_metadata_bsf.c
> > +++ b/libavcodec/h264_metadata_bsf.c
> > @@ -223,6 +223,7 @@ static int
> h264_metadata_update_sps(AVBSFContext *bsf,
> > const H264LevelDescriptor *desc;
> > int64_t bit_rate;
> > int width, height, dpb_frames;
> > +int framerate, fr_num, fr_den;
> >
> > if (sps->vui.nal_hrd_parameters_present_flag) {
> > bit_rate =
> > (sps->vui.nal_hrd_parameters.bit_rate_value_minus1[0] + 1) * @@ -244,7
> +245,18 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
> > height = 16 * (sps->pic_height_in_map_units_minus1 + 1)
> *
> > (2 - sps->frame_mbs_only_flag);
> >
> > -desc = ff_h264_guess_level(sps->profile_idc, bit_rate,
> > +if (ctx->tick_rate.num > 0 && ctx->tick_rate.den > 0)
> > +av_reduce(_num, _den,
> > +ctx->tick_rate.num, ctx->tick_rate.den,
> 65535);
> > +else
> > +av_reduce(_num, _den,
> > +ctx->tick_rate.den, ctx->tick_rate.num,
> > + 65535);
>
Re: [FFmpeg-devel] [PATCH] lavc/h264_levels: add MaxMBPS checking and update fate test.
On 06/03/2019 08:21, Decai Lin wrote:
> 1. add MaxMBPS checking for level idc setting to align with AVC spec
>AnnexA table A-1/A-6 level limits.
> 2. update h264 level fate test.
>
> Signed-off-by: Decai Lin
> ---
> libavcodec/h264_levels.c | 5 +
> libavcodec/h264_levels.h | 1 +
> libavcodec/h264_metadata_bsf.c | 14 +-
> libavcodec/tests/h264_levels.c | 41 ++---
> libavcodec/vaapi_encode_h264.c | 13 +
> 5 files changed, 70 insertions(+), 4 deletions(-)
Nice, this is a good idea. Some comments below.
> diff --git a/libavcodec/h264_levels.c b/libavcodec/h264_levels.c
> index 7a55116..e457dbe 100644
> --- a/libavcodec/h264_levels.c
> +++ b/libavcodec/h264_levels.c
> @@ -89,11 +89,13 @@ const H264LevelDescriptor *ff_h264_get_level(int
> level_idc,
>
> const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
> int64_t bitrate,
> + int framerate,
> int width, int height,
> int max_dec_frame_buffering)
> {
> int width_mbs = (width + 15) / 16;
> int height_mbs = (height + 15) / 16;
> +int64_t mb_processing_rate = (int64_t) (width_mbs * height_mbs *
> framerate);
This doesn't work - if it overflows you've already hit undefined behaviour in
the calculation before the cast.
> int no_cs3f = !(profile_idc == 66 ||
> profile_idc == 77 ||
> profile_idc == 88);
> @@ -108,6 +110,9 @@ const H264LevelDescriptor *ff_h264_guess_level(int
> profile_idc,
> if (bitrate > (int64_t)level->max_br *
> h264_get_br_factor(profile_idc))
> continue;
>
> +if (mb_processing_rate > (int64_t)level->max_mbps)
> +continue;
> +
> if (width_mbs * height_mbs > level->max_fs)
> continue;
> if (width_mbs * width_mbs > 8 * level->max_fs)
> diff --git a/libavcodec/h264_levels.h b/libavcodec/h264_levels.h
> index 4189fc6..0a0f410 100644
> --- a/libavcodec/h264_levels.h
> +++ b/libavcodec/h264_levels.h
> @@ -46,6 +46,7 @@ const H264LevelDescriptor *ff_h264_get_level(int level_idc,
> */
> const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
> int64_t bitrate,
> + int framerate,
> int width, int height,
> int max_dec_frame_buffering);
>
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index a17987a..61c2711 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -223,6 +223,7 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
> const H264LevelDescriptor *desc;
> int64_t bit_rate;
> int width, height, dpb_frames;
> +int framerate, fr_num, fr_den;
>
> if (sps->vui.nal_hrd_parameters_present_flag) {
> bit_rate =
> (sps->vui.nal_hrd_parameters.bit_rate_value_minus1[0] + 1) *
> @@ -244,7 +245,18 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
> height = 16 * (sps->pic_height_in_map_units_minus1 + 1) *
> (2 - sps->frame_mbs_only_flag);
>
> -desc = ff_h264_guess_level(sps->profile_idc, bit_rate,
> +if (ctx->tick_rate.num > 0 && ctx->tick_rate.den > 0)
> +av_reduce(_num, _den,
> +ctx->tick_rate.num, ctx->tick_rate.den, 65535);
> +else
> +av_reduce(_num, _den,
> +ctx->tick_rate.den, ctx->tick_rate.num, 65535);
Since the VUI fields are set above, I suggest using them instead here because
that will include an existing rate in the stream.
Also, the tick rate is only directly usable as fieldrate if
fixed_frame_rate_flag is set.
> +if (fr_den > 0)
> +framerate = fr_num / fr_den;
> +else
> +framerate = fr_num;
I'm not sure what this is trying to do. If the number isn't valid, the
framerate should be passed as zero to avoid constraining anything.
> +
> +desc = ff_h264_guess_level(sps->profile_idc, bit_rate, framerate,
> width, height, dpb_frames);
> if (desc) {
> level_idc = desc->level_idc;
> diff --git a/libavcodec/tests/h264_levels.c b/libavcodec/tests/h264_levels.c
> index 0e00f05..6176c0b 100644
> --- a/libavcodec/tests/h264_levels.c
> +++ b/libavcodec/tests/h264_levels.c
> @@ -62,6 +62,31 @@ static const struct {
> static const struct {
> int width;
> int height;
> +int framerate;
> +int level_idc;
> +} test_framerate[] = {
> +// Some typical sizes and
[FFmpeg-devel] [PATCH] lavc/h264_levels: add MaxMBPS checking and update fate test.
1. add MaxMBPS checking for level idc setting to align with AVC spec
AnnexA table A-1/A-6 level limits.
2. update h264 level fate test.
Signed-off-by: Decai Lin
---
libavcodec/h264_levels.c | 5 +
libavcodec/h264_levels.h | 1 +
libavcodec/h264_metadata_bsf.c | 14 +-
libavcodec/tests/h264_levels.c | 41 ++---
libavcodec/vaapi_encode_h264.c | 13 +
5 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/libavcodec/h264_levels.c b/libavcodec/h264_levels.c
index 7a55116..e457dbe 100644
--- a/libavcodec/h264_levels.c
+++ b/libavcodec/h264_levels.c
@@ -89,11 +89,13 @@ const H264LevelDescriptor *ff_h264_get_level(int level_idc,
const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
int64_t bitrate,
+ int framerate,
int width, int height,
int max_dec_frame_buffering)
{
int width_mbs = (width + 15) / 16;
int height_mbs = (height + 15) / 16;
+int64_t mb_processing_rate = (int64_t) (width_mbs * height_mbs *
framerate);
int no_cs3f = !(profile_idc == 66 ||
profile_idc == 77 ||
profile_idc == 88);
@@ -108,6 +110,9 @@ const H264LevelDescriptor *ff_h264_guess_level(int
profile_idc,
if (bitrate > (int64_t)level->max_br * h264_get_br_factor(profile_idc))
continue;
+if (mb_processing_rate > (int64_t)level->max_mbps)
+continue;
+
if (width_mbs * height_mbs > level->max_fs)
continue;
if (width_mbs * width_mbs > 8 * level->max_fs)
diff --git a/libavcodec/h264_levels.h b/libavcodec/h264_levels.h
index 4189fc6..0a0f410 100644
--- a/libavcodec/h264_levels.h
+++ b/libavcodec/h264_levels.h
@@ -46,6 +46,7 @@ const H264LevelDescriptor *ff_h264_get_level(int level_idc,
*/
const H264LevelDescriptor *ff_h264_guess_level(int profile_idc,
int64_t bitrate,
+ int framerate,
int width, int height,
int max_dec_frame_buffering);
diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index a17987a..61c2711 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
@@ -223,6 +223,7 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
const H264LevelDescriptor *desc;
int64_t bit_rate;
int width, height, dpb_frames;
+int framerate, fr_num, fr_den;
if (sps->vui.nal_hrd_parameters_present_flag) {
bit_rate =
(sps->vui.nal_hrd_parameters.bit_rate_value_minus1[0] + 1) *
@@ -244,7 +245,18 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
height = 16 * (sps->pic_height_in_map_units_minus1 + 1) *
(2 - sps->frame_mbs_only_flag);
-desc = ff_h264_guess_level(sps->profile_idc, bit_rate,
+if (ctx->tick_rate.num > 0 && ctx->tick_rate.den > 0)
+av_reduce(_num, _den,
+ctx->tick_rate.num, ctx->tick_rate.den, 65535);
+else
+av_reduce(_num, _den,
+ctx->tick_rate.den, ctx->tick_rate.num, 65535);
+if (fr_den > 0)
+framerate = fr_num / fr_den;
+else
+framerate = fr_num;
+
+desc = ff_h264_guess_level(sps->profile_idc, bit_rate, framerate,
width, height, dpb_frames);
if (desc) {
level_idc = desc->level_idc;
diff --git a/libavcodec/tests/h264_levels.c b/libavcodec/tests/h264_levels.c
index 0e00f05..6176c0b 100644
--- a/libavcodec/tests/h264_levels.c
+++ b/libavcodec/tests/h264_levels.c
@@ -62,6 +62,31 @@ static const struct {
static const struct {
int width;
int height;
+int framerate;
+int level_idc;
+} test_framerate[] = {
+// Some typical sizes and frame rates.
+// (From H.264 table A-1 and table A-6)
+{ 176, 144, 15, 10 },
+{ 320, 240, 10, 11 },
+{ 352, 288, 30, 13 },
+{ 352, 576, 25, 21 },
+{ 640, 480, 33, 30 },
+{ 720, 576, 25, 30 },
+{ 1024, 768, 35, 31 },
+{ 1280, 720, 30, 31 },
+{ 1280, 1024, 42, 32 },
+{ 1920, 1088, 30, 40 },
+{ 2048, 1024, 30, 40 },
+{ 2048, 1088, 60, 42 },
+{ 3680, 1536, 26, 50 },
+{ 4096, 2048, 30, 51 },
+{ 4096, 2160, 60, 52 },
+};
+
+static const struct {
+int width;
+int height;
int dpb_size;
int level_idc;
} test_dpb[] = {
@@ -147,14 +172,23 @@ int main(void)
} while (0)
for (i = 0; i < FF_ARRAY_ELEMS(test_sizes); i++) {
-level =
