[FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order

[Tom Yan]

It can probably be simplified to this:

...
if (progressive_frame)
    s->field_order = AV_FIELD_PROGRESSIVE;
/* top_field_first is mandated to be 0 when
   picture_structure is not 3 (i.e. not a frame picture) */
else if (top_field_first || picture_structure == 1)
    s->field_order = AV_FIELD_TT;
else
    s->field_order = AV_FIELD_BB;
...

since progressive_sequence == 1 is illegal anyway if not all the
pictures/frames in the sequence are marked with progressive_frame ==
1.

If we consider commit 88e2dc7 proper, i.e. we should set
s->field_order to AV_FIELD_PROGRESSIVE when (processive_sequence == 0
and progressive_frame == 1), there's not really a point to check
processive_sequence at all.

On Sat, 5 Feb 2022 at 20:51, Tom Yan <tom.ty89 at gmail.com> wrote:
>
> the top_field_first bit is only used to indicate the field order
> when the picture is a frame picture (which consists of two fields)
> but not when it is a field picture (which consists of one single
> top or bottom field).
>
> Signed-off-by: Tom Yan <tom.ty89 at gmail.com>
> ---
>  libavcodec/mpegvideo_parser.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> index c5dc867d24..2ddcdb0f37 100644
> --- a/libavcodec/mpegvideo_parser.c
> +++ b/libavcodec/mpegvideo_parser.c
> @@ -108,7 +108,7 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>      uint32_t start_code;
>      int frame_rate_index, ext_type, bytes_left;
>      int frame_rate_ext_n, frame_rate_ext_d;
> -    int top_field_first, repeat_first_field, progressive_frame;
> +    int picture_structure, top_field_first, repeat_first_field, progressive_frame;
>      int horiz_size_ext, vert_size_ext, bit_rate_ext;
>      int did_set_size=0;
>      int set_dim_ret = 0;
> @@ -181,6 +181,7 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>                      break;
>                  case 0x8: /* picture coding extension */
>                      if (bytes_left >= 5) {
> +                        picture_structure = buf[2] & 0x3;
>                          top_field_first = buf[3] & (1 << 7);
>                          repeat_first_field = buf[3] & (1 << 1);
>                          progressive_frame = buf[4] & (1 << 7);
> @@ -199,7 +200,9 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>                          }
>
>                          if (!pc->progressive_sequence && !progressive_frame) {
> -                            if (top_field_first)
> +                            /* top_field_first is mandated to be 0 when
> +                               picture_structure is not 3 (i.e. not a frame picture) */
> +                            if (top_field_first || picture_structure == 1)
>                                  s->field_order = AV_FIELD_TT;
>                              else
>                                  s->field_order = AV_FIELD_BB;
> --
> 2.35.1
>