* [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order
@ 2022-02-05 12:51 Tom Yan
2022-02-05 13:12 ` Tom Yan
2022-02-05 13:31 ` James Almer
0 siblings, 2 replies; 6+ messages in thread
From: Tom Yan @ 2022-02-05 12:51 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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).
Signed-off-by: Tom Yan <tom.ty89@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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order
2022-02-05 12:51 [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order Tom Yan
@ 2022-02-05 13:12 ` Tom Yan
2022-02-05 13:31 ` James Almer
1 sibling, 0 replies; 6+ messages in thread
From: Tom Yan @ 2022-02-05 13:12 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: aman
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@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@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
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order
2022-02-05 12:51 [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order Tom Yan
2022-02-05 13:12 ` Tom Yan
@ 2022-02-05 13:31 ` James Almer
2022-02-06 5:41 ` [FFmpeg-devel] [PATCH v2] " Tom Yan
1 sibling, 1 reply; 6+ messages in thread
From: James Almer @ 2022-02-05 13:31 UTC (permalink / raw)
To: ffmpeg-devel
On 2/5/2022 9:51 AM, Tom Yan 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@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;
You could maybe also set s->picture_structure with this.
> 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;
_______________________________________________
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".
^ permalink raw reply [flat|nested] 6+ messages in thread
* [FFmpeg-devel] [PATCH v2] mpegvideo_parser: check picture_structure for field order
2022-02-05 13:31 ` James Almer
@ 2022-02-06 5:41 ` Tom Yan
2022-02-06 5:45 ` Andreas Rheinhardt
0 siblings, 1 reply; 6+ messages in thread
From: Tom Yan @ 2022-02-06 5:41 UTC (permalink / raw)
To: ffmpeg-devel, jamrial; +Cc: aman, 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 <tom.ty89@gmail.com>
---
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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] mpegvideo_parser: check picture_structure for field order
2022-02-06 5:41 ` [FFmpeg-devel] [PATCH v2] " Tom Yan
@ 2022-02-06 5:45 ` Andreas Rheinhardt
2022-02-06 8:59 ` Tom Yan
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Rheinhardt @ 2022-02-06 5:45 UTC (permalink / raw)
To: ffmpeg-devel
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 <tom.ty89@gmail.com>
> ---
> 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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] mpegvideo_parser: check picture_structure for field order
2022-02-06 5:45 ` Andreas Rheinhardt
@ 2022-02-06 8:59 ` Tom Yan
0 siblings, 0 replies; 6+ messages in thread
From: Tom Yan @ 2022-02-06 8:59 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
<andreas.rheinhardt@outlook.com> 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 <tom.ty89@gmail.com>
> > ---
> > 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-request@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-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-06 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-05 12:51 [FFmpeg-devel] [PATCH] mpegvideo_parser: check picture_structure for field order Tom Yan
2022-02-05 13:12 ` Tom Yan
2022-02-05 13:31 ` James Almer
2022-02-06 5:41 ` [FFmpeg-devel] [PATCH v2] " Tom Yan
2022-02-06 5:45 ` Andreas Rheinhardt
2022-02-06 8:59 ` Tom Yan
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git