diff mbox

[FFmpeg-devel,2/2] avcodec/mpeg12dec: avoid adding PANSCAN side data unless present

Message ID 20171114180402.21244-2-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Karmani Nov. 14, 2017, 6:04 p.m. UTC 
From: Aman Gupta <aman@tmm1.net>

---
 libavcodec/mpeg12dec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Michael Niedermayer Nov. 15, 2017, midnight UTC | #1 
On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> ---
>  libavcodec/mpeg12dec.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 82bb1286ff..2c96dfa638 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -54,6 +54,7 @@ typedef struct Mpeg1Context {
>      int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
>      int repeat_field;           /* true if we must repeat the field */
>      AVPanScan pan_scan;         /* some temporary storage for the panscan */
> +    int has_pan_scan;
>      AVStereo3D stereo3d;
>      int has_stereo3d;
>      uint8_t *a53_caption;
> @@ -1458,6 +1459,7 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
>  
>      s1->pan_scan.width  = 16 * w;
>      s1->pan_scan.height = 16 * h;
> +    s1->has_pan_scan = 1;
>  
>      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
>          av_log(s->avctx, AV_LOG_DEBUG, "sde w:%d, h:%d\n", w, h);
> @@ -1489,6 +1491,8 @@ static void mpeg_decode_picture_display_extension(Mpeg1Context *s1)
>          skip_bits(&s->gb, 1); // marker
>      }
>  
> +    s1->has_pan_scan = 1;
> +
>      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
>          av_log(s->avctx, AV_LOG_DEBUG,
>                 "pde (%"PRId16",%"PRId16") (%"PRId16",%"PRId16") (%"PRId16",%"PRId16")\n",
> @@ -1621,12 +1625,16 @@ static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size)
>              }
>          }
>  
> -        pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
> -                                          AV_FRAME_DATA_PANSCAN,
> -                                          sizeof(s1->pan_scan));
> -        if (!pan_scan)
> -            return AVERROR(ENOMEM);
> -        memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> +        if (s1->has_pan_scan) {
> +            pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
> +                                            AV_FRAME_DATA_PANSCAN,
> +                                            sizeof(s1->pan_scan));
> +            if (!pan_scan)
> +                return AVERROR(ENOMEM);
> +
> +            memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> +            s1->has_pan_scan = 0;

the mpeg2 spec says:
"In   the   case    that    a    given    picture    does    not    have    a
 picture_display_extension() then the  most  recently  decoded  frame  centre
 offset shall be used."

"Following a sequence_header() the value zero shall be  used  for  all  frame
 centre offsets until a picture_display_extension() defines non-zero values."

I dont think its a good idea to ommit the "most  recently  decoded
frame  centre" on the subset of AVFrames which do not explicitly store
it. That would make it hard for applications which want to access this,
an application would have to keep track of the last one as well as
where sequence headers occured


