| Message ID | 20220206054127.36820-1-tom.ty89@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] mpegvideo_parser: check picture_structure for field order | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
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; > }
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".
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; }
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(-)