Created attachment 189525 [details] [review]
Correct patch

Correct patch, please ignore the previous one.

baseparse does interpolate timestamps based on frame duration [*]

Other than some coding style issues and the fact that priv->frame_duration need not always be set (which is not considered here), the suggested interpolation is a somewhat hack-ish one that may work for some cases (e.g. note that fsize need not be constant), but not for all that baseparse has to cover.

In essence, any downstream element (be it baseparse based) sort-of has to trust upstream timestamps and can only go so far in fiddling with those.
As such, timestamps would basically have to be fixed upstream.

In this particular case, lame(mp3) likely has a hard time in doing that correctly in the first place since a.o.o encoder does not return whole frames (iirc).  Inserting a helper element before the parser that invalidates timestamps might/should help (as that then gives downstream parser a chance to come up with its own ones).

[*] that is, if an incoming buffer holds several frames, each outgoing frame will have different timestamps derived/interpolated by previous frame duration

So the problem here, if I understand correctly is that lame encoder returns incomplete frames (for now ignoring that it also returns wrong timestamps, opened another bug for that)? I was under impression that I should be able to use mpegaudioparser to correct that, but now it looks like it doesn't cope with incomplete frames on the input "by design"?

Also please note that even though there are incomplete frames in the input, the timestamps are correct. However after the data is processed by mpegaudioparse, the timestamps on output are wrong. So perhaps there should be mentioned at least somewhere what exactly the parser expects in the input to work.

In as much as (I feel that) downstream can only 'correct' (rather than
complement/supplement) upstream that far, yes.  And it is a bit open as to how 'correct' a timestamp can be for a piece of a frame.

Also, the size-to-time == bitrate relation assumed by the calculation need not
be valid for all possible types of 'frame' and as such restrict what baseparse
can do [*]


[*] it might be argued that baseparse does actually estimate a bitrate and uses
this one for (approximate) position tracking etc fairly generally, so one might
then use this bitrate in stead to perform some interpolation on partial frame.
However, that would introduce even more 'approximation' into baseparse, and no
real assurance how well that works (in combination with possibly bogus
upstream), so such behaviour should probably be optional (controlled by some
property).  Only the interpolation performed currently (and described earlier)
can really be exact.

Well, I've only tested it with MP3 frames where the size-to-time/bitrate assumptions kinda works, at least for CBR. Perhaps (if at all) this should be handled by mpegaudioparse rather than baseparser. On second thought maybe the real issue here is Lame output not being aligned to frames.

Why does lame generate incomplete frames?  That seems like a good way to destroy timing information.  IMO, that's a bug all by itself.

Well, the lame plugin doesn't attempt to generate complete frames, it just sends to output whatever the encoder gives it. 

I've submitted a report for lame encoder generating incorrect timestamps, but now I think that the problem is that the encoder should just output entire frames.

https://bugzilla.gnome.org/show_bug.cgi?id=652150