[FFmpeg-devel] [PATCH] lavc/vaapi_encode_h264: add option to indicate the h264 encode profile

[Jun Zhao]

On 2016/12/15 7:02, Mark Thompson wrote:
> On 14/12/16 01:55, Jun Zhao wrote:
>> From 03030392ec2458679cdfb14802b80cbb196eae40 Mon Sep 17 00:00:00 2001
>> From: Yi A Wang <yi.a.wang at intel.com>
>> Date: Tue, 13 Dec 2016 10:50:54 +0800
>> Subject: [PATCH] lavc/vaapi_encode_h264: add option to indicate the h264
>>  encode profile
>>
>> add h264 encode profile option and update the docs, for avc
>> constrained baseline, disable B frames base on H.264 spec Annex A.2.1
>>
>> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
>> Signed-off-by: Yi A Wang <yi.a.wang at intel.com>
>> ---
>>  doc/codecs.texi                | 8 ++++++++
>>  libavcodec/options_table.h     | 5 ++++-
>>  libavcodec/vaapi_encode_h264.c | 5 +++++
>>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> Notwithstanding the below, this should probably be two patches (one for options/docs, one for VAAPI).
> 
> 
>> diff --git a/doc/codecs.texi b/doc/codecs.texi
>> index 9a3a56d..9ee9061 100644
>> --- a/doc/codecs.texi
>> +++ b/doc/codecs.texi
>> @@ -893,6 +893,14 @@ Possible values:
>>  
>>  @item dts_hd_ma
>>  
>> + at item hevc_main10
>> +
>> + at item h264_constrained_baseline
>> +
>> + at item h264_main
>> +
>> + at item h264_high
>> +
>>  @end table
>>  
>>  @item level @var{integer} (@emph{encoding,audio,video})
>> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
>> index 3fe7925..94b2d9b 100644
>> --- a/libavcodec/options_table.h
>> +++ b/libavcodec/options_table.h
>> @@ -395,7 +395,10 @@ static const AVOption avcodec_options[] = {
>>  {"mpeg4_core", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_CORE }, INT_MIN, INT_MAX, V|E, "profile"},
>>  {"mpeg4_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_MAIN }, INT_MIN, INT_MAX, V|E, "profile"},
>>  {"mpeg4_asp",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_ADVANCED_SIMPLE }, INT_MIN, INT_MAX, V|E, "profile"},
>> -{"main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "profile"},
> 
> This table is part of the public API of libavcodec, you can't remove things from it like this.
> 
>> +{"hevc_main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_constrained_baseline", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_CONSTRAINED_BASELINE}, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_main", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_MAIN}, INT_MIN, INT_MAX, V|E, "profile"},
>> +{"h264_high", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_HIGH}, INT_MIN, INT_MAX, V|E, "profile"},
> 
> This seems reasonable, but I'd like to hear from other people why there is only a small subset of video profiles there already?  Confusingly, the libx264, nvenc and videotoolbox encoders all have private options (also called "profile") which implement exactly the same thing where they could be using the generic one (were it to support suitable options).
> 
> If the change is desirable, it should probably get the full set of profiles corresponding to FF_PROFILE_H264_* values rather than just a restricted set coming from what VAAPI exposes.  (Also deprecate the anomalous "main10" and add the H.265 profiles properly as well, I guess?)
> 
Will move to private options, tks.
> 
>>  {"level", NULL, OFFSET(level), AV_OPT_TYPE_INT, {.i64 = FF_LEVEL_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "level"},
>>  {"unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_LEVEL_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "level"},
>>  {"lowres", "decode at 1= 1/2, 2=1/4, 3=1/8 resolutions", OFFSET(lowres), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, INT_MAX, V|A|D},
>> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
>> index 69cc483..5f37770 100644
>> --- a/libavcodec/vaapi_encode_h264.c
>> +++ b/libavcodec/vaapi_encode_h264.c
>> @@ -1190,6 +1190,11 @@ static av_cold int vaapi_encode_h264_init(AVCodecContext *avctx)
>>      switch (avctx->profile) {
>>      case FF_PROFILE_H264_CONSTRAINED_BASELINE:
>>          ctx->va_profile = VAProfileH264ConstrainedBaseline;
>> +        if (avctx->max_b_frames != 0) {
>> +            avctx->max_b_frames = 0;
>> +            av_log(avctx, AV_LOG_WARNING, "H.264 constrained baseline "
>> +                   "profile don't support encode B frame.\n");
> 
>                           ..." doesn't support encoding B frames.\n", maybe
> 
>> +        }
> 
> I guess this makes sense.  It's not really a bitstream restriction, though, only a conformance one - you can perfectly well make a usable stream with profile_idc 66 which contains B frames (though only decodable with an extended or main profile decoder), and indeed the i965 encoder is happy to do so.  Should that matter?
> 
I hope I can give a user case for disable B frame in ConstrainedBaseline/Baseline: 
for example, in video conference, user used the ConstrainedBaseline/Baseline 
profile without B frame to reduce the encode/decode latency.

For i965 can decode the stream with profile_idc 66 which contains B frame, I think 
the root cause is the i965 driver used the loose check for this type case. :)
 
>>          break;
>>      case FF_PROFILE_H264_BASELINE:
>>          ctx->va_profile = VAProfileH264Baseline;
> 
> In any case, the restriction applies to baseline profile as well as constrained baseline.
Agree
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>