Please don't top-post :)

On Fri, Sep 16, 2016 at 12:50:43PM +0000, Yogender Gupta wrote:
> 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.

This should be a separate change and not grouped in with the other
changes.

> 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.

May or may not? :)

This should be a separate change as well.

Bug fixes and behavior changes do not belong together with an API
update.

> -----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
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to