[FFmpeg-devel] [PATCH] make analyze_duration work for streams with incomplete timestamps (mp3)

[Michael Niedermayer]

On Mon, Sep 14, 2009 at 10:07:56AM +0200, Reimar D?ffinger wrote:
> On Mon, Sep 14, 2009 at 01:55:06AM +0200, Michael Niedermayer wrote:
> > On Sun, Sep 13, 2009 at 12:41:09PM +0200, Reimar D?ffinger wrote:
> > > currently max_analyze_duration does not work for e.g.
> > > http://208.80.52.108:80/KDLDFM because the mpeg audio parser is very
> > > thorough in making a mess of timestamps.
> > > Basically, only every 4th packet has a timestamp and thus none at all
> > > have any duration.
> > > Attached patch handles this case by using the difference between the
> > > minimum and current dts for comparing against max_analyze_duration when
> > > we have no other duration info.
> > 
> > i think your code introduces a nasty bug, namely it fails fatally in case
> > of positive timestamp discontinuities, that is the code retunrns before its done
> 
> IMO that is less "fatally" than the current code that for live streams
> like the one I gave where it _never_ returns, not after days or
> downloading GBs of data.

i dont disagree, but does it really matter if the bug or the workaround
is worse?
It should be fixed properly without introducing new issues


> 
> > also we already have the last_dts table that could be used but really i dont
> > think this is the correct solution
> 
> You may think so. I originally thought so too, but last_dts in this case
> is always the dts of the last packet, not the last valid one, so either
> last_dts or the current dts are always AV_NOPTS_VALUE and thus it is
> completely useless for this.

true, ive missed when duration_count was increased


> 
> > the correct solution IMHO is to set duration, and actually i would naively
> > expect compute_frame_duration)()/get_audio_frame_size() to do so, but it seems
> > that its not otherwise this issue wouldnt exist ...
> 
> I think the correct solution is for the parser to set pts/dts correctly,
> but I don't know how to do that and I don't want to wait for someone to
> do it (because I have the impression it is expected behaviour for
> parsers to just _SUCK_ (yes, in uppercase)).

i have more the impression people call everything they dont understand to
be broken and to _SUCK_ granted its missing any and all documentation but
still ...
the next step these people do is fix that brokenness (1, see below)

back to the topic
compute_pkt_fields() sets missing dts/pts for codecs that dont use frame
reordering based on duration, not the parser. This is quite a bit simpler
than reimplementing dts/pts advancing in the parsers, especially as that
would require a parser for most codecs ...
now iam still wondering why duration is not set ?

(1) if you fix it as you suggest by setting dts in the parser, duration
still would not be set correctly and you would then require a parser for
pretty much every codec, let me be blunt but that _SUCKS_ more

PS: i think i will regret in a moment that i didnt reword a few things
in here, ooh well iam probably in a too flameing mood

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090914/5c41a411/attachment.pgp>