[FFmpeg-devel,2/3] flvdec: Mark the demuxer as allowing discontinuous timestamps

Submitted by Derek Buitenhuis on Nov. 21, 2018, 3:58 p.m.

Details

Message ID 20181121155848.201788-3-derek.buitenhuis@gmail.com
State New
Headers show

Commit Message

Derek Buitenhuis Nov. 21, 2018, 3:58 p.m.
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(-)

Comments

Michael Niedermayer Nov. 22, 2018, 2:16 a.m.
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

[...]
Derek Buitenhuis Nov. 22, 2018, 10:27 a.m.
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
Carl Eugen Hoyos Nov. 22, 2018, 12:55 p.m.
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
Derek Buitenhuis Nov. 22, 2018, 5:03 p.m.
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
Jan Ekström Nov. 22, 2018, 5:18 p.m.
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
Michael Niedermayer Nov. 23, 2018, 2:12 a.m.
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?

[...]
Michael Niedermayer Nov. 23, 2018, 2:16 a.m.
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

[...]
Derek Buitenhuis Nov. 26, 2018, 1:51 p.m.
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
Derek Buitenhuis Dec. 24, 2018, 4:42 p.m.
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
Derek Buitenhuis Jan. 2, 2019, 10 p.m.
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

Patch hide | download patch | download mbox

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 = {