| Message ID | 20171025014254.31336-2-tfoucu@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 24, 2017 at 06:42:54PM -0700, Thierry Foucu 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); the return codes are documented currently: * @return <0 if no VOP found (or a damaged one) * FRAME_SKIPPED if a not coded VOP is found * 0 if a VOP is found you added a case but did not update all use cases which seems to lead to intermittent crashes (i cant say for sure as i failed to reproduce one in a debugger or with any debuging code added) Also returning frames for skiped frames when te match the previous would slow the code down as more frames would need to be processed. more so i think this is not the only codec that can skip frames so any downstream problems will likely not be worked around by forcing constant fps with frame duplication here [...]
On 10/25/2017 2:42 AM, Thierry Foucu 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(-) Would this considerably slow down pathological files with a lot of NVOPs purposely inserted? For example, it use to be A Thing to use NVOPs to hack in "variable framerate" Xvid-in-avi encodes, by for example, setting the fps to 120 for a file with 24 and 30 fps sections, and padding the rest with NVOPs. Do we care about such files? (Probably not...) I can provide some samples, probably, if needed. - Derek
Derek, On Wed, Oct 25, 2017 at 9:43 AM, Derek Buitenhuis < derek.buitenhuis@gmail.com> wrote: > On 10/25/2017 2:42 AM, Thierry Foucu 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(-) > > Would this considerably slow down pathological files with a lot of > NVOPs purposely inserted? For example, it use to be A Thing to use > NVOPs to hack in "variable framerate" Xvid-in-avi encodes, by for > example, setting the fps to 120 for a file with 24 and 30 fps sections, > and padding the rest with NVOPs. > I tried using one of those files. Without the patch, the decoder will return only 29 frames With this patch, the decoder will return 1947 frames. The time of decoding the file were the same (with or without) I think where it may slow down, it's in case we transcode the file and we are not doing constant frame rate, then the encoder will have to encode 1947 frames instead of 24 frames. But for decoding it the same. > > Do we care about such files? (Probably not...) > > I can provide some samples, probably, if needed. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Oct 25, 2017 at 4:59 AM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Tue, Oct 24, 2017 at 06:42:54PM -0700, Thierry Foucu 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); > > the return codes are documented currently: > * @return <0 if no VOP found (or a damaged one) > * FRAME_SKIPPED if a not coded VOP is found > * 0 if a VOP is found > > you added a case but did not update all use cases > > I can update the text here. But i was waiting to get some idea if the patch seems to be ok for all of you or not. If the idea sounds ok, I will update it. > which seems to > lead to intermittent crashes (i cant say for sure as i failed to > reproduce one in a debugger or with any debuging code added) > > i tried some files here and could not get any crash so far. I used asan as well to find out if something was wrong. > Also returning frames for skiped frames when te match the previous > would slow the code down as more frames would need to be processed. > more so i think this is not the only codec that can skip frames > so any downstream problems will likely not be worked around by forcing > constant fps with frame duplication here > > True, but the decoder does not return a frame, so the got_output is set to false for those packets and in this case, we are not increasing the ist->next_pts So, in we do have a lot of n_vop frame at the end, we are not setting the next_pts to them and when we are closing the filtergraph, we are setting the EOF time to the last decoded frame. And so, we are not padding the video to match the original timestamp/duration of the input. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > It is what and why we do it that matters, not just one of them. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
This time to the list properly... woops. On 10/25/2017 7:38 PM, Thierry Foucu wrote: > I tried using one of those files. > Without the patch, the decoder will return only 29 frames > With this patch, the decoder will return 1947 frames. > The time of decoding the file were the same (with or without) Makes sense (because of reference counting). > I think where it may slow down, it's in case we transcode the file and we > are not doing constant frame rate, then the encoder will have to encode > 1947 frames instead of 24 frames. > But for decoding it the same. Yeah, that's what I thought might happen. However, thinking about it a bit more, these files are old and rare enough that we probably shouldn't care about such an edge case, and the decoder outputting duplicate frames is probably easier for the API user. So, no objection from me. - Derek
2017-10-25 20:38 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>:
> Without the patch, the decoder will return only 29 frames
Isn't that the correct and expected output?
If you need cfr, you can request it.
Once the frames are duplicated, you can't undo the file
size multiplication.
Carl Eugen
On Wed, Oct 25, 2017 at 2:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-10-25 20:38 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: > > > Without the patch, the decoder will return only 29 frames > > Isn't that the correct and expected output? > Not sure what you mean by correct. There are only 29 VOP encoded, and over 1900 N_VOP packets. If you just demux the sample file, you will get 1947 packets I used the Xvid decoder from http://svn.xvid.org/trunk/xvidcore/examples/xvid_decraw.c to decode the same file, and their decoder outputted 1947 frames. > > If you need cfr, you can request it. > true, but this works as long as you have 2 frames and you can cfr between them. But what if you have the last X second of the file with only N_VOP frames In this case, the decoder will not output frames and the transcoded file will be shorter by X second > Once the frames are duplicated, you can't undo the file > size multiplication. > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Oct 25, 2017 at 04:21:28PM -0700, Thierry Foucu wrote: > On Wed, Oct 25, 2017 at 2:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > 2017-10-25 20:38 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: > > > > > Without the patch, the decoder will return only 29 frames > > > > Isn't that the correct and expected output? > > > > Not sure what you mean by correct. > There are only 29 VOP encoded, and over 1900 N_VOP packets. > If you just demux the sample file, you will get 1947 packets > > I used the Xvid decoder from > http://svn.xvid.org/trunk/xvidcore/examples/xvid_decraw.c > > to decode the same file, and their decoder outputted 1947 frames. > > > > > > If you need cfr, you can request it. > > > > true, but this works as long as you have 2 frames and you can cfr between > them. But what if you have the last X second of the file with only N_VOP > frames > In this case, the decoder will not output frames and the transcoded file > will be shorter by X second the decoder could output a final frame during decoder flush to make sure the last NVOPs duration is not lost [...]
On Wed, Oct 25, 2017 at 4:36 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > On Wed, Oct 25, 2017 at 04:21:28PM -0700, Thierry Foucu wrote: > > On Wed, Oct 25, 2017 at 2:43 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > > > 2017-10-25 20:38 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: > > > > > > > Without the patch, the decoder will return only 29 frames > > > > > > Isn't that the correct and expected output? > > > > > > > Not sure what you mean by correct. > > There are only 29 VOP encoded, and over 1900 N_VOP packets. > > If you just demux the sample file, you will get 1947 packets > > > > I used the Xvid decoder from > > http://svn.xvid.org/trunk/xvidcore/examples/xvid_decraw.c > > > > to decode the same file, and their decoder outputted 1947 frames. > > > > > > > > > > If you need cfr, you can request it. > > > > > > > true, but this works as long as you have 2 frames and you can cfr between > > them. But what if you have the last X second of the file with only N_VOP > > frames > > In this case, the decoder will not output frames and the transcoded file > > will be shorter by X second > > the decoder could output a final frame during decoder flush to make > sure the last NVOPs duration is not lost > Oh, that's sound interesting. Let me see if i understand the code enough to do exactly this. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Those who are too smart to engage in politics are punished by being > governed by those who are dumber. -- Plato > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
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