[FFmpeg-devel] [PATCHv2 2/3] avformat/utils: increase detected start_time with skip_samples

[Marton Balint]

On Wed, 9 Mar 2016, wm4 wrote:
> On Tue, 8 Mar 2016 23:44:13 +0100 (CET)
> Marton Balint <cus at passwd.hu> wrote:
>
>> On Tue, 8 Mar 2016, Hendrik Leppkes wrote:
>> 
>> > On Tue, Mar 8, 2016 at 10:54 PM, Marton Balint <cus at passwd.hu> wrote: 
>> >>
>> >> On Tue, 8 Mar 2016, wm4 wrote:
>> >> 
>> >>> On Tue,  8 Mar 2016 21:29:52 +0100
>> >>> Marton Balint <cus at passwd.hu> wrote:
>> >>> 
>> >>>> Signed-off-by: Marton Balint <cus at passwd.hu>
>> >>>> ---
>> >>>>  libavformat/utils.c               | 10 ++++--
>> >>>>  tests/ref/fate/gapless2-ipod-aac1 | 72
>> >>>> +++++++++++++++++++--------------------
>> >>>>  tests/ref/fate/gapless2-ipod-aac2 | 72
>> >>>> +++++++++++++++++++--------------------
>> >>>>  3 files changed, 80 insertions(+), 74 deletions(-)
>> >>>> 
>> >> 
>> >>> I'm a probably bit late here, but what's the rationale of increasing the
>> >>> start time?
>> >>> 
>> >>
>> >> According to docs, start time is supposed to be the pts of the first
>> >> decoded frame, therefore skipped samples must be taken into account, when
>> >> the start time is determined based on the first packet pts.
>> >> 
>> >
>> > But the skipping is performed by avcodec, not avformat, isn't it? 
>> 
>> Yes.
>> 
>> > start_time should be the PTS of the first avpacket coming out of
>> > avformat, never mind what a decoder might do to that later. 
>> 
>> Not according to the docs:
>> 
>> "AVStream->start_time: decoding: pts of the first frame of the stream in 
>> presentation order, in stream time base. Only set this if you are 
>> absolutely 100% sure that the value you set it to really is the pts of 
>> the first frame."
>> 
>> If here frame refers to a packet, why the docs is talking about 
>> presentation order?
>> 
>> Also check the libavformat/mp3dec.c, it does the same kind of start_time 
>> adjustment based on the skipped samples.
>
> True, with mp3, the first PTS is always 0, and after skipping it will
> be some positive value, and start_time is set to the skip duration. Is
> this the same here?

Yes. These are iTunes (-compatible) m4a files by the way with iTunes 
gapless metadata. I don't think you can make any assumptions based on 
a negative timestamp, even for MOV, elst should contain the proper start 
time:
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFAppenG/QTFFAppenG.html

> Also, what the mp3 demuxer does is not necessarily correct. And
> also, what works for audio-only formats isn't necessarily right for if
> there are video tracks involved.

Only audio stream start times are adjusted, I don't see why does it make 
a difference if there are other (video) streams.

> What I'm really worried about is what utils.c will determine as total 
> stream start time?

What do you mean by "total stream start time"? Are you referring to 
AVFormat->start_time? Isn't that simply the minimum of the stream start 
times?

> From your added tests it looks like mov.c does now what I'd expect,
> though. I just want to be sure this mess doesn't become worse.
>

Sure, ok.

> Last but not least, why do we have about 3 mechanisms to signal
> pre-skip? (Skip side data, skip_samples field, and delay field.)

As far as I see, avformat code injects skip side data based on the
AVStream->skip_samples field, avcodec code uses skip side data to 
skip the samples.

I guess the delay field is the old way of signalling the 
number of to-be-skipped frames. As this can only signal whole frames and 
not samples, it has a very limited use. Generic code does not do anything 
with this.

Regards,
Marton