[FFmpeg-devel] libavcodec/mpegvideo_parser: set field_order correctly for progressive mpeg2

Submitted by Aman Gupta on Dec. 6, 2017, 10:54 p.m.

Details

Message ID 20171206225450.92658-1-ffmpeg@tmm1.net
State Accepted
Commit 88e2dc7d0448d1d4656c78454bc5f17063b867e7
Headers show

Commit Message

Aman Gupta Dec. 6, 2017, 10:54 p.m.
From: Aman Gupta <aman@tmm1.net>

---
 libavcodec/mpegvideo_parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Dec. 7, 2017, 7:42 p.m.
On Wed, Dec 06, 2017 at 02:54:50PM -0800, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> ---
>  libavcodec/mpegvideo_parser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
> index de70cd5632..be240b6890 100644
> --- a/libavcodec/mpegvideo_parser.c
> +++ b/libavcodec/mpegvideo_parser.c
> @@ -131,7 +131,7 @@ static void mpegvideo_extract_headers(AVCodecParserContext *s,
>                              }
>                          }
>  
> -                        if (!pc->progressive_sequence) {
> +                        if (!pc->progressive_sequence && !progressive_frame) {

Iam not against this if it results in more correct interlaced detection
in practice
But the spec uses progressive_sequence alone to determine the output

    ISO/IEC 13818-2: 1995 (E) / Recommendation ITU-T H.262 (1995 E)

    If progressive_sequence is 0 the period between two successive fields at the output of the decoding
    process is half of the reciprocal of the frame_rate. See Figure 7-20.

So the commit message seems wrong or inprecisse 

[...]
Aman Gupta Dec. 7, 2017, 7:47 p.m.
On Thu, Dec 7, 2017 at 11:42 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Dec 06, 2017 at 02:54:50PM -0800, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > ---
> >  libavcodec/mpegvideo_parser.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mpegvideo_parser.c
> b/libavcodec/mpegvideo_parser.c
> > index de70cd5632..be240b6890 100644
> > --- a/libavcodec/mpegvideo_parser.c
> > +++ b/libavcodec/mpegvideo_parser.c
> > @@ -131,7 +131,7 @@ static void
> mpegvideo_extract_headers(AVCodecParserContext *s,
> >                              }
> >                          }
> >
> > -                        if (!pc->progressive_sequence) {
> > +                        if (!pc->progressive_sequence &&
> !progressive_frame) {
>
> Iam not against this if it results in more correct interlaced detection
> in practice


In my experience, most progressive mpeg2 samples are reported as interlaced
by ffprobe before this change. See for example
https://tmm1.s3.amazonaws.com/720p.ts


> But the spec uses progressive_sequence alone to determine the output
>
>     ISO/IEC 13818-2: 1995 (E) / Recommendation ITU-T H.262 (1995 E)
>
>     If progressive_sequence is 0 the period between two successive fields
> at the output of the decoding
>     process is half of the reciprocal of the frame_rate. See Figure 7-20.
>
> So the commit message seems wrong or inprecisse
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Dec. 7, 2017, 10:42 p.m.
On Thu, Dec 07, 2017 at 07:47:58PM +0000, Aman Gupta wrote:
> On Thu, Dec 7, 2017 at 11:42 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Dec 06, 2017 at 02:54:50PM -0800, Aman Gupta wrote:
> > > From: Aman Gupta <aman@tmm1.net>
> > >
> > > ---
> > >  libavcodec/mpegvideo_parser.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/mpegvideo_parser.c
> > b/libavcodec/mpegvideo_parser.c
> > > index de70cd5632..be240b6890 100644
> > > --- a/libavcodec/mpegvideo_parser.c
> > > +++ b/libavcodec/mpegvideo_parser.c
> > > @@ -131,7 +131,7 @@ static void
> > mpegvideo_extract_headers(AVCodecParserContext *s,
> > >                              }
> > >                          }
> > >
> > > -                        if (!pc->progressive_sequence) {
> > > +                        if (!pc->progressive_sequence &&
> > !progressive_frame) {
> >
> > Iam not against this if it results in more correct interlaced detection
> > in practice
> 
> 
> In my experience, most progressive mpeg2 samples are reported as interlaced
> by ffprobe before this change. See for example
> https://tmm1.s3.amazonaws.com/720p.ts

ok, can you make the commit message convey this more clearly?
as its now it looks like this is a fix for some spec non compliance


[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpegvideo_parser.c b/libavcodec/mpegvideo_parser.c
index de70cd5632..be240b6890 100644
--- a/libavcodec/mpegvideo_parser.c
+++ b/libavcodec/mpegvideo_parser.c
@@ -131,7 +131,7 @@  static void mpegvideo_extract_headers(AVCodecParserContext *s,
                             }
                         }
 
-                        if (!pc->progressive_sequence) {
+                        if (!pc->progressive_sequence && !progressive_frame) {
                             if (top_field_first)
                                 s->field_order = AV_FIELD_TT;
                             else