[FFmpeg-devel] [PATCH] lavc/h264_levels: add MaxMBPS checking and update fate test.
Lin, Decai
decai.lin at intel.com
Fri Mar 8 08:07:56 EET 2019
> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: 2019年3月7日 8:24
> To: ffmpeg-devel at 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 <decai.lin at intel.com>
> > ---
> > 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(&fr_num, &fr_den,
> > + ctx->tick_rate.num, ctx->tick_rate.den,
> 65535);
> > + else
> > + av_reduce(&fr_num, &fr_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 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 },
>
> These are all just under the constraint - add a few which are just over the
> given numbers too. E.g. 1280x720 at 31fps doesn't fit in level 3.1 so needs
> 3.2.
>
> > +};
> > +
> > +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 = ff_h264_guess_level(0, 0, test_sizes[i].width,
> > + level = ff_h264_guess_level(0, 0, 0, test_sizes[i].width,
> > test_sizes[i].height, 0);
> > CHECK(test_sizes[i].level_idc, "size %dx%d",
> > test_sizes[i].width, test_sizes[i].height);
> > }
> >
> > + for (i = 0; i < FF_ARRAY_ELEMS(test_framerate); i++) {
> > + level = ff_h264_guess_level(0, 0, test_framerate[i].framerate,
> > + test_framerate[i].width,
> > + test_framerate[i].height, 0);
> > + CHECK(test_framerate[i].level_idc, "framerate %d, size %dx%d",
> > + test_framerate[i].framerate, test_framerate[i].width,
> > + test_framerate[i].height);
> > + }
> > +
> > for (i = 0; i < FF_ARRAY_ELEMS(test_dpb); i++) {
> > - level = ff_h264_guess_level(0, 0, test_dpb[i].width,
> > + level = ff_h264_guess_level(0, 0, 0, test_dpb[i].width,
> > test_dpb[i].height,
> > test_dpb[i].dpb_size);
> > CHECK(test_dpb[i].level_idc, "size %dx%d dpb %d", @@ -164,7
> > +198,7 @@ int main(void)
> >
> > for (i = 0; i < FF_ARRAY_ELEMS(test_bitrate); i++) {
> > level = ff_h264_guess_level(test_bitrate[i].profile_idc,
> > - test_bitrate[i].bitrate,
> > + test_bitrate[i].bitrate, 0,
> > 0, 0, 0);
> > CHECK(test_bitrate[i].level_idc, "bitrate %"PRId64" profile %d",
> > test_bitrate[i].bitrate, test_bitrate[i].profile_idc);
> > @@ -173,6 +207,7 @@ int main(void)
> > for (i = 0; i < FF_ARRAY_ELEMS(test_all); i++) {
> > level = ff_h264_guess_level(test_all[i].profile_idc,
> > test_all[i].bitrate,
> > + 0,
> > test_all[i].width,
> > test_all[i].height,
> > test_all[i].dpb_frames); diff
> > --git a/libavcodec/vaapi_encode_h264.c
> > b/libavcodec/vaapi_encode_h264.c index 91be33f..7e7fd6a 100644
> > --- a/libavcodec/vaapi_encode_h264.c
> > +++ b/libavcodec/vaapi_encode_h264.c
> > @@ -329,9 +329,22 @@ static int
> vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
> > sps->level_idc = avctx->level;
> > } else {
> > const H264LevelDescriptor *level;
> > + int framerate, fr_num, fr_den;
> > +
> > + if (avctx->framerate.num > 0 && avctx->framerate.den > 0)
> > + av_reduce(&fr_num, &fr_den,
> > + avctx->framerate.num, avctx->framerate.den,
> 65535);
> > + else
> > + av_reduce(&fr_num, &fr_den,
> > + avctx->time_base.den, avctx->time_base.num,
> > + 65535);
>
> I think only framerate should apply here, not time_base.
>
> I don't think the reduction does anything? You're going to immediately
> round down on the division below anyway. (And if that's a problem, maybe
> the guess_level function should take an AVRational instead of an int.)
>
> > + if (fr_den > 0)
> > + framerate = fr_num / fr_den;
> > + else
> > + framerate = fr_num;
>
> As above, not sure the second branch is helpful.
>
> >
> > level = ff_h264_guess_level(sps->profile_idc,
> > avctx->bit_rate,
> > + framerate,
> > priv->mb_width * 16,
> > priv->mb_height * 16,
> > priv->dpb_frames);
> >
>
Thanks Mark, will update based on your comments.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list