[1/7] avcodec: Always fill the encoder target framerate

Message ID 20180216170209.52876-2-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.
In preparation of using it in the encoders.
---
 libavcodec/avcodec.h | 3 ++-
 libavcodec/utils.c   | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Vittorio Giovara Feb. 17, 2018, 3:52 a.m. | #1
On Fri, Feb 16, 2018 at 12:02 PM, Luca Barbato <lu_zero@gentoo.org> wrote:

> In preparation of using it in the encoders.
> ---
>  libavcodec/avcodec.h | 3 ++-
>  libavcodec/utils.c   | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7eaa0c9277..f86ae0b8d6 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2638,7 +2638,8 @@ typedef struct AVCodecContext {
>       *             bitstream, the decoder may export it here. { 0, 1} when
>       *             unknown.
>       * - encoding: May be used to signal the framerate of CFR content to
> an
> -     *             encoder.
> +     *             encoder or the target average framerate for
> rate-control
> +     *             tuning.
>       */
>      AVRational framerate;
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index ba3457664a..887c5618e2 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -539,6 +539,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              goto free_and_end;
>          }
>
> +        if (avctx->framerate.num <= 0) {
> +            avctx->framerate.num = avctx->time_base.den;
> +            avctx->framerate.den = avctx->time_base.num;
> +        }
>

Would be nice to have a comment that this is a fallback for when framerate
is not set.
Probably it would make sense to check for time_base.num too, in order to
avoid 0 as denominator.

+
>          if (avctx->codec->sample_fmts) {
>              for (i = 0; avctx->codec->sample_fmts[i] !=
> AV_SAMPLE_FMT_NONE; i++) {
>                  if (avctx->sample_fmt == avctx->codec->sample_fmts[i])
> --
>
Luca Barbato Feb. 17, 2018, 2:02 p.m. | #2
On 17/02/2018 04:52, Vittorio Giovara wrote:
> Would be nice to have a comment that this is a fallback for when framerate
> is not set.
> Probably it would make sense to check for time_base.num too, in order to
> avoid 0 as denominator.

time_base is checked just above. Do you have a wording in mind for the 
comment?
Mark Thompson Feb. 21, 2018, 9:20 p.m. | #3
On 16/02/18 17:02, Luca Barbato wrote:
> In preparation of using it in the encoders.
> ---
>  libavcodec/avcodec.h | 3 ++-
>  libavcodec/utils.c   | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 7eaa0c9277..f86ae0b8d6 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2638,7 +2638,8 @@ typedef struct AVCodecContext {
>       *             bitstream, the decoder may export it here. { 0, 1} when
>       *             unknown.
>       * - encoding: May be used to signal the framerate of CFR content to an
> -     *             encoder.
> +     *             encoder or the target average framerate for rate-control
> +     *             tuning.
>       */
>      AVRational framerate;
>  
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index ba3457664a..887c5618e2 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -539,6 +539,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              goto free_and_end;
>          }
>  
> +        if (avctx->framerate.num <= 0) {
> +            avctx->framerate.num = avctx->time_base.den;
> +            avctx->framerate.den = avctx->time_base.num;
> +        }
> +
>          if (avctx->codec->sample_fmts) {
>              for (i = 0; avctx->codec->sample_fmts[i] != AV_SAMPLE_FMT_NONE; i++) {
>                  if (avctx->sample_fmt == avctx->codec->sample_fmts[i])
> 

How should an encoder detect that its input is actually variable framerate after this change and the following one?  Currently it can check for !(framerate.{num,den} > 0), but after this change it can't distinguish that from the given constrant framerate until it gets some frames (possibly arbitrarily many).

Perhaps there should be a flag somewhere (AVCodecContext.flags2, I guess) which would distinguish between the two cases?

- Mark
Luca Barbato Feb. 21, 2018, 10:53 p.m. | #4
On 21/02/2018 22:20, Mark Thompson wrote:
> Perhaps there should be a flag somewhere (AVCodecContext.flags2, I
> guess) which would distinguish between the two cases?

I can add a flag for it indeed. Which encoders would be able to use it ?

lu

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 7eaa0c9277..f86ae0b8d6 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2638,7 +2638,8 @@  typedef struct AVCodecContext {
      *             bitstream, the decoder may export it here. { 0, 1} when
      *             unknown.
      * - encoding: May be used to signal the framerate of CFR content to an
-     *             encoder.
+     *             encoder or the target average framerate for rate-control
+     *             tuning.
      */
     AVRational framerate;
 
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index ba3457664a..887c5618e2 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -539,6 +539,11 @@  FF_ENABLE_DEPRECATION_WARNINGS
             goto free_and_end;
         }
 
+        if (avctx->framerate.num <= 0) {
+            avctx->framerate.num = avctx->time_base.den;
+            avctx->framerate.den = avctx->time_base.num;
+        }
+
         if (avctx->codec->sample_fmts) {
             for (i = 0; avctx->codec->sample_fmts[i] != AV_SAMPLE_FMT_NONE; i++) {
                 if (avctx->sample_fmt == avctx->codec->sample_fmts[i])