| Message ID | 20171208042316.94655-2-rodger.combs@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
2017-12-08 5:23 GMT+01:00 Rodger Combs <rodger.combs@gmail.com>: > + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && > + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { The brackets look ugly. Carl Eugen
On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: > --- > libavformat/avformat.h | 1 + > libavformat/options_table.h | 1 + > libavformat/utils.c | 8 ++++++++ > 3 files changed, 10 insertions(+) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 4f2798a871..d10d583dff 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { > #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats > #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. > #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer > +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt > > /** > * Maximum size of the data read from input for determining > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > index b8fa47c6fd..515574d3e0 100644 > --- a/libavformat/options_table.h > +++ b/libavformat/options_table.h > @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { > {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, > {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, > {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, > +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, > {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, > {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, > {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 84e49208b8..98af941e9f 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > st->cur_dts = wrap_timestamp(st, st->cur_dts); > } > > + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && > + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { > + pkt->pts = pkt->dts = AV_NOPTS_VALUE; > + av_log(s, AV_LOG_WARNING, > + "Discarded timestamp on corrupted packet (stream = %d)\n", > + pkt->stream_index); > + } how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to the timestamp ? We set this for truncated / EOF cases and all kinds of stuff that are known to be unrelated to the timestamps [...]
> On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: >> --- >> libavformat/avformat.h | 1 + >> libavformat/options_table.h | 1 + >> libavformat/utils.c | 8 ++++++++ >> 3 files changed, 10 insertions(+) >> >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >> index 4f2798a871..d10d583dff 100644 >> --- a/libavformat/avformat.h >> +++ b/libavformat/avformat.h >> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { >> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats >> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. >> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer >> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt >> >> /** >> * Maximum size of the data read from input for determining >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h >> index b8fa47c6fd..515574d3e0 100644 >> --- a/libavformat/options_table.h >> +++ b/libavformat/options_table.h >> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { >> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, >> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, >> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, >> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, >> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, >> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, >> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index 84e49208b8..98af941e9f 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c > >> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) >> st->cur_dts = wrap_timestamp(st, st->cur_dts); >> } >> >> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && >> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { >> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; >> + av_log(s, AV_LOG_WARNING, >> + "Discarded timestamp on corrupted packet (stream = %d)\n", >> + pkt->stream_index); >> + } > > how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to > the timestamp ? Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? > > We set this for truncated / EOF cases and all kinds of stuff that > are known to be unrelated to the timestamps > > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The real ebay dictionary, page 1 > "Used only once" - "Some unspecified defect prevented a second use" > "In good condition" - "Can be repaird by experienced expert" > "As is" - "You wouldnt want it even if you were payed for it, if you knew ..." > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
On Fri, Dec 08, 2017 at 10:34:39PM -0600, Rodger Combs wrote: > > > > On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: > >> --- > >> libavformat/avformat.h | 1 + > >> libavformat/options_table.h | 1 + > >> libavformat/utils.c | 8 ++++++++ > >> 3 files changed, 10 insertions(+) > >> > >> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >> index 4f2798a871..d10d583dff 100644 > >> --- a/libavformat/avformat.h > >> +++ b/libavformat/avformat.h > >> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { > >> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats > >> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. > >> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer > >> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt > >> > >> /** > >> * Maximum size of the data read from input for determining > >> diff --git a/libavformat/options_table.h b/libavformat/options_table.h > >> index b8fa47c6fd..515574d3e0 100644 > >> --- a/libavformat/options_table.h > >> +++ b/libavformat/options_table.h > >> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { > >> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, > >> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, > >> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, > >> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, > >> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, > >> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, > >> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, > >> diff --git a/libavformat/utils.c b/libavformat/utils.c > >> index 84e49208b8..98af941e9f 100644 > >> --- a/libavformat/utils.c > >> +++ b/libavformat/utils.c > > > >> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > >> st->cur_dts = wrap_timestamp(st, st->cur_dts); > >> } > >> > >> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && > >> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { > >> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; > >> + av_log(s, AV_LOG_WARNING, > >> + "Discarded timestamp on corrupted packet (stream = %d)\n", > >> + pkt->stream_index); > >> + } > > > > how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to > > the timestamp ? > > Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? I think thats a great idea, maybe if we already change it we should also split the truncated case out as it is very common. AV_PKT_FLAG_DATA_CORRUPT // the packet data may be corrupted AV_PKT_FLAG_DATA_TRUNCATED // the packet size may be corrupted, data available is undamaged unless another flag indicates otherwise. Any headers or sub-packets which are complete are non corrupted- AV_PKT_FLAG_TS_CORRUPT // the packets timestamps and or duration may be corrupted #define AV_PKT_FLAG_CORRUPT (AV_PKT_FLAG_DATA_CORRUPT | AV_PKT_FLAG_DATA_TRUNCATED | AV_PKT_FLAG_TS_CORRUPT) or some variation of this Thanks [...]
> On Dec 9, 2017, at 12:19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, Dec 08, 2017 at 10:34:39PM -0600, Rodger Combs wrote: >> >> >>> On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >>> On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: >>>> --- >>>> libavformat/avformat.h | 1 + >>>> libavformat/options_table.h | 1 + >>>> libavformat/utils.c | 8 ++++++++ >>>> 3 files changed, 10 insertions(+) >>>> >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>> index 4f2798a871..d10d583dff 100644 >>>> --- a/libavformat/avformat.h >>>> +++ b/libavformat/avformat.h >>>> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { >>>> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats >>>> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. >>>> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer >>>> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt >>>> >>>> /** >>>> * Maximum size of the data read from input for determining >>>> diff --git a/libavformat/options_table.h b/libavformat/options_table.h >>>> index b8fa47c6fd..515574d3e0 100644 >>>> --- a/libavformat/options_table.h >>>> +++ b/libavformat/options_table.h >>>> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { >>>> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, >>>> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, >>>> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, >>>> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, >>>> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, >>>> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, >>>> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, >>>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>>> index 84e49208b8..98af941e9f 100644 >>>> --- a/libavformat/utils.c >>>> +++ b/libavformat/utils.c >>> >>>> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) >>>> st->cur_dts = wrap_timestamp(st, st->cur_dts); >>>> } >>>> >>>> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && >>>> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { >>>> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; >>>> + av_log(s, AV_LOG_WARNING, >>>> + "Discarded timestamp on corrupted packet (stream = %d)\n", >>>> + pkt->stream_index); >>>> + } >>> >>> how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to >>> the timestamp ? >> >> Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? > > I think thats a great idea, maybe if we already change it we should > also split the truncated case out as it is very common. > > AV_PKT_FLAG_DATA_CORRUPT // the packet data may be corrupted > AV_PKT_FLAG_DATA_TRUNCATED // the packet size may be corrupted, data available is undamaged unless another flag indicates otherwise. Any headers or sub-packets which are complete are non corrupted- > AV_PKT_FLAG_TS_CORRUPT // the packets timestamps and or duration may be corrupted > > #define AV_PKT_FLAG_CORRUPT (AV_PKT_FLAG_DATA_CORRUPT | AV_PKT_FLAG_DATA_TRUNCATED | AV_PKT_FLAG_TS_CORRUPT) What kind of API bump would be needed to make this kind of change? > > or some variation of this > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > There will always be a question for which you do not know the correct answer. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
On Sun, Dec 10, 2017 at 05:17:44AM -0600, Rodger Combs wrote: > > > > On Dec 9, 2017, at 12:19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > On Fri, Dec 08, 2017 at 10:34:39PM -0600, Rodger Combs wrote: > >> > >> > >>> On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: > >>> > >>> On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: > >>>> --- > >>>> libavformat/avformat.h | 1 + > >>>> libavformat/options_table.h | 1 + > >>>> libavformat/utils.c | 8 ++++++++ > >>>> 3 files changed, 10 insertions(+) > >>>> > >>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >>>> index 4f2798a871..d10d583dff 100644 > >>>> --- a/libavformat/avformat.h > >>>> +++ b/libavformat/avformat.h > >>>> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { > >>>> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats > >>>> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. > >>>> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer > >>>> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt > >>>> > >>>> /** > >>>> * Maximum size of the data read from input for determining > >>>> diff --git a/libavformat/options_table.h b/libavformat/options_table.h > >>>> index b8fa47c6fd..515574d3e0 100644 > >>>> --- a/libavformat/options_table.h > >>>> +++ b/libavformat/options_table.h > >>>> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { > >>>> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, > >>>> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, > >>>> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, > >>>> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, > >>>> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, > >>>> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, > >>>> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, > >>>> diff --git a/libavformat/utils.c b/libavformat/utils.c > >>>> index 84e49208b8..98af941e9f 100644 > >>>> --- a/libavformat/utils.c > >>>> +++ b/libavformat/utils.c > >>> > >>>> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > >>>> st->cur_dts = wrap_timestamp(st, st->cur_dts); > >>>> } > >>>> > >>>> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && > >>>> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { > >>>> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; > >>>> + av_log(s, AV_LOG_WARNING, > >>>> + "Discarded timestamp on corrupted packet (stream = %d)\n", > >>>> + pkt->stream_index); > >>>> + } > >>> > >>> how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to > >>> the timestamp ? > >> > >> Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? > > > > I think thats a great idea, maybe if we already change it we should > > also split the truncated case out as it is very common. > > > > AV_PKT_FLAG_DATA_CORRUPT // the packet data may be corrupted > > AV_PKT_FLAG_DATA_TRUNCATED // the packet size may be corrupted, data available is undamaged unless another flag indicates otherwise. Any headers or sub-packets which are complete are non corrupted- > > AV_PKT_FLAG_TS_CORRUPT // the packets timestamps and or duration may be corrupted > > > > #define AV_PKT_FLAG_CORRUPT (AV_PKT_FLAG_DATA_CORRUPT | AV_PKT_FLAG_DATA_TRUNCATED | AV_PKT_FLAG_TS_CORRUPT) > > What kind of API bump would be needed to make this kind of change? IIUC we are still within the major bump thing (didnt check but others have said it in relation to other changes) a minor bump should probably be done anyway to signal that these additional flag may be set in the output [...]
Michael Niedermayer (2017-12-10): > IIUC we are still within the major bump thing (didnt check but others > have said it in relation to other changes) Well, there are no laws that say when we leave the "major bump thing". We dice how long it lasts. I can suggest this: versions with minor 0 have an unstable ABI; we mark the end of the "major bump thing" by bumping the minor. Regards,
On Sun, Dec 10, 2017 at 04:30:25PM +0100, Nicolas George wrote: > Michael Niedermayer (2017-12-10): > > IIUC we are still within the major bump thing (didnt check but others > > have said it in relation to other changes) > > Well, there are no laws that say when we leave the "major bump thing". > We dice how long it lasts. > > I can suggest this: versions with minor 0 have an unstable ABI; we mark > the end of the "major bump thing" by bumping the minor. we could do this next time, we did not do this in the past and cannot now as minor has already been bumped also this would add the restriction that we could not do any versioning during this period which would be a bad idea IMHO [...]
> On Dec 10, 2017, at 09:27, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, Dec 10, 2017 at 05:17:44AM -0600, Rodger Combs wrote: >> >> >>> On Dec 9, 2017, at 12:19, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >>> On Fri, Dec 08, 2017 at 10:34:39PM -0600, Rodger Combs wrote: >>>> >>>> >>>>> On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: >>>>> >>>>> On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: >>>>>> --- >>>>>> libavformat/avformat.h | 1 + >>>>>> libavformat/options_table.h | 1 + >>>>>> libavformat/utils.c | 8 ++++++++ >>>>>> 3 files changed, 10 insertions(+) >>>>>> >>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>>>>> index 4f2798a871..d10d583dff 100644 >>>>>> --- a/libavformat/avformat.h >>>>>> +++ b/libavformat/avformat.h >>>>>> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { >>>>>> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats >>>>>> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. >>>>>> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer >>>>>> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt >>>>>> >>>>>> /** >>>>>> * Maximum size of the data read from input for determining >>>>>> diff --git a/libavformat/options_table.h b/libavformat/options_table.h >>>>>> index b8fa47c6fd..515574d3e0 100644 >>>>>> --- a/libavformat/options_table.h >>>>>> +++ b/libavformat/options_table.h >>>>>> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { >>>>>> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, >>>>>> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, >>>>>> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, >>>>>> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, >>>>>> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, >>>>>> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, >>>>>> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, >>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c >>>>>> index 84e49208b8..98af941e9f 100644 >>>>>> --- a/libavformat/utils.c >>>>>> +++ b/libavformat/utils.c >>>>> >>>>>> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) >>>>>> st->cur_dts = wrap_timestamp(st, st->cur_dts); >>>>>> } >>>>>> >>>>>> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && >>>>>> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { >>>>>> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; >>>>>> + av_log(s, AV_LOG_WARNING, >>>>>> + "Discarded timestamp on corrupted packet (stream = %d)\n", >>>>>> + pkt->stream_index); >>>>>> + } >>>>> >>>>> how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to >>>>> the timestamp ? >>>> >>>> Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? >>> >>> I think thats a great idea, maybe if we already change it we should >>> also split the truncated case out as it is very common. >>> >>> AV_PKT_FLAG_DATA_CORRUPT // the packet data may be corrupted >>> AV_PKT_FLAG_DATA_TRUNCATED // the packet size may be corrupted, data available is undamaged unless another flag indicates otherwise. Any headers or sub-packets which are complete are non corrupted- >>> AV_PKT_FLAG_TS_CORRUPT // the packets timestamps and or duration may be corrupted >>> >>> #define AV_PKT_FLAG_CORRUPT (AV_PKT_FLAG_DATA_CORRUPT | AV_PKT_FLAG_DATA_TRUNCATED | AV_PKT_FLAG_TS_CORRUPT) >> >> What kind of API bump would be needed to make this kind of change? > > IIUC we are still within the major bump thing (didnt check but others > have said it in relation to other changes) > > a minor bump should probably be done anyway to signal that these > additional flag may be set in the output Come to think of it, I think a major bump would be required, as this would break ABI (applications checking for AV_PKT_FLAG_CORRUPT would need to be rebuilt to handle anything that sets AV_PKT_FLAG_DATA_CORRUPT or the others). > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Complexity theory is the science of finding the exact solution to an > approximation. Benchmarking OTOH is finding an approximation of the exact > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
On Wed, Dec 13, 2017 at 03:04:06AM -0600, Rodger Combs wrote: > > > > On Dec 10, 2017, at 09:27, Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > On Sun, Dec 10, 2017 at 05:17:44AM -0600, Rodger Combs wrote: > >> > >> > >>> On Dec 9, 2017, at 12:19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >>> > >>> On Fri, Dec 08, 2017 at 10:34:39PM -0600, Rodger Combs wrote: > >>>> > >>>> > >>>>> On Dec 8, 2017, at 11:06, Michael Niedermayer <michael@niedermayer.cc> wrote: > >>>>> > >>>>> On Thu, Dec 07, 2017 at 10:23:15PM -0600, Rodger Combs wrote: > >>>>>> --- > >>>>>> libavformat/avformat.h | 1 + > >>>>>> libavformat/options_table.h | 1 + > >>>>>> libavformat/utils.c | 8 ++++++++ > >>>>>> 3 files changed, 10 insertions(+) > >>>>>> > >>>>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h > >>>>>> index 4f2798a871..d10d583dff 100644 > >>>>>> --- a/libavformat/avformat.h > >>>>>> +++ b/libavformat/avformat.h > >>>>>> @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { > >>>>>> #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats > >>>>>> #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. > >>>>>> #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer > >>>>>> +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt > >>>>>> > >>>>>> /** > >>>>>> * Maximum size of the data read from input for determining > >>>>>> diff --git a/libavformat/options_table.h b/libavformat/options_table.h > >>>>>> index b8fa47c6fd..515574d3e0 100644 > >>>>>> --- a/libavformat/options_table.h > >>>>>> +++ b/libavformat/options_table.h > >>>>>> @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { > >>>>>> {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, > >>>>>> {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, > >>>>>> {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, > >>>>>> +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, > >>>>>> {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, > >>>>>> {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, > >>>>>> {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, > >>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c > >>>>>> index 84e49208b8..98af941e9f 100644 > >>>>>> --- a/libavformat/utils.c > >>>>>> +++ b/libavformat/utils.c > >>>>> > >>>>>> @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) > >>>>>> st->cur_dts = wrap_timestamp(st, st->cur_dts); > >>>>>> } > >>>>>> > >>>>>> + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && > >>>>>> + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { > >>>>>> + pkt->pts = pkt->dts = AV_NOPTS_VALUE; > >>>>>> + av_log(s, AV_LOG_WARNING, > >>>>>> + "Discarded timestamp on corrupted packet (stream = %d)\n", > >>>>>> + pkt->stream_index); > >>>>>> + } > >>>>> > >>>>> how many of the cases that set AV_PKT_FLAG_CORRUPT can even be due to > >>>>> the timestamp ? > >>>> > >>>> Pretty much just the new TEI check, or potentially other CRC checks. I don't _think_ the continuity-counter check failing could indicate an incorrect timestamp. I suppose we could add a second flag just for corruption that could affect timestamps? > >>> > >>> I think thats a great idea, maybe if we already change it we should > >>> also split the truncated case out as it is very common. > >>> > >>> AV_PKT_FLAG_DATA_CORRUPT // the packet data may be corrupted > >>> AV_PKT_FLAG_DATA_TRUNCATED // the packet size may be corrupted, data available is undamaged unless another flag indicates otherwise. Any headers or sub-packets which are complete are non corrupted- > >>> AV_PKT_FLAG_TS_CORRUPT // the packets timestamps and or duration may be corrupted > >>> > >>> #define AV_PKT_FLAG_CORRUPT (AV_PKT_FLAG_DATA_CORRUPT | AV_PKT_FLAG_DATA_TRUNCATED | AV_PKT_FLAG_TS_CORRUPT) > >> > >> What kind of API bump would be needed to make this kind of change? > > > > IIUC we are still within the major bump thing (didnt check but others > > have said it in relation to other changes) > > > > a minor bump should probably be done anyway to signal that these > > additional flag may be set in the output > > Come to think of it, I think a major bump would be required, as this would break ABI (applications checking for AV_PKT_FLAG_CORRUPT would need to be rebuilt to handle anything that sets AV_PKT_FLAG_DATA_CORRUPT or the others). yes of couse, thats why i talked about still being in the major bump API/ABI unstability period above If one wants to add these flags in a ABI compatible way, that can be done but its more messy, one can just add the new flags as independant new flags #define AV_PKT_FLAG_CORRUPT 0x0002 (unchanged) #define AV_PKT_FLAG_DATA_CORRUPT 0x0020 #define AV_PKT_FLAG_DATA_TRUNCATED 0x0040 #define AV_PKT_FLAG_TS_CORRUPT 0x0080 then to set one in the new code, code like this would be used flags |= AV_PKT_FLAG_CORRUPT | AV_PKT_FLAG_DATA_CORRUPT this is compatible with the old code to check code like this would be needed tmp = flags & 0x00E2 if (tmp == AV_PKT_FLAG_CORRUPT || (tmp&AV_PKT_FLAG_DATA_CORRUPT)) to support both the old style and new more specific style i dont think we need this though as we are quite close to the last ABI bump [...]
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 4f2798a871..d10d583dff 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -1450,6 +1450,7 @@ typedef struct AVFormatContext { #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Add bitstream filters as requested by the muxer +#define AVFMT_FLAG_DISCARD_CORRUPT_TS 0x400000 ///< Discard timestamps of frames marked corrupt /** * Maximum size of the data read from input for determining diff --git a/libavformat/options_table.h b/libavformat/options_table.h index b8fa47c6fd..515574d3e0 100644 --- a/libavformat/options_table.h +++ b/libavformat/options_table.h @@ -58,6 +58,7 @@ static const AVOption avformat_options[] = { {"bitexact", "do not write random/volatile data", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_BITEXACT }, 0, 0, E, "fflags" }, {"shortest", "stop muxing with the shortest stream", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_SHORTEST }, 0, 0, E, "fflags" }, {"autobsf", "add needed bsfs automatically", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_AUTO_BSF }, 0, 0, E, "fflags" }, +{"discardcorruptts", "discard timestamps on corrupted frames", 0, AV_OPT_TYPE_CONST, { .i64 = AVFMT_FLAG_DISCARD_CORRUPT_TS }, 0, 0, E, "fflags" }, {"analyzeduration", "specify how many microseconds are analyzed to probe the input", OFFSET(max_analyze_duration), AV_OPT_TYPE_INT64, {.i64 = 0 }, 0, INT64_MAX, D}, {"cryptokey", "decryption key", OFFSET(key), AV_OPT_TYPE_BINARY, {.dbl = 0}, 0, 0, D}, {"indexmem", "max memory used for timestamp index (per stream)", OFFSET(max_index_size), AV_OPT_TYPE_INT, {.i64 = 1<<20 }, 0, INT_MAX, D}, diff --git a/libavformat/utils.c b/libavformat/utils.c index 84e49208b8..98af941e9f 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -873,6 +873,14 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) st->cur_dts = wrap_timestamp(st, st->cur_dts); } + if ((s->flags & AVFMT_FLAG_DISCARD_CORRUPT_TS) && + (pkt->flags & AV_PKT_FLAG_CORRUPT)) { + pkt->pts = pkt->dts = AV_NOPTS_VALUE; + av_log(s, AV_LOG_WARNING, + "Discarded timestamp on corrupted packet (stream = %d)\n", + pkt->stream_index); + } + pkt->dts = wrap_timestamp(st, pkt->dts); pkt->pts = wrap_timestamp(st, pkt->pts);