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

[Aman Gupta]

On Tue, Nov 14, 2017 at 4:01 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> > From: Aman Gupta <aman at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>