[...]
Aman Karmani Nov. 15, 2017, 4:38 a.m. UTC | #2 
On Tue, Nov 14, 2017 at 4:01 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> >
> > ---
> >  libavcodec/mpeg12dec.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > index 82bb1286ff..2c96dfa638 100644
> > --- a/libavcodec/mpeg12dec.c
> > +++ b/libavcodec/mpeg12dec.c
> > @@ -54,6 +54,7 @@ typedef struct Mpeg1Context {
> >      int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
> >      int repeat_field;           /* true if we must repeat the field */
> >      AVPanScan pan_scan;         /* some temporary storage for the
> panscan */
> > +    int has_pan_scan;
> >      AVStereo3D stereo3d;
> >      int has_stereo3d;
> >      uint8_t *a53_caption;
> > @@ -1458,6 +1459,7 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context
> *s1)
> >
> >      s1->pan_scan.width  = 16 * w;
> >      s1->pan_scan.height = 16 * h;
> > +    s1->has_pan_scan = 1;
> >
> >      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
> >          av_log(s->avctx, AV_LOG_DEBUG, "sde w:%d, h:%d\n", w, h);
> > @@ -1489,6 +1491,8 @@ static void mpeg_decode_picture_display_extension(Mpeg1Context
> *s1)
> >          skip_bits(&s->gb, 1); // marker
> >      }
> >
> > +    s1->has_pan_scan = 1;
> > +
> >      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
> >          av_log(s->avctx, AV_LOG_DEBUG,
> >                 "pde (%"PRId16",%"PRId16") (%"PRId16",%"PRId16")
> (%"PRId16",%"PRId16")\n",
> > @@ -1621,12 +1625,16 @@ static int mpeg_field_start(MpegEncContext *s,
> const uint8_t *buf, int buf_size)
> >              }
> >          }
> >
> > -        pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
> > -                                          AV_FRAME_DATA_PANSCAN,
> > -                                          sizeof(s1->pan_scan));
> > -        if (!pan_scan)
> > -            return AVERROR(ENOMEM);
> > -        memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> > +        if (s1->has_pan_scan) {
> > +            pan_scan = av_frame_new_side_data(s->
> current_picture_ptr->f,
> > +                                            AV_FRAME_DATA_PANSCAN,
> > +                                            sizeof(s1->pan_scan));
> > +            if (!pan_scan)
> > +                return AVERROR(ENOMEM);
> > +
> > +            memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> > +            s1->has_pan_scan = 0;
>
> the mpeg2 spec says:
> "In   the   case    that    a    given    picture    does    not    have
>   a
>  picture_display_extension() then the  most  recently  decoded  frame
> centre
>  offset shall be used."
>
> "Following a sequence_header() the value zero shall be  used  for  all
> frame
>  centre offsets until a picture_display_extension() defines non-zero
> values."
>
> I dont think its a good idea to ommit the "most  recently  decoded
> frame  centre" on the subset of AVFrames which do not explicitly store
> it. That would make it hard for applications which want to access this,
> an application would have to keep track of the last one as well as
> where sequence headers occured


Sorry, my mistake.

Would the patch make sense if I removed the "has_pan_scan = 0" reset? Or is
it better just to always include PANSCAN side data to make it easier for
API consumers.

Aman


>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Old school: Use the lowest level language in which you can solve the
> problem
>             conveniently.
> New school: Use the highest level language in which the latest
> supercomputer
>             can solve the problem without the user falling asleep waiting.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer Nov. 16, 2017, 1:59 a.m. UTC | #3 
On Tue, Nov 14, 2017 at 08:38:51PM -0800, Aman Gupta wrote:
> On Tue, Nov 14, 2017 at 4:01 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> > > From: Aman Gupta <aman@tmm1.net>
> > >
> > > ---
> > >  libavcodec/mpeg12dec.c | 20 ++++++++++++++------
> > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > > index 82bb1286ff..2c96dfa638 100644
> > > --- a/libavcodec/mpeg12dec.c
> > > +++ b/libavcodec/mpeg12dec.c
> > > @@ -54,6 +54,7 @@ typedef struct Mpeg1Context {
> > >      int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
> > >      int repeat_field;           /* true if we must repeat the field */
> > >      AVPanScan pan_scan;         /* some temporary storage for the
> > panscan */
> > > +    int has_pan_scan;
> > >      AVStereo3D stereo3d;
> > >      int has_stereo3d;
> > >      uint8_t *a53_caption;
> > > @@ -1458,6 +1459,7 @@ static void mpeg_decode_sequence_display_extension(Mpeg1Context
> > *s1)
> > >
> > >      s1->pan_scan.width  = 16 * w;
> > >      s1->pan_scan.height = 16 * h;
> > > +    s1->has_pan_scan = 1;
> > >
> > >      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
> > >          av_log(s->avctx, AV_LOG_DEBUG, "sde w:%d, h:%d\n", w, h);
> > > @@ -1489,6 +1491,8 @@ static void mpeg_decode_picture_display_extension(Mpeg1Context
> > *s1)
> > >          skip_bits(&s->gb, 1); // marker
> > >      }
> > >
> > > +    s1->has_pan_scan = 1;
> > > +
> > >      if (s->avctx->debug & FF_DEBUG_PICT_INFO)
> > >          av_log(s->avctx, AV_LOG_DEBUG,
> > >                 "pde (%"PRId16",%"PRId16") (%"PRId16",%"PRId16")
> > (%"PRId16",%"PRId16")\n",
> > > @@ -1621,12 +1625,16 @@ static int mpeg_field_start(MpegEncContext *s,
> > const uint8_t *buf, int buf_size)
> > >              }
> > >          }
> > >
> > > -        pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
> > > -                                          AV_FRAME_DATA_PANSCAN,
> > > -                                          sizeof(s1->pan_scan));
> > > -        if (!pan_scan)
> > > -            return AVERROR(ENOMEM);
> > > -        memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> > > +        if (s1->has_pan_scan) {
> > > +            pan_scan = av_frame_new_side_data(s->
> > current_picture_ptr->f,
> > > +                                            AV_FRAME_DATA_PANSCAN,
> > > +                                            sizeof(s1->pan_scan));
> > > +            if (!pan_scan)
> > > +                return AVERROR(ENOMEM);
> > > +
> > > +            memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
> > > +            s1->has_pan_scan = 0;
> >
> > the mpeg2 spec says:
> > "In   the   case    that    a    given    picture    does    not    have
> >   a
> >  picture_display_extension() then the  most  recently  decoded  frame
> > centre
> >  offset shall be used."
> >
> > "Following a sequence_header() the value zero shall be  used  for  all
> > frame
> >  centre offsets until a picture_display_extension() defines non-zero
> > values."
> >
> > I dont think its a good idea to ommit the "most  recently  decoded
> > frame  centre" on the subset of AVFrames which do not explicitly store
> > it. That would make it hard for applications which want to access this,
> > an application would have to keep track of the last one as well as
> > where sequence headers occured
> 
> 
> Sorry, my mistake.
> 
> Would the patch make sense if I removed the "has_pan_scan = 0" reset? Or is

i think so, yes

> it better just to always include PANSCAN side data to make it easier for
> API consumers.
> 
> Aman

[...]
diff mbox

Patch

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 82bb1286ff..2c96dfa638 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -54,6 +54,7 @@  typedef struct Mpeg1Context {
     int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
     int repeat_field;           /* true if we must repeat the field */
     AVPanScan pan_scan;         /* some temporary storage for the panscan */
+    int has_pan_scan;
     AVStereo3D stereo3d;
     int has_stereo3d;
     uint8_t *a53_caption;
@@ -1458,6 +1459,7 @@  static void mpeg_decode_sequence_display_extension(Mpeg1Context *s1)
 
     s1->pan_scan.width  = 16 * w;
     s1->pan_scan.height = 16 * h;
+    s1->has_pan_scan = 1;
 
     if (s->avctx->debug & FF_DEBUG_PICT_INFO)
         av_log(s->avctx, AV_LOG_DEBUG, "sde w:%d, h:%d\n", w, h);
@@ -1489,6 +1491,8 @@  static void mpeg_decode_picture_display_extension(Mpeg1Context *s1)
         skip_bits(&s->gb, 1); // marker
     }
 
+    s1->has_pan_scan = 1;
+
     if (s->avctx->debug & FF_DEBUG_PICT_INFO)
         av_log(s->avctx, AV_LOG_DEBUG,
                "pde (%"PRId16",%"PRId16") (%"PRId16",%"PRId16") (%"PRId16",%"PRId16")\n",
@@ -1621,12 +1625,16 @@  static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size)
             }
         }
 
-        pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
-                                          AV_FRAME_DATA_PANSCAN,
-                                          sizeof(s1->pan_scan));
-        if (!pan_scan)
-            return AVERROR(ENOMEM);
-        memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
+        if (s1->has_pan_scan) {
+            pan_scan = av_frame_new_side_data(s->current_picture_ptr->f,
+                                            AV_FRAME_DATA_PANSCAN,
+                                            sizeof(s1->pan_scan));
+            if (!pan_scan)
+                return AVERROR(ENOMEM);
+
+            memcpy(pan_scan->data, &s1->pan_scan, sizeof(s1->pan_scan));
+            s1->has_pan_scan = 0;
+        }
 
         if (s1->a53_caption) {
             AVFrameSideData *sd = av_frame_new_side_data(