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

[Michael Niedermayer]

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 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

i think so, yes

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171116/93984a86/attachment.sig>