diff mbox

[FFmpeg-devel] nvenc: Make AUD optional for h264_nvenc and hevc_nvenc

Message ID 5866CC2C.40706@email.cz
State New
Headers show

Commit Message

Miroslav Slugeň Dec. 30, 2016, 9:05 p.m. UTC 
Dne 30.12.2016 v 17:38 Mark Thompson napsal(a):
> On 30/12/16 16:12, Miroslav Slugeň wrote:
>> Dne 30.12.2016 v 15:32 Mark Thompson napsal(a):
>>> On 29/12/16 21:02, Miroslav Slugeň wrote:
>>>> Somebody changed AUD to active in NVENC by default, which is not very clever, libx264 also has this future disabled, so we should stay in sync with libx264 behavior.
>>>>
>>>> Enabled AUD will work only without B-frames. There is BUG in Nvidia NVENC when you use AUD for H264 with B-frames, it will return corrupted stream, because NVIDIA is inserting AUD type 0 (I-frame) before B-frames instead of AUD type 7 (any-frame).
>>>>
>>>> H264 encoded with B-frames and AUD active will not play for example on Panasonic TX-AS640E, other decoders just ignore wrong AUD type.
>>>>
>>>>   From 8205523dfa477eaaeda3e10d59a42e024dafbfdb Mon Sep 17 00:00:00 2001
>>>> From: Miroslav Slugen <thunder.m@email.cz>
>>>> Date: Thu, 29 Dec 2016 21:50:13 +0100
>>>> Subject: [PATCH 1/1] NVENC: Make AUD optional
>>>>
>>>> ---
>>>>    libavcodec/nvenc.c      | 4 ++--
>>>>    libavcodec/nvenc.h      | 1 +
>>>>    libavcodec/nvenc_h264.c | 1 +
>>>>    libavcodec/nvenc_hevc.c | 1 +
>>>>    4 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
>>>> index f64fd8a..d57a90c 100644
>>>> --- a/libavcodec/nvenc.c
>>>> +++ b/libavcodec/nvenc.c
>>>> @@ -756,7 +756,7 @@ static av_cold int nvenc_setup_h264_config(AVCodecContext *avctx)
>>>>          h264->disableSPSPPS = (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) ? 1 : 0;
>>>>        h264->repeatSPSPPS  = (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) ? 0 : 1;
>>>> -    h264->outputAUD     = 1;
>>>> +    h264->outputAUD     = (ctx->aud == 1) ? 1 : 0;
>>> Just write ctx->aud, no need for the conditional.
>> I understand, but in our implementation we reserved -1 for autodetection which could be done later based on other parameters of encoder. Should i then rewrite it to: h264->outputAUD = ctx->aud; now, until autodetection is complete?
> I don't mind; if it makes a following patch look simpler then sure, leave it.
>
> I'm curious how this autodetection can work, though.  In general you prefer never to have AUD enabled unless required (to avoid issues like your own problem this is fixing, and because it is just redundant padding which increases the size of the stream to no gain), so the cases where you want it enabled are going to be where it is an absolute requirement anyway (such as bluray), hence I see no meaningful "auto" case.
>
>>>>          if (avctx->refs >= 0) {
>>>>            /* 0 means "let the hardware decide" */
>>>> @@ -840,7 +840,7 @@ static av_cold int nvenc_setup_hevc_config(AVCodecContext *avctx)
>>>>          hevc->disableSPSPPS = (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) ? 1 : 0;
>>>>        hevc->repeatSPSPPS  = (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) ? 0 : 1;
>>>> -    hevc->outputAUD     = 1;
>>>> +    hevc->outputAUD     = (ctx->aud == 1) ? 1 : 0;
>>> Similarly here.
>>>
>>>>          if (avctx->refs >= 0) {
>>>>            /* 0 means "let the hardware decide" */
>>>> diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
>>>> index 5bc0cba..c435e05 100644
>>>> --- a/libavcodec/nvenc.h
>>>> +++ b/libavcodec/nvenc.h
>>>> @@ -153,6 +153,7 @@ typedef struct NvencContext
>>>>        int strict_gop;
>>>>        int aq_strength;
>>>>        int quality;
>>>> +    int aud;
>>>>    } NvencContext;
>>>>      int ff_nvenc_encode_init(AVCodecContext *avctx);
>>>> diff --git a/libavcodec/nvenc_h264.c b/libavcodec/nvenc_h264.c
>>>> index 71e27fd..cae27aa 100644
>>>> --- a/libavcodec/nvenc_h264.c
>>>> +++ b/libavcodec/nvenc_h264.c
>>>> @@ -107,6 +107,7 @@ static const AVOption options[] = {
>>>>                                                                OFFSET(aq_strength),  AV_OPT_TYPE_INT,   { .i64 = 8 }, 1, 15, VE },
>>>>        { "cq",           "Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control",
>>>>                                                                OFFSET(quality),      AV_OPT_TYPE_INT,   { .i64 = 0 }, 0, 51, VE },
>>>> +    { "aud",          "Use access unit delimiters",         OFFSET(aud),          AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
>>>>        { NULL }
>>>>    };
>>>>    diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c
>>>> index ef739b6..e44ca7e 100644
>>>> --- a/libavcodec/nvenc_hevc.c
>>>> +++ b/libavcodec/nvenc_hevc.c
>>>> @@ -104,6 +104,7 @@ static const AVOption options[] = {
>>>>                                                                OFFSET(aq_strength),  AV_OPT_TYPE_INT,   { .i64 = 8 }, 1, 15, VE },
>>>>        { "cq",           "Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control",
>>>>                                                                OFFSET(quality),      AV_OPT_TYPE_INT,   { .i64 = 0 }, 0, 51, VE },
>>>> +    { "aud",          "Use access unit delimiters",         OFFSET(aud),          AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
>>>>        { NULL }
>>>>    };
>>>>    --
>>>> 2.1.4
>>>>
>>> LGTM otherwise.
>>>
>>> Thanks,
>>>
>>> - Mark
>> Thanks :)
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Resending with requested modifications and basic implementation of 
bluray_compat function which enables AUD.

Comments

Timo Rothenpieler Jan. 1, 2017, 1:53 p.m. UTC | #1 
Sounds and looks fine to me.
I agree that having AUD enabled by default wasn't the best solution, as
it's useless overhead for most cases.

both applied
Kieran Kunhya Jan. 1, 2017, 1:57 p.m. UTC | #2 
>
> Resending with requested modifications and basic implementation of
> bluray_compat function which enables AUD.
>
>
>
> -
>

This is a misleading option name because Blu-ray compatibility is a lot
more than what you implement. There are lots of rules I implemented in x264
years ago.

Kieran

>
Miroslav Slugeň Jan. 1, 2017, 5:27 p.m. UTC | #3 
Dne 1.1.2017 v 14:57 Kieran Kunhya napsal(a):
>> Resending with requested modifications and basic implementation of
>> bluray_compat function which enables AUD.
>>
>>
>>
>> -
>>
> This is a misleading option name because Blu-ray compatibility is a lot
> more than what you implement. There are lots of rules I implemented in x264
> years ago.
>
> Kieran
Yes, it is not yet fully completed, it will require much more changes 
and tests of encoded samples. But i think it is good start... Maybe we 
could print some warnings about using this function for now.

>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

From 1440d075607ad3f1a6a535f7132719af3ab86cef Mon Sep 17 00:00:00 2001
From: Miroslav Slugen <thunder.m@email.cz>
Date: Fri, 30 Dec 2016 22:04:31 +0100
Subject: [PATCH 2/2] NVENC: Add bluray_compat basic implementation

---
 libavcodec/nvenc.c      | 15 +++++++++++++++
 libavcodec/nvenc.h      |  1 +
 libavcodec/nvenc_h264.c |  1 +
 libavcodec/nvenc_hevc.c |  1 +
 4 files changed, 18 insertions(+)

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 306594b..78f4d1d 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -958,6 +958,21 @@  static av_cold int nvenc_setup_encoder(AVCodecContext *avctx)
     ctx->init_encode_params.enableEncodeAsync = 0;
     ctx->init_encode_params.enablePTD = 1;
 
+    if (ctx->bluray_compat) {
+        ctx->aud = 1;
+        avctx->refs = FFMIN(FFMAX(avctx->refs, 0), 6);
+        avctx->max_b_frames = FFMIN(avctx->max_b_frames, 3);
+        switch (avctx->codec->id) {
+        case AV_CODEC_ID_H264:
+            /* maximum level depends on used resolution */
+            break;
+        case AV_CODEC_ID_HEVC:
+            ctx->level = NV_ENC_LEVEL_HEVC_51;
+            ctx->tier = NV_ENC_TIER_HEVC_HIGH;
+            break;
+        }
+    }
+
     if (avctx->gop_size > 0) {
         if (avctx->max_b_frames >= 0) {
             /* 0 is intra-only, 1 is I/P only, 2 is one B-Frame, 3 two B-frames, and so on. */
diff --git a/libavcodec/nvenc.h b/libavcodec/nvenc.h
index c435e05..ab48cf7 100644
--- a/libavcodec/nvenc.h
+++ b/libavcodec/nvenc.h
@@ -154,6 +154,7 @@  typedef struct NvencContext
     int aq_strength;
     int quality;
     int aud;
+    int bluray_compat;
 } NvencContext;
 
 int ff_nvenc_encode_init(AVCodecContext *avctx);
diff --git a/libavcodec/nvenc_h264.c b/libavcodec/nvenc_h264.c
index cae27aa..76dc48b 100644
--- a/libavcodec/nvenc_h264.c
+++ b/libavcodec/nvenc_h264.c
@@ -108,6 +108,7 @@  static const AVOption options[] = {
     { "cq",           "Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control",
                                                             OFFSET(quality),      AV_OPT_TYPE_INT,   { .i64 = 0 }, 0, 51, VE },
     { "aud",          "Use access unit delimiters",         OFFSET(aud),          AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
+    { "bluray-compat", "Bluray compatibility workarounds",  OFFSET(bluray_compat),AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
     { NULL }
 };
 
diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c
index e44ca7e..cd1dcb9 100644
--- a/libavcodec/nvenc_hevc.c
+++ b/libavcodec/nvenc_hevc.c
@@ -105,6 +105,7 @@  static const AVOption options[] = {
     { "cq",           "Set target quality level (0 to 51, 0 means automatic) for constant quality mode in VBR rate control",
                                                             OFFSET(quality),      AV_OPT_TYPE_INT,   { .i64 = 0 }, 0, 51, VE },
     { "aud",          "Use access unit delimiters",         OFFSET(aud),          AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
+    { "bluray-compat", "Bluray compatibility workarounds",  OFFSET(bluray_compat),AV_OPT_TYPE_BOOL,  { .i64 = 0 }, 0, 1, VE },
     { NULL }
 };
 
-- 
2.1.4