Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On 11/11/2016 4:01 PM, Timo Rothenpieler wrote: > -1 and 0 both mean false now, and I left in the option to pass -1 to > stay compatible with possible 3rd parties who pass it. Well that's certainly non-obvious... > So changing to default to 0 doesn't really matter, and I decided to keep > the patch minimal. > Changing to default to 0 would be fine as well. This patch LGTM if you push a 2nd patch with it that changes the default to 0. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
> Actually, looking closer, the default is still -1. > > Thus either this patch us incorrect, or the default should be changed to be 0. -1 and 0 both mean false now, and I left in the option to pass -1 to stay compatible with possible 3rd parties who pass it. So changing to default to 0 doesn't really matter, and I decided to keep the patch minimal. Changing to default to 0 would be fine as well. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On 11/11/2016 3:26 PM, Derek Buitenhuis wrote: > This patch LGTM. > > When I added this option in c981b1145a857c8f962c93b8eecb1c613b20ffe9, the > type was > INT, and the default was -1, and it was correct. It was later broken in > fb99ef0bd39a1859d0e65c6c16caa8e17dd3cfbe. I suggest looking at everything > touched > in fb99ef0bd39a1859d0e65c6c16caa8e17dd3cfbe to make sure nothing else was > similarily > broken. Actually, looking closer, the default is still -1. Thus either this patch us incorrect, or the default should be changed to be 0. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On 11/11/2016 2:30 PM, Michael Niedermayer wrote: > if noone adds suport fo the 3rd constant then the patch should > likely be applied, unless i miss something This patch LGTM. When I added this option in c981b1145a857c8f962c93b8eecb1c613b20ffe9, the type was INT, and the default was -1, and it was correct. It was later broken in fb99ef0bd39a1859d0e65c6c16caa8e17dd3cfbe. I suggest looking at everything touched in fb99ef0bd39a1859d0e65c6c16caa8e17dd3cfbe to make sure nothing else was similarily broken. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On Thu, Oct 13, 2016 at 05:32:54PM +0200, Michael Niedermayer wrote: > On Wed, Oct 12, 2016 at 09:57:16PM +0200, Timo Rothenpieler wrote: > > Currently, it forces IDR frames for both true and false. > > Not entirely sure what the original idea behind the tri-state bool > > option is. > > --- > > libavcodec/libx264.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > there is X264_TYPE_IDR, X264_TYPE_KEYFRAME and X264_TYPE_I > > so there are 3 possibilities, no idea if that was the original idea ping if noone adds suport fo the 3rd constant then the patch should likely be applied, unless i miss something [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On Wed, Oct 12, 2016 at 09:57:16PM +0200, Timo Rothenpieler wrote: > Currently, it forces IDR frames for both true and false. > Not entirely sure what the original idea behind the tri-state bool > option is. > --- > libavcodec/libx264.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) there is X264_TYPE_IDR, X264_TYPE_KEYFRAME and X264_TYPE_I so there are 3 possibilities, no idea if that was the original idea [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
On Wed, Oct 12, 2016 at 21:57:16 +0200, Timo Rothenpieler wrote:
> Currently, it forces IDR frames for both true and false.
> Not entirely sure what the original idea behind the tri-state bool
> option is.
The option's default of -1 ("auto") doesn't seem useful either, since
the code doesn't do any sort of automatic handling. The default (and
the minimum) might as well be 0 now, with your change.
Moritz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/libx264: fix forced_idr logic
Currently, it forces IDR frames for both true and false.
Not entirely sure what the original idea behind the tri-state bool
option is.
---
libavcodec/libx264.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 9e12464..32e767e 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -293,8 +293,8 @@ static int X264_frame(AVCodecContext *ctx, AVPacket *pkt,
const AVFrame *frame,
switch (frame->pict_type) {
case AV_PICTURE_TYPE_I:
-x4->pic.i_type = x4->forced_idr >= 0 ? X264_TYPE_IDR
- : X264_TYPE_KEYFRAME;
+x4->pic.i_type = x4->forced_idr > 0 ? X264_TYPE_IDR
+: X264_TYPE_KEYFRAME;
break;
case AV_PICTURE_TYPE_P:
x4->pic.i_type = X264_TYPE_P;
--
2.10.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
