[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

[Michael Niedermayer]

On Tue, Feb 24, 2009 at 09:38:55PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> On Tue, Feb 24, 2009 at 08:20:53PM +0100, Ivan Schreter wrote:
>>   [...]
>>> One problem remains, though: FPS detection doesn't take field pictures 
>>> into account. So if the format returns frames containing field pictures 
>>> with 1/2 frame duration (or better to say, with DTS distance of 1/2 frame 
>>> duration), then it will detect 2*fps instead of correct fps.
>>>     
>>
>> yes
>>
>>
>>   
>>> I'd propose adding a flag field_picture_flag to AVPacket (and pass it 
>>> from the parser) to notify the user about field pictures, so it can 
>>> correctly compute frame rate. What do you think? Or do you have a better 
>>> idea?
>>>     
>>
>> i dont know, it doesnt seem to be such a big issue to me to have 2*fps ...
>>
>>   
> It is for the user. See, I want to convert a H.264 interlaced video to some 
> other format. So I'd call ffmpeg -i h264.mts -vcodec mpeg2video-acodec mp2 
> -f vob h264.vob. What will this do? It will create a vob with 2*fps and 
> each picture duplicated, which is definitely not what we want... True, the 
> user could specify -r 25 to force frame rate to 25fps, but it's not 
> logical. So IMHO we need this. Agreed?

yes, there is a problem


[...]
>> [...]
>>
>>   
>>> @@ -3150,6 +3147,47 @@
>>>       * subtitles are correctly displayed after seeking.
>>>       */
>>>      int64_t convergence_duration;
>>> +
>>> +    // Timestamp generation support:
>>> +    /**
>>> +     * Synchronization point for start of timestamp generation.
>>> +     *
>>> +     * Set to >0 for sync point, 0 for no sync point and <0 for 
>>> undefined
>>> +     * (default).
>>> +     *
>>> +     * For example, this corresponds to presence of H.264 buffering 
>>> period
>>> +     * SEI message.
>>> +     */
>>> +    int dts_sync_point;
>>>     
>>
>> unneeded, the other 2 can just be set to AV_NOPTS_VALUE when unavailable
>> (they need to be 64bit then though)
>>   
> Not really. If used, the two others are _always_ available (even if it 
> wouldn't be required, there could be and there is a file where they are). 
> dts_ref_dts_delta contains delta to reference DTS. Reference DTS changes at 
> the beginning of buffering period. So the only way would be to detect new 

right, sorry ive misunderstood the H264 spec



> buffering period, if we take one frame in the future and look at 
> dts_ref_dts_delta there. If it's less than current dts_ref_dts_delta, then 
> current frame is start of new buffering period.
>

> Since we don't have a crystal ball to look into the future :-), it has to 
> be done differently, therefore the flag to indicate start of new buffering 
> period. Note that delta for first frame of new buffering period refers to 
> the _old_ buffering period (i.e., old reference DTS).

yes


>
> Additionally, on and around buffering period change, we potentially have no 
> DTS from the container. So we cannot use something you suggested below as 
> well.
>
> Clear?

yes

[...]
>>> +            if (pkt->dts != AV_NOPTS_VALUE) {
>>> +                // got DTS from the stream, update reference timestamp
>>> +                st->reference_dts = pkt->dts - pc->dts_ref_dts_delta * 
>>> num / den;
>>>     
>>
>> av_rescale_q() should simplify this and similar code.
>>
>>   
> True, but then I'd have to convert a scalar value to an AVRational, which 
> involves copy into memory and more complex/slower code. And I doubt it 
> would really simplify it optically in this particular case. So I decided 
> that for this particular case, it's better without.

nope, no its just:
av_rescale_q(pc->dts_ref_dts_delta, st->codec->time_base, st->time_base.num)
no AVRational convertions ...

anyway i think these are no reason to delay the patch, so both patchs ok
the 2 lines can be fixed in a seperate patch

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20090224/95df276e/attachment.pgp>