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

[Tom Yan]

This is really just a matter of choice. If you don't care about
spec-conformance in this particular case, top_field_first *could* be 1
and/or picture_structure *could* be not 3 even when
progressive_sequence is 1, so which one do you choose to believe? The
truth is, it doesn't even matter.

If you insist on making it more clumsy and silly, I can resend.

On Sun, 6 Feb 2022 at 13:46, Andreas Rheinhardt
 wrote:
>
> Tom Yan:
> > 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).
> >
> > also removing the unnecessary progressive_sequence check (the bit
> > is mandated to be 0 if progressive_frame is 0 on any picture in the
> > sequence).
> >
>
> Just because something is mandated does not mean that it is so;
> spec-incompliant files exist.
>
> > Signed-off-by: Tom Yan 
> > ---
> >  libavcodec/mpegvideo_parser.c | 15 +--
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> > index c5dc867d24..004ff602f6 100644
> > --- a/libavcodec/mpegvideo_parser.c
> > +++ b/libavcodec/mpegvideo_parser.c
> > @@ -181,6 +181,7 @@ static void 
> > mpegvideo_extract_headers(AVCodecParserContext *s,
> >  break;
> >  case 0x8: /* picture coding extension */
> >  if (bytes_left >= 5) {
> > +s->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);
> > @@ -198,13 +199,15 @@ static void 
> > mpegvideo_extract_headers(AVCodecParserContext *s,
> >  }
> >  }
> >
> > -if (!pc->progressive_sequence && 
> > !progressive_frame) {
> > -if (top_field_first)
> > -s->field_order = AV_FIELD_TT;
> > -else
> > -s->field_order = AV_FIELD_BB;
> > -} else
> > +if (progressive_frame)
> >  s->field_order = AV_FIELD_PROGRESSIVE;
> > +else if (top_field_first ||
> > + /* top_field_first is mandated to be 0 
> > when
> > +the picture is not a frame picture) */
> > + s->picture_structure == 
> > AV_PICTURE_STRUCTURE_TOP_FIELD)
> > +s->field_order = AV_FIELD_TT;
> > +else
> > +s->field_order = AV_FIELD_BB;
> >  }
> >  break;
> >  }
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

[Andreas Rheinhardt]

Tom Yan:
> 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).
> 
> also removing the unnecessary progressive_sequence check (the bit
> is mandated to be 0 if progressive_frame is 0 on any picture in the
> sequence).
> 

Just because something is mandated does not mean that it is so;
spec-incompliant files exist.

> Signed-off-by: Tom Yan 
> ---
>  libavcodec/mpegvideo_parser.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> index c5dc867d24..004ff602f6 100644
> --- a/libavcodec/mpegvideo_parser.c
> +++ b/libavcodec/mpegvideo_parser.c
> @@ -181,6 +181,7 @@ static void 
> mpegvideo_extract_headers(AVCodecParserContext *s,
>  break;
>  case 0x8: /* picture coding extension */
>  if (bytes_left >= 5) {
> +s->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);
> @@ -198,13 +199,15 @@ static void 
> mpegvideo_extract_headers(AVCodecParserContext *s,
>  }
>  }
>  
> -if (!pc->progressive_sequence && !progressive_frame) 
> {
> -if (top_field_first)
> -s->field_order = AV_FIELD_TT;
> -else
> -s->field_order = AV_FIELD_BB;
> -} else
> +if (progressive_frame)
>  s->field_order = AV_FIELD_PROGRESSIVE;
> +else if (top_field_first ||
> + /* top_field_first is mandated to be 0 when
> +the picture is not a frame picture) */
> + s->picture_structure == 
> AV_PICTURE_STRUCTURE_TOP_FIELD)
> +s->field_order = AV_FIELD_TT;
> +else
> +s->field_order = AV_FIELD_BB;
>  }
>  break;
>  }

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

[Tom Yan]

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).

also removing the unnecessary progressive_sequence check (the bit
is mandated to be 0 if progressive_frame is 0 on any picture in the
sequence).

Signed-off-by: Tom Yan 
---
 libavcodec/mpegvideo_parser.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
index c5dc867d24..004ff602f6 100644
--- a/libavcodec/mpegvideo_parser.c
+++ b/libavcodec/mpegvideo_parser.c
@@ -181,6 +181,7 @@ static void mpegvideo_extract_headers(AVCodecParserContext 
*s,
 break;
 case 0x8: /* picture coding extension */
 if (bytes_left >= 5) {
+s->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);
@@ -198,13 +199,15 @@ static void 
mpegvideo_extract_headers(AVCodecParserContext *s,
 }
 }
 
-if (!pc->progressive_sequence && !progressive_frame) {
-if (top_field_first)
-s->field_order = AV_FIELD_TT;
-else
-s->field_order = AV_FIELD_BB;
-} else
+if (progressive_frame)
 s->field_order = AV_FIELD_PROGRESSIVE;
+else if (top_field_first ||
+ /* top_field_first is mandated to be 0 when
+the picture is not a frame picture) */
+ s->picture_structure == 
AV_PICTURE_STRUCTURE_TOP_FIELD)
+s->field_order = AV_FIELD_TT;
+else
+s->field_order = AV_FIELD_BB;
 }
 break;
 }
-- 
2.35.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".