[vlc-devel] [PATCH] Intel Video QuickSync encoder support

[Rafaël Carré]

Hello Julien,

A few small suggestions and one question, the rest looks OK.

Le 29/05/2013 18:34, Julien 'Lta' BALLET a écrit :
> Bonjour,
> 
> Third version of the patch with a few tuning of the default parameter
> sets after a suggestion from Intel as well as a few fixes after
> previous review's comments.
> 
> Regards,
> Julien 'Lta' BALLET
> 
> 
> 0001-Adds-Intel-QuickSync-video-encoder.patch
> 
> 
> From 145f2116348ae507ef4954623617edad026f6fa4 Mon Sep 17 00:00:00 2001
> From: Julien 'Lta' BALLET <contact at lta.io>
> Date: Wed, 29 May 2013 14:09:46 +0200
> Subject: [PATCH] Adds Intel QuickSync video encoder
> 
> ---
>  configure.ac             |   5 +
>  modules/codec/Modules.am |   1 +
>  modules/codec/qsv.c      | 749 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 755 insertions(+)
>  create mode 100644 modules/codec/qsv.c


> +static uint64_t qsv_params_get_value(const char *const *text,
> +                                     const int const *list,
> +                                     size_t size, char *sel)
> +{
> +    size /= sizeof(int);

/= sizeof(list[0]);

Will help if ever list type change in the future (e.g. to long)

> +
> +    for (size_t i = 0; i < size; i++)
> +        if (!strcmp(sel, text[i]))
> +            return list[i];
> +
> +    // The default value is always the first item in the array. So if not found, use it.
> +    return list[0];
> +}


> +        sys->params.mfx.CodecProfile = qsv_params_get_value((const char * const*)profile_mpeg2_text,

If you have a warning about const char * const * versus const char[]
const* you can use &profile_mpeg2_text[0] instead of casting.

> +/*
> + * Convert the Intel Media SDK's timestamps into VLC's format.
> + */
> +static void qsv_set_block_ts(encoder_t *enc, encoder_sys_t *sys, block_t *block, mfxBitstream *bs)
> +{
> +    if (bs->TimeStamp) {

You can use:

if (!bs->TimeStamp)
	return;

And deindent the rest of the function.

> +        block->i_pts = qsv_timestamp_to_mtime(bs->TimeStamp) + sys->offset_pts;
> +        if (bs->DecodeTimeStamp && bs->DecodeTimeStamp <= bs->TimeStamp)
> +            block->i_dts = qsv_timestamp_to_mtime(bs->DecodeTimeStamp) + sys->offset_pts;
> +        else {
> +            /* HW encoder (sometimes) doesn't set the DecodeTimeStamp field
> +               so we give decoder some (100ms) headroom. */

Not sure I understand what you are doing here, if you set the h264 dts,
shouldn't it take in account p / b frames ? Isn't it going to conflict
with the dts that were set correctly ?

> +            msg_Dbg(enc, "Encode returning empty DTS or DTS > PTS");
> +            if (block->i_pts <= 100 * 1000)
> +                block->i_dts = 0;
> +            else
> +                block->i_dts = block->i_pts - 100 * 1000;
> +        }
> +    }
> +}