Thanks Diego, Luca. Will take care of your comments for the next submissions.
Few points based on your review :

1. We can keep the default as HQ only instead of medium (mapping of medium is 
already to HQ), however we found of lot of people comfortable using slow, 
medium and fast, similar to what they find in x264. And that is the motivation 
of adding these, and making one of these defaults. At some point we may even 
have these kind of presets only, nothing decided as yet.
2. Am bad at writing, will try to improve the log message next time
3. I couldn't find any entry for Range extensions for HEVC, so added, if you 
feel it already exists let me know, and I will try to put a patch with the 
existing one.
4. The preset change may not be stray, the GUIDs have to be in the array in the 
same order as they are referred by. LLHP and LLHQ somehow got interchanged and 
that is why this change, let me know if I got this wrong.
5. Will follow the notation more closely as suggested by you.

Thanks,
Yogender

-----Original Message-----
From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf Of Diego 
Biurrun
Sent: Friday, September 16, 2016 1:27 AM
To: libav development
Subject: Re: [libav-devel] [Libav-devel][Patch] Patch for SDK 7 for NVENC

Some comments for next time as Luca said he wanted to take care of it.
Please try to maintain the style of the file you are working in.

> --- a/libavcodec/nvenc_hevc.c
> +++ b/libavcodec/nvenc_hevc.c
> @@ -27,8 +27,11 @@
>  #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM  
> static const AVOption options[] = {
> -    { "preset",   "Set the encoding preset",              OFFSET(preset),    
>   AV_OPT_TYPE_INT,    { .i64 = PRESET_HQ }, PRESET_DEFAULT, 
> PRESET_LOSSLESS_HP, VE, "preset" },
> +    { "preset",   "Set the encoding preset",              OFFSET(preset),    
>   AV_OPT_TYPE_INT,    { .i64 = PRESET_MEDIUM }, PRESET_DEFAULT, 
> PRESET_LOSSLESS_HP, VE, "preset" },
> --- a/libavcodec/nvenc_h264.c
> +++ b/libavcodec/nvenc_h264.c
> @@ -27,8 +27,11 @@
>  #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM  
> static const AVOption options[] = {
> -    { "preset",   "Set the encoding preset",              OFFSET(preset),    
>   AV_OPT_TYPE_INT,    { .i64 = PRESET_HQ }, PRESET_DEFAULT, 
> PRESET_LOSSLESS_HP, VE, "preset" },
> +    { "preset",   "Set the encoding preset",              OFFSET(preset),    
>   AV_OPT_TYPE_INT,    { .i64 = PRESET_MEDIUM }, PRESET_DEFAULT, 
> PRESET_LOSSLESS_HP, VE, "preset" },

Why are you changing the default from HQ to MEDIUM?

On Thu, Sep 15, 2016 at 10:40:10AM +0000, Yogender Gupta wrote:
> From cc4f606cfba7b1ced644fe597b1b424f2af0f2bc Mon Sep 17 00:00:00 2001
> From: Yogender Gupta <ygu...@nvidia.com>
> Date: Thu, 15 Sep 2016 16:06:44 +0530
> Subject: [PATCH] Patch for SDK 7 for NVENC

The log message should be

  nvenc: Update for SDK 7

or similar.

> ---
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/nvenc.c      | 117 
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  libavcodec/nvenc.h      |  23 +++++++++-
>  libavcodec/nvenc_h264.c |  15 ++++++-  libavcodec/nvenc_hevc.c |  19 
> ++++++--
>  5 files changed, 166 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 
> 7a5f10f..607688c 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2961,6 +2961,7 @@ typedef struct AVCodecContext {
>  #define FF_PROFILE_HEVC_MAIN                        1
>  #define FF_PROFILE_HEVC_MAIN_10                     2
>  #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
> +#define FF_PROFILE_HEVC_REXT                        4

This is an installed header, so there should be a matching APIchanges entry.

> --- a/libavcodec/nvenc.c
> +++ b/libavcodec/nvenc.c
> @@ -463,12 +471,15 @@ static int nvec_map_preset(NVENCContext *ctx)
>      GUIDTuple presets[] = {
>          { NV_ENC_PRESET_BD_GUID },
>          { NV_ENC_PRESET_LOW_LATENCY_DEFAULT_GUID, NVENC_LOWLATENCY },
> -        { NV_ENC_PRESET_LOW_LATENCY_HP_GUID,      NVENC_LOWLATENCY },
>          { NV_ENC_PRESET_LOW_LATENCY_HQ_GUID,      NVENC_LOWLATENCY },
> +        { NV_ENC_PRESET_LOW_LATENCY_HP_GUID,      NVENC_LOWLATENCY },
>          { NV_ENC_PRESET_LOSSLESS_DEFAULT_GUID,    NVENC_LOSSLESS },
>          { NV_ENC_PRESET_LOSSLESS_HP_GUID,         NVENC_LOSSLESS },

That looks like a stray change.

> @@ -589,6 +600,44 @@ static void 
> nvenc_setup_rate_control(AVCodecContext *avctx)
>  
>      if (rc->averageBitRate > 0)
>          avctx->bit_rate = rc->averageBitRate;
> +
> +    if (ctx->aq)
> +    {

  if (ctx->aq) {

like in the rest of the file, more below.

> +        av_log(avctx, AV_LOG_INFO, "AQ Enabled\n");

"enabled" :)

> +            av_log(avctx, AV_LOG_WARNING, "Lookahead Not Enabled. 
> + Increase Buffer Delay (-delay) \n");

Let's not capitalize every single word :)

> +        }
> +        else {

  } else {

> +            ctx->config.rcParams.enableLookahead = 1;
> +            ctx->config.rcParams.lookaheadDepth = av_clip(ctx->rc_lookahead, 
> 0, lkd_bound);
> +            ctx->config.rcParams.disableIadapt = ctx->no_scenecut;
> +            ctx->config.rcParams.disableBadapt = !ctx->b_adapt;

Align the = for better readability.

> +            av_log(avctx, AV_LOG_INFO, "Lookahead Enabled with depth %d, 
> Scenecut %s, B-Adapt %s\n",
> +                ctx->config.rcParams.lookaheadDepth, 
> ctx->config.rcParams.disableIadapt ? "Disabled" : "Enabled",
> +                ctx->config.rcParams.disableBadapt ? "Disabled" : 
> + "Enabled");

Indent the next line to align with the function parameters, like so:

  av_log(avctx, AV_LOG_INFO, "Lookahead Enabled with depth %d, Scenecut %s, 
B-Adapt %s\n",
         ctx->config.rcParams.lookaheadDepth, 
ctx->config.rcParams.disableIadapt ? "Disabled" : "Enabled",
         ctx->config.rcParams.disableBadapt ? "Disabled" : "Enabled");

> @@ -692,9 +741,38 @@ static int nvenc_setup_hevc_config(AVCodecContext 
> *avctx)
>  
> -    /* No other profile is supported in the current SDK version 5 */
> -    cc->profileGUID = NV_ENC_HEVC_PROFILE_MAIN_GUID;
> -    avctx->profile  = FF_PROFILE_HEVC_MAIN;
> +    switch(ctx->profile) {

switch (

> --- a/libavcodec/nvenc.h
> +++ b/libavcodec/nvenc.h
> @@ -160,6 +171,16 @@ typedef struct NVENCContext {
>      int device;
>      int flags;
>      int async_depth;
> +    int rc_lookahead;
> +    int aq;
> +    int no_scenecut;
> +    int b_adapt;
> +    int temporalAQ;
> +    int zeroReorderDelay;
> +    int enableNonRefP;
> +    int strictGOPTarget;
> +    int aqStrength;
> +    int targetQuality;

The rest of the struct members are not CamelCase, so these should rather use _ 
IMO.

Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to