| Message ID | 20161012195716.11304-1-timo@rothenpieler.org |
|---|---|
| State | Accepted |
| Headers | show |
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
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 [...]
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 [...]
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
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
> 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.
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
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;