Re: [FFmpeg-devel] [PATCH 2/2] avcodec/mpeg12dec: avoid adding PANSCAN side data unless present
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
On Tue, Nov 14, 2017 at 4:01 PM Michael Niedermayerwrote: > 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
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
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