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

[Michael Niedermayer]

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


[...]
-- 
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.
-------------- 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/20171115/20b90f60/attachment.sig>