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?
> +#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?
> + 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.
> + { "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.).
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)?
> + { "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?
Tim
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
