Quoting Vittorio Giovara (2015-11-08 20:19:05)
> On Sun, Nov 8, 2015 at 11:54 AM, Anton Khirnov <[email protected]> wrote:
> > All encoders set pts and dts properly now (and have been doing that for
> > a while), so there is no good reason to do any timestamp guessing in the
> > muxer.
> > ---
> > libavformat/avformat.h | 8 ++++++
> > libavformat/internal.h | 12 ++++++++
> > libavformat/mux.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > libavformat/utils.c | 10 +++++++
> > libavformat/version.h | 3 ++
> > 5 files changed, 108 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> > index 470bbc6..e441486 100644
> > --- a/libavformat/avformat.h
> > +++ b/libavformat/avformat.h
> > @@ -688,6 +688,8 @@ typedef struct AVIndexEntry {
> > */
> > #define AV_DISPOSITION_ATTACHED_PIC 0x0400
> >
> > +typedef struct AVStreamInternal AVStreamInternal;
> > +
> > /**
> > * Stream structure.
> > * New fields can be added to the end with minor version bumps.
> > @@ -878,6 +880,12 @@ typedef struct AVStream {
> > support seeking natively. */
> > int nb_index_entries;
> > unsigned int index_entries_allocated_size;
> > +
> > + /**
> > + * An opaque field for libavformat internal usage.
> > + * Must not be accessed in any way by callers.
> > + */
> > + AVStreamInternal *internal;
> > } AVStream;
> >
> > #define AV_PROGRAM_RUNNING 1
> > diff --git a/libavformat/internal.h b/libavformat/internal.h
> > index a65a3b7..7bbf775 100644
> > --- a/libavformat/internal.h
> > +++ b/libavformat/internal.h
> > @@ -90,6 +90,18 @@ struct AVFormatInternal {
> > * Timebase for the timestamp offset.
> > */
> > AVRational offset_timebase;
> > +
> > +#if FF_API_COMPUTE_PKT_FIELDS2
> > + int missing_ts_warning;
> > +#endif
> > +};
> > +
> > +struct AVStreamInternal {
> > + /**
> > + * Set to 1 if the codec allows reordering, so pts can be different
> > + * from dts.
> > + */
> > + int reorder;
> > };
>
> Is this new private field necessary? if reordering is the only
> usecase, couldn't we just check the AV_CODEC_PROP_REORDER flag from
> avctx->codec?
There is no avctx->codec available to the muxer.
This is a property of the codec id, so we either have to look up the
corresponding codec descripter at each call, or do what I did.
>
> > -int av_write_frame(AVFormatContext *s, AVPacket *pkt)
> > +static int prepare_input_packet(AVFormatContext *s, AVPacket *pkt)
> > {
> > int ret;
>
> would it be a problem to move the prepare_input_packet() factorization
> to a separate patch?
There is no factorization, I'm just renaming the check_packet()
function. Since it no longer just checks, but potentially also modifies
the packet, the old name would be misleading.
>
> > @@ -389,16 +406,70 @@ int av_write_frame(AVFormatContext *s, AVPacket *pkt)
> > if (ret < 0)
> > return ret;
> >
> > +#if !FF_API_COMPUTE_PKT_FIELDS2
> > + /* sanitize the timestamps */
> > + if (!(s->oformat->flags & AVFMT_NOTIMESTAMPS)) {
> > + AVStream *st = s->streams[pkt->stream_index];
> > +
> > + /* when there is no reordering (so dts is equal to pts), but
> > + * only one of them is set, set the other as well */
> > + if (!st->internal->reorder) {
> > + if (pkt->pts == AV_NOPTS_VALUE && pkt->dts != AV_NOPTS_VALUE)
> > + pkt->pts = pkt->dts;
> > + if (pkt->dts == AV_NOPTS_VALUE && pkt->pts != AV_NOPTS_VALUE)
> > + pkt->dts = pkt->pts;
> > + }
> > +
> > + /* check that the timestamps are set */
> > + if (pkt->pts == AV_NOPTS_VALUE || pkt->dts == AV_NOPTS_VALUE) {
> > + av_log(s, AV_LOG_ERROR,
> > + "Timestamps are unset in a packet for stream %d\n",
> > st->index);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + /* check that the dts are increasing (or at least non-decreasing,
> > + * if the format allows it */
> > + if (st->cur_dts != AV_NOPTS_VALUE &&
> > + ((!(s->oformat->flags & AVFMT_TS_NONSTRICT) && st->cur_dts >=
> > pkt->dts) ||
> > + st->cur_dts > pkt->dts)) {
> > + av_log(s, AV_LOG_ERROR,
> > + "Application provided invalid, non monotonically
> > increasing "
> > + "dts to muxer in stream %d: %" PRId64 " >= %" PRId64
> > "\n",
> > + st->index, st->cur_dts, pkt->dts);
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (pkt->pts < pkt->dts) {
> > + av_log(s, AV_LOG_ERROR, "pts %" PRId64 " < dts %" PRId64 " in
> > stream %d\n",
> > + pkt->pts, pkt->dts, st->index);
> > + return AVERROR(EINVAL);
> > + }
> > + }
> > +#endif
> > +
> > + return 0;
> > +}
> > +
> > +int av_write_frame(AVFormatContext *s, AVPacket *pkt)
> > +{
> > + int ret;
> > +
> > + ret = prepare_input_packet(s, pkt);
> > + if (ret < 0)
> > + return ret;
> > +
> > if (!pkt) {
> > if (s->oformat->flags & AVFMT_ALLOW_FLUSH)
> > return s->oformat->write_packet(s, pkt);
> > return 1;
> > }
> >
> > +#if FF_API_COMPUTE_PKT_FIELDS2
> > ret = compute_pkt_fields2(s, s->streams[pkt->stream_index], pkt);
> >
> > if (ret < 0 && !(s->oformat->flags & AVFMT_NOTIMESTAMPS))
> > return ret;
> > +#endif
> >
> > ret = write_packet(s, pkt);
> >
> > @@ -552,7 +623,7 @@ int av_interleaved_write_frame(AVFormatContext *s,
> > AVPacket *pkt)
> > {
> > int ret, flush = 0;
> >
> > - ret = check_packet(s, pkt);
> > + ret = prepare_input_packet(s, pkt);
> > if (ret < 0)
> > goto fail;
> >
> > @@ -561,8 +632,10 @@ int av_interleaved_write_frame(AVFormatContext *s,
> > AVPacket *pkt)
> >
> > av_log(s, AV_LOG_TRACE, "av_interleaved_write_frame size:%d dts:%"
> > PRId64 " pts:%" PRId64 "\n",
> > pkt->size, pkt->dts, pkt->pts);
> > +#if FF_API_COMPUTE_PKT_FIELDS2
> > if ((ret = compute_pkt_fields2(s, st, pkt)) < 0 &&
> > !(s->oformat->flags & AVFMT_NOTIMESTAMPS))
> > goto fail;
> > +#endif
> >
> > if (pkt->dts == AV_NOPTS_VALUE && !(s->oformat->flags &
> > AVFMT_NOTIMESTAMPS)) {
> > ret = AVERROR(EINVAL);
> > diff --git a/libavformat/utils.c b/libavformat/utils.c
> > index 734f83f..bb17b4a 100644
> > --- a/libavformat/utils.c
> > +++ b/libavformat/utils.c
> > @@ -2460,6 +2460,8 @@ static void free_stream(AVStream **pst)
> > if (st->attached_pic.data)
> > av_packet_unref(&st->attached_pic);
> >
> > + av_freep(&st->internal);
> > +
> > av_dict_free(&st->metadata);
> > av_freep(&st->probe_data.buf);
> > av_free(st->index_entries);
> > @@ -2551,6 +2553,11 @@ AVStream *avformat_new_stream(AVFormatContext *s,
> > const AVCodec *c)
> > av_free(st);
> > return NULL;
> > }
> > +
> > + st->internal = av_mallocz(sizeof(*st->internal));
> > + if (!st->internal)
> > + goto fail;
> > +
> > if (s->iformat) {
> > /* no default bitrate if decoding */
> > st->codec->bit_rate = 0;
> > @@ -2583,6 +2590,9 @@ AVStream *avformat_new_stream(AVFormatContext *s,
> > const AVCodec *c)
> >
> > s->streams[s->nb_streams++] = st;
> > return st;
> > +fail:
> > + free_stream(&st);
> > + return NULL;
> > }
> >
> > AVProgram *av_new_program(AVFormatContext *ac, int id)
> > diff --git a/libavformat/version.h b/libavformat/version.h
> > index d74968a..d004a55 100644
> > --- a/libavformat/version.h
> > +++ b/libavformat/version.h
> > @@ -60,5 +60,8 @@
> > #ifndef FF_API_LAVF_FMT_RAWPICTURE
> > #define FF_API_LAVF_FMT_RAWPICTURE (LIBAVFORMAT_VERSION_MAJOR < 58)
> > #endif
> > +#ifndef FF_API_COMPUTE_PKT_FIELDS2
> > +#define FF_API_COMPUTE_PKT_FIELDS2 (LIBAVFORMAT_VERSION_MAJOR < 58)
> > +#endif
>
> I might be missing something, but aren't FF_API defines only for
> public fields? This is mostly private so you could change the code
> right away couldn't you?
I could, but callers that do not set the timestamps properly would
break. Since we supported that until now, it makes more sense to
deprecate it first.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
