diff mbox series

[FFmpeg-devel] ffmpeg: fix order between field order autodetection and override

Message ID 20210721172916.40739-1-jeebjp@gmail.com
State Accepted
Commit 4c694093be68d401c60819e5171817c62afef8b2
Headers show
Series [FFmpeg-devel] ffmpeg: fix order between field order autodetection and override
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Jan Ekström July 21, 2021, 5:29 p.m. UTC
Somehow I missed this in fbb44bc51a647862eb05ae3f9d7d49a0be9bed57 ,
even though the lines were within the context. Probably the code
originally being after the this logic had something to do with it,
but previously it only touched the avformat context's codecpar,
which did not affect the encoder codec context whatsoever.

Fixes #9320
Fixes #9339
---
 fftools/ffmpeg.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Jan Ekström July 24, 2021, 6:47 p.m. UTC | #1
On Wed, Jul 21, 2021 at 8:29 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Somehow I missed this in fbb44bc51a647862eb05ae3f9d7d49a0be9bed57 ,
> even though the lines were within the context. Probably the code
> originally being after the this logic had something to do with it,
> but previously it only touched the avformat context's codecpar,
> which did not affect the encoder codec context whatsoever.
>
> Fixes #9320
> Fixes #9339
> ---
>  fftools/ffmpeg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 40e8010096..b0ce7c7c32 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3483,12 +3483,7 @@ static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
>              enc_ctx->bits_per_raw_sample = frame_bits_per_raw_sample;
>          }
>
> -        if (ost->top_field_first == 0) {
> -            enc_ctx->field_order = AV_FIELD_BB;
> -        } else if (ost->top_field_first == 1) {
> -            enc_ctx->field_order = AV_FIELD_TT;
> -        }
> -
> +        // Field order: autodetection
>          if (frame) {
>              if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT | AV_CODEC_FLAG_INTERLACED_ME) &&
>                  ost->top_field_first >= 0)
> @@ -3503,6 +3498,13 @@ static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
>                  enc_ctx->field_order = AV_FIELD_PROGRESSIVE;
>          }
>
> +        // Field order: override
> +        if (ost->top_field_first == 0) {
> +            enc_ctx->field_order = AV_FIELD_BB;
> +        } else if (ost->top_field_first == 1) {
> +            enc_ctx->field_order = AV_FIELD_TT;
> +        }
> +
>          if (ost->forced_keyframes) {
>              if (!strncmp(ost->forced_keyframes, "expr:", 5)) {
>                  ret = av_expr_parse(&ost->forced_keyframes_pexpr, ost->forced_keyframes+5,
> --
> 2.31.1
>

Ping.
Paul B Mahol July 25, 2021, 7:14 p.m. UTC | #2
On Wed, Jul 21, 2021 at 7:29 PM Jan Ekström <jeebjp@gmail.com> wrote:

> Somehow I missed this in fbb44bc51a647862eb05ae3f9d7d49a0be9bed57 ,
> even though the lines were within the context. Probably the code
> originally being after the this logic had something to do with it,
> but previously it only touched the avformat context's codecpar,
> which did not affect the encoder codec context whatsoever.
>
> Fixes #9320
> Fixes #9339
> ---
>  fftools/ffmpeg.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 40e8010096..b0ce7c7c32 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3483,12 +3483,7 @@ static int init_output_stream_encode(OutputStream
> *ost, AVFrame *frame)
>              enc_ctx->bits_per_raw_sample = frame_bits_per_raw_sample;
>          }
>
> -        if (ost->top_field_first == 0) {
> -            enc_ctx->field_order = AV_FIELD_BB;
> -        } else if (ost->top_field_first == 1) {
> -            enc_ctx->field_order = AV_FIELD_TT;
> -        }
> -
> +        // Field order: autodetection
>          if (frame) {
>              if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT |
> AV_CODEC_FLAG_INTERLACED_ME) &&
>                  ost->top_field_first >= 0)
> @@ -3503,6 +3498,13 @@ static int init_output_stream_encode(OutputStream
> *ost, AVFrame *frame)
>                  enc_ctx->field_order = AV_FIELD_PROGRESSIVE;
>          }
>
> +        // Field order: override
> +        if (ost->top_field_first == 0) {
> +            enc_ctx->field_order = AV_FIELD_BB;
> +        } else if (ost->top_field_first == 1) {
> +            enc_ctx->field_order = AV_FIELD_TT;
> +        }
> +
>          if (ost->forced_keyframes) {
>              if (!strncmp(ost->forced_keyframes, "expr:", 5)) {
>                  ret = av_expr_parse(&ost->forced_keyframes_pexpr,
> ost->forced_keyframes+5,
> --
> 2.31.1
>
>
LGTM


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Jan Ekström July 25, 2021, 7:42 p.m. UTC | #3
On Sun, Jul 25, 2021 at 10:38 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On Wed, Jul 21, 2021 at 7:29 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > Somehow I missed this in fbb44bc51a647862eb05ae3f9d7d49a0be9bed57 ,
> > even though the lines were within the context. Probably the code
> > originally being after the this logic had something to do with it,
> > but previously it only touched the avformat context's codecpar,
> > which did not affect the encoder codec context whatsoever.
> >
> > Fixes #9320
> > Fixes #9339
> > ---
> >  fftools/ffmpeg.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 40e8010096..b0ce7c7c32 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -3483,12 +3483,7 @@ static int init_output_stream_encode(OutputStream
> > *ost, AVFrame *frame)
> >              enc_ctx->bits_per_raw_sample = frame_bits_per_raw_sample;
> >          }
> >
> > -        if (ost->top_field_first == 0) {
> > -            enc_ctx->field_order = AV_FIELD_BB;
> > -        } else if (ost->top_field_first == 1) {
> > -            enc_ctx->field_order = AV_FIELD_TT;
> > -        }
> > -
> > +        // Field order: autodetection
> >          if (frame) {
> >              if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT |
> > AV_CODEC_FLAG_INTERLACED_ME) &&
> >                  ost->top_field_first >= 0)
> > @@ -3503,6 +3498,13 @@ static int init_output_stream_encode(OutputStream
> > *ost, AVFrame *frame)
> >                  enc_ctx->field_order = AV_FIELD_PROGRESSIVE;
> >          }
> >
> > +        // Field order: override
> > +        if (ost->top_field_first == 0) {
> > +            enc_ctx->field_order = AV_FIELD_BB;
> > +        } else if (ost->top_field_first == 1) {
> > +            enc_ctx->field_order = AV_FIELD_TT;
> > +        }
> > +
> >          if (ost->forced_keyframes) {
> >              if (!strncmp(ost->forced_keyframes, "expr:", 5)) {
> >                  ret = av_expr_parse(&ost->forced_keyframes_pexpr,
> > ost->forced_keyframes+5,
> > --
> > 2.31.1
> >
> >
> LGTM
>

Thanks, applied with the recommendations you made for the commit
message, as well as fixing a case of "the this logic".

Jan
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 40e8010096..b0ce7c7c32 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -3483,12 +3483,7 @@  static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
             enc_ctx->bits_per_raw_sample = frame_bits_per_raw_sample;
         }
 
-        if (ost->top_field_first == 0) {
-            enc_ctx->field_order = AV_FIELD_BB;
-        } else if (ost->top_field_first == 1) {
-            enc_ctx->field_order = AV_FIELD_TT;
-        }
-
+        // Field order: autodetection
         if (frame) {
             if (enc_ctx->flags & (AV_CODEC_FLAG_INTERLACED_DCT | AV_CODEC_FLAG_INTERLACED_ME) &&
                 ost->top_field_first >= 0)
@@ -3503,6 +3498,13 @@  static int init_output_stream_encode(OutputStream *ost, AVFrame *frame)
                 enc_ctx->field_order = AV_FIELD_PROGRESSIVE;
         }
 
+        // Field order: override
+        if (ost->top_field_first == 0) {
+            enc_ctx->field_order = AV_FIELD_BB;
+        } else if (ost->top_field_first == 1) {
+            enc_ctx->field_order = AV_FIELD_TT;
+        }
+
         if (ost->forced_keyframes) {
             if (!strncmp(ost->forced_keyframes, "expr:", 5)) {
                 ret = av_expr_parse(&ost->forced_keyframes_pexpr, ost->forced_keyframes+5,