diff mbox

[FFmpeg-devel] nvenc: FIX wrong forced keyframe behavior

Message ID 58A097A9.5010702@email.cz
State New
Headers show

Commit Message

Miroslav Slugeň Feb. 12, 2017, 5:13 p.m. UTC 
There was error in forcing key frames. If IDR was set to -1 (default) 
forced key frame will be never propagated to encoder.

I also suggest to backport this patch to current stable version, because 
-forced_keyframe behavior in NVENC is actualy broken.

Comments

Timo Rothenpieler Feb. 12, 2017, 7:29 p.m. UTC | #1 
The current behavior is intended like it is.
On the default of -1, it does not care about I/IDR frame requests, in
mode 0 it will generate intra frames, and in mode 1 it will generate
full IDR frames.
Miroslav Slugeň Feb. 12, 2017, 7:43 p.m. UTC | #2 
Dne 12.2.2017 v 20:29 Timo Rothenpieler napsal(a):
> The current behavior is intended like it is.
> On the default of -1, it does not care about I/IDR frame requests, in
> mode 0 it will generate intra frames, and in mode 1 it will generate
> full IDR frames.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This is not clever, because i checked both libx264 and libx265 and both 
of them has same behavior like i proposed in patch.

Also explanation of this function is than wrong:
{ "forced-idr",   "If forcing keyframes, force them as IDR frames."}

There is nothing about disabled forcing keyframes at all.

So we have two options:
1. We should change it in NVENC
or
2. We should change it in libx264 and libx265

M.
Timo Rothenpieler Feb. 12, 2017, 7:52 p.m. UTC | #3 
On 2/12/2017 8:43 PM, Miroslav Slugeň wrote:
> Dne 12.2.2017 v 20:29 Timo Rothenpieler napsal(a):
>> The current behavior is intended like it is.
>> On the default of -1, it does not care about I/IDR frame requests, in
>> mode 0 it will generate intra frames, and in mode 1 it will generate
>> full IDR frames.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> This is not clever, because i checked both libx264 and libx265 and both
> of them has same behavior like i proposed in patch.
> 
> Also explanation of this function is than wrong:
> { "forced-idr",   "If forcing keyframes, force them as IDR frames."}
> 
> There is nothing about disabled forcing keyframes at all.
> 
> So we have two options:
> 1. We should change it in NVENC
> or
> 2. We should change it in libx264 and libx265

Changing it in nvenc now would be kind of an API break, as the behaviour
might change without someone expecting it.
Miroslav Slugeň Feb. 12, 2017, 7:56 p.m. UTC | #4 
Dne 12.2.2017 v 20:52 Timo Rothenpieler napsal(a):
> On 2/12/2017 8:43 PM, Miroslav Slugeň wrote:
>> Dne 12.2.2017 v 20:29 Timo Rothenpieler napsal(a):
>>> The current behavior is intended like it is.
>>> On the default of -1, it does not care about I/IDR frame requests, in
>>> mode 0 it will generate intra frames, and in mode 1 it will generate
>>> full IDR frames.
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> This is not clever, because i checked both libx264 and libx265 and both
>> of them has same behavior like i proposed in patch.
>>
>> Also explanation of this function is than wrong:
>> { "forced-idr",   "If forcing keyframes, force them as IDR frames."}
>>
>> There is nothing about disabled forcing keyframes at all.
>>
>> So we have two options:
>> 1. We should change it in NVENC
>> or
>> 2. We should change it in libx264 and libx265
> Changing it in nvenc now would be kind of an API break, as the behaviour
> might change without someone expecting it.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I don't think so, onlything is changed is when you forcing keyframes 
with -forced_keyframes option it was not working in NVENC without 
specifiing -force_idr 0 or 1, is there any other situation when some 
parts of ffmpeg will force key frames and NVENC naturaly ignore them?

M.
Timo Rothenpieler Feb. 12, 2017, 8:01 p.m. UTC | #5 
On 2/12/2017 8:56 PM, Miroslav Slugeň wrote:
>>> 2. We should change it in libx264 and libx265
>> Changing it in nvenc now would be kind of an API break, as the behaviour
>> might change without someone expecting it.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> I don't think so, onlything is changed is when you forcing keyframes
> with -forced_keyframes option it was not working in NVENC without
> specifiing -force_idr 0 or 1, is there any other situation when some
> parts of ffmpeg will force key frames and NVENC naturaly ignore them?

It's primarily a concern with API users which might set the pict_type.
Probably does not affect any notable amount of users.
Might as well remove -1 as a supported value from the option then.
Miroslav Slugeň Feb. 12, 2017, 8:09 p.m. UTC | #6 
Dne 12.2.2017 v 21:01 Timo Rothenpieler napsal(a):
> On 2/12/2017 8:56 PM, Miroslav Slugeň wrote:
>>>> 2. We should change it in libx264 and libx265
>>> Changing it in nvenc now would be kind of an API break, as the behaviour
>>> might change without someone expecting it.
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> I don't think so, onlything is changed is when you forcing keyframes
>> with -forced_keyframes option it was not working in NVENC without
>> specifiing -force_idr 0 or 1, is there any other situation when some
>> parts of ffmpeg will force key frames and NVENC naturaly ignore them?
> It's primarily a concern with API users which might set the pict_type.
> Probably does not affect any notable amount of users.
> Might as well remove -1 as a supported value from the option then.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I checked libx264 and there is -1 option, so i think it should stay 
there also for future use (like automatic IDR/INTRA decisions). But it 
could be removed also.

M.
diff mbox

Patch

From 60a1efef9d4146f9ad38a779ebf05407c64e385d Mon Sep 17 00:00:00 2001
From: Miroslav Slugen <thunder.m@email.cz>
Date: Sun, 12 Feb 2017 18:10:15 +0100
Subject: [PATCH 1/1] nvenc: fix wrong forced keyframe behavior

---
 libavcodec/nvenc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/libavcodec/nvenc.c b/libavcodec/nvenc.c
index 7005465..20af109 100644
--- a/libavcodec/nvenc.c
+++ b/libavcodec/nvenc.c
@@ -1687,9 +1687,10 @@  int ff_nvenc_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
             pic_params.pictureStruct = NV_ENC_PIC_STRUCT_FRAME;
         }
 
-        if (ctx->forced_idr >= 0 && frame->pict_type == AV_PICTURE_TYPE_I) {
-            pic_params.encodePicFlags =
-                ctx->forced_idr ? NV_ENC_PIC_FLAG_FORCEIDR : NV_ENC_PIC_FLAG_FORCEINTRA;
+        if (frame->pict_type == AV_PICTURE_TYPE_I) {
+            pic_params.encodePicFlags = ctx->forced_idr > 0
+                                        ? NV_ENC_PIC_FLAG_FORCEIDR
+                                        : NV_ENC_PIC_FLAG_FORCEINTRA;
         } else {
             pic_params.encodePicFlags = 0;
         }
-- 
2.1.4