Message ID | 20171114180402.21244-2-ffmpeg@tmm1.net |
---|---|
State | New |
Headers | show |
On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote: > From: Aman Gupta <aman@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 [...]
On Tue, Nov 14, 2017 at 4:01 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote: > > From: Aman Gupta <aman@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
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@niedermayer.cc> > wrote: > > > On Tue, Nov 14, 2017 at 10:04:02AM -0800, Aman Gupta wrote: > > > From: Aman Gupta <aman@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 [...]
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; + } if (s1->a53_caption) { AVFrameSideData *sd = av_frame_new_side_data(
From: Aman Gupta <aman@tmm1.net> --- libavcodec/mpeg12dec.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)