Re: [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 
> wrote:
> 
> > On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> > > From: Aman Gupta 
> > >
> > > ---
> > >  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(>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, >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, >pan_scan, sizeof(s1->pan_scan));
> > > +s1->has_pan_scan = 0;
> >
> > the mpeg2 spec says:
> > "In   the   casethatagivenpicturedoesnothave
> >   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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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 
wrote:

> On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote:
> > From: Aman Gupta 
> >
> > ---
> >  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(>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, >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, >pan_scan, sizeof(s1->pan_scan));
> > +s1->has_pan_scan = 0;
>
> the mpeg2 spec says:
> "In   the   casethatagivenpicturedoesnothave
>   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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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 
> 
> ---
>  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(>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, >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, >pan_scan, sizeof(s1->pan_scan));
> +s1->has_pan_scan = 0;

the mpeg2 spec says:
"In   the   casethatagivenpicturedoesnothavea
 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.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Aman Gupta]

From: Aman Gupta 

---
 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(>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, >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, >pan_scan, sizeof(s1->pan_scan));
+s1->has_pan_scan = 0;
+}
 
 if (s1->a53_caption) {
 AVFrameSideData *sd = av_frame_new_side_data(
-- 
2.14.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel