Re: [FFmpeg-devel] [PATCH 2/5] avcodec: use the new AVFrame interlace flags in all decoders and encoders

[Anton Khirnov <anton@khirnov.net>]

Quoting James Almer (2023-04-12 21:49:33)
> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
> index 76e70aa648..a407154959 100644
> --- a/libavcodec/cuviddec.c
> +++ b/libavcodec/cuviddec.c
> @@ -631,10 +631,10 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
> -        frame->interlaced_frame = !parsed_frame.is_deinterlacing && !parsed_frame.dispinfo.progressive_frame;
> +        frame->flags |= AV_FRAME_FLAG_INTERLACED * (!parsed_frame.is_deinterlacing && !parsed_frame.dispinfo.progressive_frame);

I think the code would look better like this:
    if (!parsed_frame.is_deinterlacing && !parsed_frame.dispinfo.progressive_frame)) {
        frame->flags |= AV_FRAME_FLAG_INTERLACED |
                        (AV_FRAME_FLAG_TOP_FIELD_FIRST * !!parsed_frame.dispinfo.top_field_first);
    }

> diff --git a/libavcodec/dnxhdenc.c b/libavcodec/dnxhdenc.c
> index 176bf972d8..a41a820ed0 100644
> --- a/libavcodec/dnxhdenc.c
> +++ b/libavcodec/dnxhdenc.c
> @@ -1251,7 +1251,7 @@ static void dnxhd_load_picture(DNXHDEncContext *ctx, const AVFrame *frame)
>          ctx->thread[i]->dct_uv_offset = ctx->m.uvlinesize*8;
>      }
>  
> -    ctx->cur_field = frame->interlaced_frame && !frame->top_field_first;
> +    ctx->cur_field = (frame->flags &AV_FRAME_FLAG_INTERLACED) && !(frame->flags & AV_FRAME_FLAG_TOP_FIELD_FIRST);

Missing space.
Also, long line.

> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 7767e16cf1..4c69893af8 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -1150,7 +1150,7 @@ static int h264_export_frame_props(H264Context *h)
>      AVFrame *out = cur->f;
>      int ret;
>  
> -    out->interlaced_frame = 0;
> +    out->flags &= ~AV_FRAME_FLAG_INTERLACED;
>      out->repeat_pict      = 0;
>  
>      /* Signal interlacing information externally. */
> @@ -1174,15 +1174,15 @@ static int h264_export_frame_props(H264Context *h)
>              break;
>          case H264_SEI_PIC_STRUCT_TOP_FIELD:
>          case H264_SEI_PIC_STRUCT_BOTTOM_FIELD:
> -            out->interlaced_frame = 1;
> +            out->flags |= AV_FRAME_FLAG_INTERLACED;
>              break;
>          case H264_SEI_PIC_STRUCT_TOP_BOTTOM:
>          case H264_SEI_PIC_STRUCT_BOTTOM_TOP:
>              if (FIELD_OR_MBAFF_PICTURE(h))
> -                out->interlaced_frame = 1;
> +                out->flags |= AV_FRAME_FLAG_INTERLACED;
>              else
>                  // try to flag soft telecine progressive
> -                out->interlaced_frame = h->prev_interlaced_frame;
> +                out->flags |= AV_FRAME_FLAG_INTERLACED * !!h->prev_interlaced_frame;
>              break;
>          case H264_SEI_PIC_STRUCT_TOP_BOTTOM_TOP:
>          case H264_SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM:
> @@ -1201,32 +1201,32 @@ static int h264_export_frame_props(H264Context *h)
>  
>          if ((pt->ct_type & 3) &&
>              pt->pic_struct <= H264_SEI_PIC_STRUCT_BOTTOM_TOP)
> -            out->interlaced_frame = (pt->ct_type & (1 << 1)) != 0;
> +            out->flags |= AV_FRAME_FLAG_INTERLACED * ((pt->ct_type & (1 << 1)) != 0);
>      } else {
>          /* Derive interlacing flag from used decoding process. */
> -        out->interlaced_frame = FIELD_OR_MBAFF_PICTURE(h);
> +        out->flags |= AV_FRAME_FLAG_INTERLACED * !!FIELD_OR_MBAFF_PICTURE(h);
>      }
> -    h->prev_interlaced_frame = out->interlaced_frame;
> +    h->prev_interlaced_frame = !!(out->flags & AV_FRAME_FLAG_INTERLACED);

Might be more readable to set a local 'interlaced' variable in all those
checks above, which then gets synced to the frame flags here.

Same below.

> diff --git a/libavcodec/hevc_refs.c b/libavcodec/hevc_refs.c
> index 96153a2459..c7a53088bc 100644
> --- a/libavcodec/hevc_refs.c
> +++ b/libavcodec/hevc_refs.c
> @@ -111,8 +111,9 @@ static HEVCFrame *alloc_frame(HEVCContext *s)
>          for (j = 0; j < frame->ctb_count; j++)
>              frame->rpl_tab[j] = (RefPicListTab *)frame->rpl_buf->data;
>  
> -        frame->frame->top_field_first  = s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_TOP_FIELD;
> -        frame->frame->interlaced_frame = (s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_TOP_FIELD) || (s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_BOTTOM_FIELD);
> +        frame->frame->flags |= AV_FRAME_FLAG_TOP_FIELD_FIRST * (s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_TOP_FIELD);
> +        frame->frame->flags |= AV_FRAME_FLAG_INTERLACED * ((s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_TOP_FIELD) ||
> +                                                           (s->sei.picture_timing.picture_struct == AV_PICTURE_STRUCTURE_BOTTOM_FIELD));

When the conditions are this long, seems better to make it into a if().

-- 
Anton Khirnov
_______________________________________________
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".