| Message ID | 20181121155848.201788-3-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 21, 2018 at 03:58:47PM +0000, Derek Buitenhuis wrote: > Any FLV file, not necessarily valid, but in extremely common use for live > or archived live purposes, may output discontinuous timestamps. As it currently > is, only files produced by NGINX's RTMP module will be marked as supporting > discontinuous timestamps, which is obviously untrue, and the fix was only > ever put in place for a single bug report / file. The FLV demuxer, however > will happily ingest any FLV, and output discontinuous timestamps, regardless > of whether this "live" demuxer is used, making the current set of flags untrue. > > Add the flag to the main demuxer, as this is how it *actually* works. Lying to > downstream API users about a demuxer's behavior is not OK. > > Also set AVFMT_NOBINSEARCH, as this should be true given discontinuous timetsamps. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/flvdec.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) the specification says this: "Timestamp UI24 Time in milliseconds at which the data in this tag applies. This is relative to the first tag in the FLV file, which always has a timestamp of 0. " So flv does not seem to allow discontinuities. Any tool writing files with discontinuities would be faulty declaring files which have no discontinuities as having some would result in worse performing seeking in some cases. Both slower and or less accurate. Also flv IIRC allows gaps in tracks like no packets where there is silence. This may interfere with discontinuities as it looks like a discontinuity. Can these files with discontinuities be detected somehow from the headers? If so then the probe function could be changed so they get the discontinuity flag while valid files do not have the disadvantages from having it set Thanks [...]
On 22/11/2018 02:16, Michael Niedermayer wrote: > the specification says this: > "Timestamp UI24 Time in milliseconds at which the data in this tag > applies. This is relative to the first tag in the FLV file, > which always has a timestamp of 0. > " > So flv does not seem to allow discontinuities. Any tool writing files with > discontinuities would be faulty I know they're not valid, however in the real world, tons and tons of these files exists, since this is how live FLV is done quite often. > declaring files which have no discontinuities as having some would > result in worse performing seeking in some cases. Both slower and or less > accurate. Also flv IIRC allows gaps in tracks like no packets where there is > silence. This may interfere with discontinuities as it looks > like a discontinuity. I agree, but then how do you propose we handle FLVs that do have discontinuites? Currently the demuxer just outputs the discontinuiting (negative or positive), and it breaks, since codebases expect that demuxers which do this will be properly marked with the DISCONT flag. > Can these files with discontinuities be detected somehow from the headers? > If so then the probe function could be changed so they get the discontinuity > flag while valid files do not have the disadvantages from having it set As far as I am aware, no, they cannot be detected by header. FLV is crappy like that. Every downstream codebase that cares could force the 'live_flv' demuxer for all FLVs, but this seems ugly and needlessly special cased. I am open to better solutions... - Derek
2018-11-22 11:27 GMT+01:00, Derek Buitenhuis <derek.buitenhuis@gmail.com>: > Every downstream codebase that cares could force the 'live_flv' > demuxer for all FLVs, but this seems ugly and needlessly special cased. Only the downstreams that expect invalid files, no? No opinion here about the patch, Carl Eugen
On 22/11/2018 12:55, Carl Eugen Hoyos wrote:
> Only the downstreams that expect invalid files, no?
It's the generic way to handle containers which can have
discontinuous timestamps. Nothing to do with expecting invalid
files.
Cheers,
- Derek
On Thu, Nov 22, 2018 at 4:17 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Nov 21, 2018 at 03:58:47PM +0000, Derek Buitenhuis wrote: > > Any FLV file, not necessarily valid, but in extremely common use for live > > or archived live purposes, may output discontinuous timestamps. As it currently > > is, only files produced by NGINX's RTMP module will be marked as supporting > > discontinuous timestamps, which is obviously untrue, and the fix was only > > ever put in place for a single bug report / file. The FLV demuxer, however > > will happily ingest any FLV, and output discontinuous timestamps, regardless > > of whether this "live" demuxer is used, making the current set of flags untrue. > > > > Add the flag to the main demuxer, as this is how it *actually* works. Lying to > > downstream API users about a demuxer's behavior is not OK. > > > > Also set AVFMT_NOBINSEARCH, as this should be true given discontinuous timetsamps. > > > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > > --- > > libavformat/flvdec.c | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > the specification says this: > "Timestamp UI24 Time in milliseconds at which the data in this tag > applies. This is relative to the first tag in the FLV file, > which always has a timestamp of 0. > " > So flv does not seem to allow discontinuities. Any tool writing files with > discontinuities would be faulty How do you separate FLV files that came from RTMP originally (be it live or VOD) from files that were "just files"? Additionally, wasn't FLV with a time base of 1000 and a 32bit (signed in "file" format and unsigned in RTMP (?!?!)) time stamp. See 4d3dd167dfdfa2f36724f5613f05f46e3477c0d1 . I think I'm trying to sit down and note that FLV is a colossal mess for everyone at this point. > > declaring files which have no discontinuities as having some would > result in worse performing seeking in some cases. Both slower and or less > accurate. Also flv IIRC allows gaps in tracks like no packets where there is > silence. This may interfere with discontinuities as it looks > like a discontinuity. Yes, timestamps jumping is valid and I don't see this being similar to discontinuities. The fact that ffmpeg.c doesn't like intra-stream timestamp jumps in case this flag is set is orthogonal to this specific thing IMHO. > > Can these files with discontinuities be detected somehow from the headers? > If so then the probe function could be changed so they get the discontinuity > flag while valid files do not have the disadvantages from having it set > Not that I know, unless FLV dumped from RTMP somehow magically has some special identifier. I have not noticed any so far. Jan
On Thu, Nov 22, 2018 at 07:18:04PM +0200, Jan Ekström wrote: > On Thu, Nov 22, 2018 at 4:17 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Wed, Nov 21, 2018 at 03:58:47PM +0000, Derek Buitenhuis wrote: > > > Any FLV file, not necessarily valid, but in extremely common use for live > > > or archived live purposes, may output discontinuous timestamps. As it currently > > > is, only files produced by NGINX's RTMP module will be marked as supporting > > > discontinuous timestamps, which is obviously untrue, and the fix was only > > > ever put in place for a single bug report / file. The FLV demuxer, however > > > will happily ingest any FLV, and output discontinuous timestamps, regardless > > > of whether this "live" demuxer is used, making the current set of flags untrue. > > > > > > Add the flag to the main demuxer, as this is how it *actually* works. Lying to > > > downstream API users about a demuxer's behavior is not OK. > > > > > > Also set AVFMT_NOBINSEARCH, as this should be true given discontinuous timetsamps. > > > > > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > > > --- > > > libavformat/flvdec.c | 14 ++++---------- > > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > the specification says this: > > "Timestamp UI24 Time in milliseconds at which the data in this tag > > applies. This is relative to the first tag in the FLV file, > > which always has a timestamp of 0. > > " > > So flv does not seem to allow discontinuities. Any tool writing files with > > discontinuities would be faulty > > How do you separate FLV files that came from RTMP originally (be it > live or VOD) from files that were "just files"? > > Additionally, wasn't FLV with a time base of 1000 and a 32bit (signed > in "file" format and unsigned in RTMP (?!?!)) time stamp. See > 4d3dd167dfdfa2f36724f5613f05f46e3477c0d1 . > > I think I'm trying to sit down and note that FLV is a colossal mess > for everyone at this point. > > > > > declaring files which have no discontinuities as having some would > > result in worse performing seeking in some cases. Both slower and or less > > accurate. Also flv IIRC allows gaps in tracks like no packets where there is > > silence. This may interfere with discontinuities as it looks > > like a discontinuity. > > Yes, timestamps jumping is valid and I don't see this being similar to > discontinuities. The fact that ffmpeg.c doesn't like intra-stream > timestamp jumps in case this flag is set is orthogonal to this > specific thing IMHO. a simple example: 0,1,2,3,4,403,404,405 these are a files timestamps, is this a discontinuity or a gap of silence? [...]
On Thu, Nov 22, 2018 at 10:27:18AM +0000, Derek Buitenhuis wrote: > On 22/11/2018 02:16, Michael Niedermayer wrote: > > the specification says this: > > "Timestamp UI24 Time in milliseconds at which the data in this tag > > applies. This is relative to the first tag in the FLV file, > > which always has a timestamp of 0. > > " > > So flv does not seem to allow discontinuities. Any tool writing files with > > discontinuities would be faulty > > I know they're not valid, however in the real world, tons and tons of these files > exists, since this is how live FLV is done quite often. > > > declaring files which have no discontinuities as having some would > > result in worse performing seeking in some cases. Both slower and or less > > accurate. Also flv IIRC allows gaps in tracks like no packets where there is > > silence. This may interfere with discontinuities as it looks > > like a discontinuity. > > I agree, but then how do you propose we handle FLVs that do have discontinuites? > Currently the demuxer just outputs the discontinuiting (negative or positive), > and it breaks, since codebases expect that demuxers which do this will be properly > marked with the DISCONT flag. > > > Can these files with discontinuities be detected somehow from the headers? > > If so then the probe function could be changed so they get the discontinuity > > flag while valid files do not have the disadvantages from having it set > > As far as I am aware, no, they cannot be detected by header. FLV is crappy like that. do we have some sample flv files that require this patchset / that have discontinuites. another thing i just realize now, why is this discontinuity issues coming up now? flv support is very old. This should be a long standing issue thanks [...]
On 23/11/2018 02:16, Michael Niedermayer wrote: > do we have some sample flv files that require this patchset / that have > discontinuites. I have many. I've mailed you one privately, while I work on getting a public one. > another thing i just realize now, why is this discontinuity issues coming up > now? flv support is very old. This should be a long standing issue Probably not many codebases check the DISCONT flag. I only just added proper discontinuity handling to some codebases this year, and that's why *I* noticed. Chances are most people use the ffmpeg cli which seems to handle stuff differently, and doesn't necessarily care about the flag. In my case, I only try to 'fix' discontinuities if they appear to be in a format that allows them, during indexing. I could add a workaround specific to FLV, or some threshold stuff, but I'd prefer that demuxers which may output discontinuous timestamps say they do. Cheers, - Derek
On 26/11/2018 13:51, Derek Buitenhuis wrote: > On 23/11/2018 02:16, Michael Niedermayer wrote: >> do we have some sample flv files that require this patchset / that have >> discontinuites. > > I have many. I've mailed you one privately, while I work on getting a public one. > >> another thing i just realize now, why is this discontinuity issues coming up >> now? flv support is very old. This should be a long standing issue > > Probably not many codebases check the DISCONT flag. I only just added proper > discontinuity handling to some codebases this year, and that's why *I* noticed. > Chances are most people use the ffmpeg cli which seems to handle stuff differently, > and doesn't necessarily care about the flag. In my case, I only try to 'fix' > discontinuities if they appear to be in a format that allows them, during indexing. > > I could add a workaround specific to FLV, or some threshold stuff, but I'd prefer > that demuxers which may output discontinuous timestamps say they do. Ping. Is there a decision on eitehr what to do or to ignore this? I'll update my downstream code with an FLV edge case if need be. - Derek
On 24/12/2018 16:42, Derek Buitenhuis wrote: > Ping. Is there a decision on eitehr what to do or to ignore this? > > I'll update my downstream code with an FLV edge case if need be. Ping. - Derek
diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c index 4b9f46902b..032e466bab 100644 --- a/libavformat/flvdec.c +++ b/libavformat/flvdec.c @@ -74,7 +74,7 @@ typedef struct FLVContext { AVRational framerate; } FLVContext; -static int probe(AVProbeData *p, int live) +static int flv_probe(AVProbeData *p) { const uint8_t *d = p->buf; unsigned offset = AV_RB32(d + 5); @@ -85,22 +85,15 @@ static int probe(AVProbeData *p, int live) d[3] < 5 && d[5] == 0 && offset + 100 < p->buf_size && offset > 8) { - int is_live = !memcmp(d + offset + 40, "NGINX RTMP", 10); - if (live == is_live) - return AVPROBE_SCORE_MAX; + return AVPROBE_SCORE_MAX; } return 0; } -static int flv_probe(AVProbeData *p) -{ - return probe(p, 0); -} - static int live_flv_probe(AVProbeData *p) { - return probe(p, 1); + return 0; } static void add_keyframes_index(AVFormatContext *s) @@ -1321,6 +1314,7 @@ AVInputFormat ff_flv_demuxer = { .read_close = flv_read_close, .extensions = "flv", .priv_class = &flv_class, + .flags = AVFMT_TS_DISCONT | AVFMT_NOBINSEARCH, }; static const AVClass live_flv_class = {
Any FLV file, not necessarily valid, but in extremely common use for live or archived live purposes, may output discontinuous timestamps. As it currently is, only files produced by NGINX's RTMP module will be marked as supporting discontinuous timestamps, which is obviously untrue, and the fix was only ever put in place for a single bug report / file. The FLV demuxer, however will happily ingest any FLV, and output discontinuous timestamps, regardless of whether this "live" demuxer is used, making the current set of flags untrue. Add the flag to the main demuxer, as this is how it *actually* works. Lying to downstream API users about a demuxer's behavior is not OK. Also set AVFMT_NOBINSEARCH, as this should be true given discontinuous timetsamps. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/flvdec.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)