On 12/04/14 18:13, Tim Walker wrote:
> On 12 Apr 2014, at 16:36, Luca Barbato <[email protected]> wrote:
>
>> + //q->param.mfx.TimeStampCalc = 0; // API 1.3
>> + //q->param.mfx.ExtendedPicStruct = 0; // API 1.3
>> + //q->param.mfx.BRCParamMultiplier = 0; // API 1.3
>> + //q->param.mfx.SliceGroupsPresent = 0; // API 1.6
>
> These fields are part of a union and are incompatible with TargetUsage,
> GopPicSize etc. Don't leave them here to be commented out accidentally by
> someone.
>
>> + q->param.mfx.RateControlMethod =
>> + (q->qpi >= 0 && q->qpp >= 0 && q->qpb >= 0) ||
>> + avctx->flags & CODEC_FLAG_QSCALE ? MFX_RATECONTROL_CQP :
>> + avctx->rc_max_rate &&
>> + avctx->rc_max_rate == avctx->bit_rate ? MFX_RATECONTROL_CBR :
>> + MFX_RATECONTROL_VBR;
>> +
>> + if (ret == MFX_CODEC_AVC)
>> + av_log(avctx, AV_LOG_VERBOSE, "Codec:AVC\n");
>> + else if (ret == MFX_CODEC_MPEG2)
>> + av_log(avctx, AV_LOG_VERBOSE, "Codec:MPEG2\n");
>
> VC-1? Maybe make this a switch, too.
>
>> + if (q->param.mfx.GopPicSize)
>> + av_log(avctx, AV_LOG_VERBOSE, "GopPicSize:%d\n",
>> q->param.mfx.GopPicSize);
>> + if (q->param.mfx.GopRefDist)
>> + av_log(avctx, AV_LOG_VERBOSE, "GopRefDist:%d\n",
>> q->param.mfx.GopRefDist);
>> + if (q->param.mfx.NumSlice)
>> + av_log(avctx, AV_LOG_VERBOSE, "NumSlice:%d\n",
>> q->param.mfx.NumSlice);
>> + if (q->param.mfx.NumRefFrame)
>> + av_log(avctx, AV_LOG_VERBOSE, "NumRefFrame:%d\n",
>> q->param.mfx.NumRefFrame);
>> +
>> + switch (q->param.mfx.RateControlMethod) {
>> + case MFX_RATECONTROL_CBR: // API 1.0
>> + av_log(avctx, AV_LOG_VERBOSE, "RateControlMethod:CBR\n");
>> + //q->param.mfx.InitialDelayInKB;
>
> Remove this please, or make it do something useful. Other cases too.
>
>> + q->param.mfx.QPI = av_clip(quant, 0, 51);
>
> Are we sure this is correct for MPEG-2 and VC-1 too?
Absolutely not, good point =)
>
>> +#if 0
>> + case MFX_RATECONTROL_LA: // API 1.7
>> + av_log(avctx, AV_LOG_ERROR,
>> + "RateControlMethod:LA is unimplemented.\n");
>> + /*
>> + q->param.mfx.InitialDelayInKB;
>> + q->param.mfx.TargetKbps;
>> + q->param.mfx.MaxKbps;
>> + q->extco2.LookAheadDepth; // API 1.7
>> + q->extco2.Trellis; // API 1.6
>> + */
>> + return AVERROR(EINVAL);
>> +#endif
>
> Do we really want big disabled blocks like this?
No, we want to merge it using the current stable api at the time, so
hopefully that block would be active code.
>
>> + if (impl & MFX_IMPL_SOFTWARE)
>> + av_log(avctx, AV_LOG_VERBOSE,
>> + "Using Intel QuickSync encoder software implementation.\n");
>> + else if (impl & MFX_IMPL_HARDWARE)
>> + av_log(avctx, AV_LOG_VERBOSE,
>> + "Using Intel QuickSync encoder hardware accelerated
>> implementation.\n");
>> + else
>> + av_log(avctx, AV_LOG_VERBOSE,
>> + "Unknown Intel QuickSync encoder implementation %d.\n",
>> impl);
>
> You can't do it like this:
>
> typedef mfxI32 mfxIMPL;
> #define MFX_IMPL_BASETYPE(x) (0x00ff & (x))
>
> enum {
> MFX_IMPL_AUTO = 0x0000, /* Auto Selection/In or Not
> Supported/Out */
> MFX_IMPL_SOFTWARE = 0x0001, /* Pure Software Implementation */
> MFX_IMPL_HARDWARE = 0x0002, /* Hardware Accelerated Implementation
> (default device) */
> MFX_IMPL_AUTO_ANY = 0x0003, /* Auto selection of any
> hardware/software implementation */
> MFX_IMPL_HARDWARE_ANY = 0x0004, /* Auto selection of any hardware
> implementation */
> MFX_IMPL_HARDWARE2 = 0x0005, /* Hardware accelerated implementation
> (2nd device) */
> MFX_IMPL_HARDWARE3 = 0x0006, /* Hardware accelerated implementation
> (3rd device) */
> MFX_IMPL_HARDWARE4 = 0x0007, /* Hardware accelerated implementation
> (4th device) */
>
> MFX_IMPL_VIA_ANY = 0x0100,
> MFX_IMPL_VIA_D3D9 = 0x0200,
> MFX_IMPL_VIA_D3D11 = 0x0300,
> MFX_IMPL_VIA_VAAPI = 0x0400,
>
> MFX_IMPL_AUDIO = 0x8000,
>
> MFX_IMPL_UNSUPPORTED = 0x0000 /* One of the MFXQueryIMPL returns */
> };
> (MFX_IMPL_HARDWARE2 & MFX_IMPL_HARDWARE) is false
> (MFX_IMPL_HARDWARE2 & MFX_IMPL_SOFTWARE) is true
> (MFX_IMPL_HARDWARE4 & MFX_IMPL_SOFTWARE) is true
> I would switch(MFX_IMPL_BASETYPE(impl))
>
> …and consider hardware 2..4 too.
You are right.
>> + { "level", NULL, OFFSET(qsv.level), AV_OPT_TYPE_INT, { .i64 =
>> MFX_LEVEL_UNKNOWN }, 0, INT_MAX, VE, "level" },
>> + { "unknown", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_UNKNOWN },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "1" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_1 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "1b" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_1b },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "11" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_11 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "12" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_12 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "13" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_13 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "2" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_2 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "21" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_21 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "22" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_22 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "3" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_3 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "31" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_31 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "32" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_32 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "4" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_4 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "41" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_41 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "42" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_42 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "5" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_5 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "51", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_51 },
>> INT_MIN, INT_MAX, VE, "level" },
>> + { "52", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_LEVEL_AVC_52 },
>> INT_MIN, INT_MAX, VE, "level" },
>
> Broken alignment.
>
> Also, I'd make the level names 1.1 instead of 11, etc. as the latter can be
> specified directly (MFX_LEVEL_AVC_11 == 11, etc.).
Sure.
> Not sure about 1 vs. 1.0.
>
>> + { "preset", NULL, OFFSET(qsv.preset), AV_OPT_TYPE_INT, { .i64 =
>> MFX_TARGETUSAGE_BALANCED }, 0, 7, VE },
>
> Maybe use named constants for the limits (MFX_TARGETUSAGE_UNKNOWN,
> MFX_TARGETUSAGE_BEST_SPEED)?
Sounds good.
>> + { "fast", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
>> MFX_TARGETUSAGE_BEST_SPEED }, INT_MIN, INT_MAX, VE, "preset" },
>> + { "medium", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
>> MFX_TARGETUSAGE_BALANCED }, INT_MIN, INT_MAX, VE, "preset" },
>> + { "slow", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
>> MFX_TARGETUSAGE_BEST_QUALITY }, INT_MIN, INT_MAX, VE, "preset" },
>
> I'd call them speed, balanced and quality. Thoughts?
Sure.
Thanks a lot for the input =)
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
