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.w...@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.z...@intel.com>
>> Signed-off-by: Yi A Wang <yi.a.w...@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
>>
>> +@item hevc_main10
>> +
>> +@item h264_constrained_baseline
>> +
>> +@item h264_main
>> +
>> +@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
