[3/7] x265: Use the framerate information instead of the timebase

Message ID 20180216170209.52876-4-lu_zero@gentoo.org
State New
Headers show
Series
  • [1/7] avcodec: Always fill the encoder target framerate
Related show

Commit Message

Luca Barbato Feb. 16, 2018, 5:02 p.m.
Unbreaks the rate-control behaviour.
---
 libavcodec/libx265.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Diego Biurrun Feb. 16, 2018, 5:28 p.m. | #1
On Fri, Feb 16, 2018 at 06:02:05PM +0100, Luca Barbato wrote:
> Unbreaks the rate-control behaviour.

How does this unbreak what?

Diego
Luca Barbato Feb. 16, 2018, 7:23 p.m. | #2
On 16/02/2018 18:28, Diego Biurrun wrote:
> On Fri, Feb 16, 2018 at 06:02:05PM +0100, Luca Barbato wrote:
>> Unbreaks the rate-control behaviour.
> 
> How does this unbreak what?
> 

If your timebase is 1/1000 and your actual frame rate is 30, it gets 
extra creative in deciding how many bits allocate per frame.

lu
wm4 Feb. 16, 2018, 11:57 p.m. | #3
On Fri, 16 Feb 2018 18:02:05 +0100
Luca Barbato <lu_zero@gentoo.org> wrote:

> Unbreaks the rate-control behaviour.
> ---
>  libavcodec/libx265.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
> index fd5452193b..73aff2caef 100644
> --- a/libavcodec/libx265.c
> +++ b/libavcodec/libx265.c
> @@ -116,8 +116,8 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx)
>      }
>  
>      ctx->params->frameNumThreads = avctx->thread_count;
> -    ctx->params->fpsNum          = avctx->time_base.den;
> -    ctx->params->fpsDenom        = avctx->time_base.num * avctx->ticks_per_frame;
> +    ctx->params->fpsNum          = avctx->framerate.num;
> +    ctx->params->fpsDenom        = avctx->framerate.den * avctx->ticks_per_frame;
>      ctx->params->sourceWidth     = avctx->width;
>      ctx->params->sourceHeight    = avctx->height;
>      ctx->params->bEnablePsnr     = !!(avctx->flags & AV_CODEC_FLAG_PSNR);

Doesn't this use a potentially broken demuxer reported framerate?
(Not uncommon with mkv files at least.)
Luca Barbato Feb. 17, 2018, 2:15 a.m. | #4
On 17/02/2018 00:57, wm4 wrote:
> On Fri, 16 Feb 2018 18:02:05 +0100
> Luca Barbato <lu_zero@gentoo.org> wrote:
> 
>> Unbreaks the rate-control behaviour.
>> ---
>>   libavcodec/libx265.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>> index fd5452193b..73aff2caef 100644
>> --- a/libavcodec/libx265.c
>> +++ b/libavcodec/libx265.c
>> @@ -116,8 +116,8 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx)
>>       }
>>   
>>       ctx->params->frameNumThreads = avctx->thread_count;
>> -    ctx->params->fpsNum          = avctx->time_base.den;
>> -    ctx->params->fpsDenom        = avctx->time_base.num * avctx->ticks_per_frame;
>> +    ctx->params->fpsNum          = avctx->framerate.num;
>> +    ctx->params->fpsDenom        = avctx->framerate.den * avctx->ticks_per_frame;
>>       ctx->params->sourceWidth     = avctx->width;
>>       ctx->params->sourceHeight    = avctx->height;
>>       ctx->params->bEnablePsnr     = !!(avctx->flags & AV_CODEC_FLAG_PSNR);
> 
> Doesn't this use a potentially broken demuxer reported framerate?
> (Not uncommon with mkv files at least.)

It is better than a definitely broken timebase.

Before you usually feed 1000 fps or worse, now it has at least a chance 
to have a correct framerate by default.

lu
Vittorio Giovara Feb. 17, 2018, 3:50 a.m. | #5
On Fri, Feb 16, 2018 at 9:15 PM, Luca Barbato <lu_zero@gentoo.org> wrote:

> On 17/02/2018 00:57, wm4 wrote:
>
>> On Fri, 16 Feb 2018 18:02:05 +0100
>> Luca Barbato <lu_zero@gentoo.org> wrote:
>>
>> Unbreaks the rate-control behaviour.
>>>
>>
Would be nice that the commit message is expanded, explaining what is
currently broken and how this fixes it.


> ---
>>>   libavcodec/libx265.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
>>> index fd5452193b..73aff2caef 100644
>>> --- a/libavcodec/libx265.c
>>> +++ b/libavcodec/libx265.c
>>> @@ -116,8 +116,8 @@ static av_cold int libx265_encode_init(AVCodecContext
>>> *avctx)
>>>       }
>>>         ctx->params->frameNumThreads = avctx->thread_count;
>>> -    ctx->params->fpsNum          = avctx->time_base.den;
>>> -    ctx->params->fpsDenom        = avctx->time_base.num *
>>> avctx->ticks_per_frame;
>>> +    ctx->params->fpsNum          = avctx->framerate.num;
>>> +    ctx->params->fpsDenom        = avctx->framerate.den *
>>> avctx->ticks_per_frame;
>>>       ctx->params->sourceWidth     = avctx->width;
>>>       ctx->params->sourceHeight    = avctx->height;
>>>       ctx->params->bEnablePsnr     = !!(avctx->flags &
>>> AV_CODEC_FLAG_PSNR);
>>>
>>
>> Doesn't this use a potentially broken demuxer reported framerate?
>> (Not uncommon with mkv files at least.)
>>
> --
Vittorio
Luca Barbato Feb. 18, 2018, 5:13 p.m. | #6
On 16/02/2018 18:28, Diego Biurrun wrote:
> On Fri, Feb 16, 2018 at 06:02:05PM +0100, Luca Barbato wrote:
>> Unbreaks the rate-control behaviour.
> 
> How does this unbreak what?

Locally amended adding:

The API expects an average framerate, the timebase is often
1/1000 while the reported average framerate is 30.

Before this patch the resulting bitrate would be surprising if
the timebase provided isn't the inverse of the average framerate.

lu

Patch

diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c
index fd5452193b..73aff2caef 100644
--- a/libavcodec/libx265.c
+++ b/libavcodec/libx265.c
@@ -116,8 +116,8 @@  static av_cold int libx265_encode_init(AVCodecContext *avctx)
     }
 
     ctx->params->frameNumThreads = avctx->thread_count;
-    ctx->params->fpsNum          = avctx->time_base.den;
-    ctx->params->fpsDenom        = avctx->time_base.num * avctx->ticks_per_frame;
+    ctx->params->fpsNum          = avctx->framerate.num;
+    ctx->params->fpsDenom        = avctx->framerate.den * avctx->ticks_per_frame;
     ctx->params->sourceWidth     = avctx->width;
     ctx->params->sourceHeight    = avctx->height;
     ctx->params->bEnablePsnr     = !!(avctx->flags & AV_CODEC_FLAG_PSNR);