| Message ID | 20171019165105.29916-1-tfoucu@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Oct 19, 2017 at 09:51:05AM -0700, Thierry Foucu wrote: > Instead of returning nothing when we detect the xvid skipped frame, we > could return the last decoded frame, if we do have one. FRAME_SKIPPED is not limited to xvid (packed frames). There are a few other pathes that return it. For standard mpeg4video, outputing the next frame smells like violating the spec. [...]
On Thu, Oct 19, 2017 at 3:43 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Thu, Oct 19, 2017 at 09:51:05AM -0700, Thierry Foucu wrote: > > Instead of returning nothing when we detect the xvid skipped frame, we > > could return the last decoded frame, if we do have one. > > FRAME_SKIPPED is not limited to xvid (packed frames). There are a few > other pathes that return it. > > What if i change the test to check for mpeg4 video codec, something like if (ret == FRAME_SKIPPED) { + if (CONFIG_MPEG4_DECODER && actvx->codec_id == AV_CODEC_ID_MPEG4 && s->next_picture_ptr) { + if ((ret = av_frame_ref(pict, s->next_picture_ptr->f)) < 0) { + return ret; + } + *got_frame = 1; + } return get_consumed_bytes(s, buf_size); + } or should i do something specific for xvid bug? I could return another value for the xvid (packed frames) here https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/mpeg4videodec.c#L2590 and check for it instead of the frame_skipped. For standard mpeg4video, outputing the next frame smells like violating > the spec. > > i'm not output the next frame, i'm output the last decoded frame The problem we have is that we have video with the last 5 second of it being xvid packed frames. When we demux the video, we assume the video to be 5 second long, but when we decode it, we have only the first 2 frames, then all the others are packed frames and so, the output is 2 frames long (instead of 5 seconds) Here are the options we were thinking: * if packed frames, duplicate and return the last frame if we do have it (this is this patch) * increase the next PTS even if we have packed frames, so the last PTS is passed down correctly to the filter chain when closing it and using the fps filter, we could pad the last 5 second missing. > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Observe your enemies, for they first find out your faults. -- Antisthenes > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
On Thu, Oct 19, 2017 at 04:20:48PM -0700, Thierry Foucu wrote: > On Thu, Oct 19, 2017 at 3:43 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > On Thu, Oct 19, 2017 at 09:51:05AM -0700, Thierry Foucu wrote: > > > Instead of returning nothing when we detect the xvid skipped frame, we > > > could return the last decoded frame, if we do have one. > > > > FRAME_SKIPPED is not limited to xvid (packed frames). There are a few > > other pathes that return it. > > > > > What if i change the test to check for mpeg4 video codec, something like > > if (ret == FRAME_SKIPPED) { > + if (CONFIG_MPEG4_DECODER && actvx->codec_id == AV_CODEC_ID_MPEG4 > && s->next_picture_ptr) { > + if ((ret = av_frame_ref(pict, s->next_picture_ptr->f)) < 0) { > + return ret; > + } > + *got_frame = 1; > + } > return get_consumed_bytes(s, buf_size); > + } > > or should i do something specific for xvid bug? I could return another > value for the xvid (packed frames) here > https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/mpeg4videodec.c#L2590 > > and check for it instead of the frame_skipped. FRAME_SKIPPED is returned in several cases * vop not coded Your code seems correct for this but a testcase would be very usefull to confirm this It would be more ideal to limit this to the case where the last frame differs from the returned frame. As outputting no frame is cheaper than outputing a repeated frame * some xvid/divx specific cases I dont know what is correct here * some error conditions Your code is probably not optimal in some cases here * skiped frames in i263 your patch shouldnt lead to wrong output but it would slow the code down by having to handle more frames > > For standard mpeg4video, outputing the next frame smells like violating > > the spec. > > > > > i'm not output the next frame, i'm output the last decoded frame i dont think thats the case with B frames. next/last are the reference frames for B frames, If the last is a B frame it should not be in there. What your code does seems to match what the standard mpeg4 wants for vop_coded = 0 though. (this would make your code a bugfix for a case i think i have never seen, vop_coded = 0 with b frames) [...]
Michael, the following patch seems to work. I restricted the change to N_VOP (from xvid) frame. I tried to match it with what the xvidcore decoder does for such packets. What do you think?
Michael, what do you think of this patch? I tried to reflect what the xvid decoder does for those N_VOP frame and in case it is not packed. On Tue, Oct 24, 2017 at 6:42 PM, Thierry Foucu <tfoucu@gmail.com> wrote: > Changed the return value when no VOD were encoded in Mpeg4 bistream. > And if we do have already a decoded frames and we are not in xvid_packed > mode, output the existing decoded frame instead of nothing. > --- > libavcodec/h263dec.c | 9 ++++++++- > libavcodec/mpeg4videodec.c | 2 +- > libavcodec/mpegutils.h | 1 + > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c > index c7cf4bc0c2..394a366f9c 100644 > --- a/libavcodec/h263dec.c > +++ b/libavcodec/h263dec.c > @@ -506,8 +506,15 @@ retry: > s->height= avctx->coded_height; > } > } > - if (ret == FRAME_SKIPPED) > + if (ret == FRAME_SKIPPED || ret == FRAME_NOT_CODED) { > + if (s->next_picture_ptr && ret == FRAME_NOT_CODED && > !s->divx_packed) { > + if ((ret = av_frame_ref(pict, s->next_picture_ptr->f)) < 0) { > + return ret; > + } > + *got_frame = 1; > + } > return get_consumed_bytes(s, buf_size); > + } > > /* skip if the header was thrashed */ > if (ret < 0) { > diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c > index 82c4f8fc8c..3a9ed12971 100644 > --- a/libavcodec/mpeg4videodec.c > +++ b/libavcodec/mpeg4videodec.c > @@ -2394,7 +2394,7 @@ static int decode_vop_header(Mpeg4DecContext *ctx, > GetBitContext *gb) > if (get_bits1(gb) != 1) { > if (s->avctx->debug & FF_DEBUG_PICT_INFO) > av_log(s->avctx, AV_LOG_ERROR, "vop not coded\n"); > - return FRAME_SKIPPED; > + return FRAME_NOT_CODED; > } > if (ctx->new_pred) > decode_new_pred(ctx, gb); > diff --git a/libavcodec/mpegutils.h b/libavcodec/mpegutils.h > index 1bf73fea02..97786135c6 100644 > --- a/libavcodec/mpegutils.h > +++ b/libavcodec/mpegutils.h > @@ -32,6 +32,7 @@ > * Return value for header parsers if frame is not coded. > * */ > #define FRAME_SKIPPED 100 > +#define FRAME_NOT_CODED 101 > > /* picture type */ > #define PICT_TOP_FIELD 1 > -- > 2.15.0.rc0.271.g36b669edcc-goog > >
diff --git a/libavcodec/h263dec.c b/libavcodec/h263dec.c index bcb2b08bb0..e1667bcbc1 100644 --- a/libavcodec/h263dec.c +++ b/libavcodec/h263dec.c @@ -507,8 +507,15 @@ retry: s->height= avctx->coded_height; } } - if (ret == FRAME_SKIPPED) + if (ret == FRAME_SKIPPED) { + if (s->next_picture_ptr) { + if ((ret = av_frame_ref(pict, s->next_picture_ptr->f)) < 0) { + return ret; + } + *got_frame = 1; + } return get_consumed_bytes(s, buf_size); + } /* skip if the header was thrashed */ if (ret < 0) {