[FFmpeg-devel] [PATCH] MPEG-TS/-PS : better probing for duration and start-time for all streams in a container

[Gaullier Nicolas]

>> It appears that, in the fate-suite, mpeg2/mpeg2_field_encoding.ts is somewhat broken.
>> This sample was certainly extracted by means of a simple binary cut. 
>> It is common practice with TS, so there is nothing really "wrong" with 
>> it, but the fact is that the first broken packets were considered valid in the current implementation.
>> Besides, since there is no "B" frames (only I/P in this stream), the following fallback rule apply:
>> [in utils.c/update_initial_durations]
>> if (!st->codec->has_b_frames)
>>                 pktl->pkt.pts = cur_dts; At the end, a corrupted 
>> (truncated) packet is assigned a valid PTS value, and the new 
>> algorithm of the patch takes it into account, leading to a wrong estimation of the start_time (and fate breaks).
>> To resolve this, I think the cleanest solution is to consider the 
>> first truncated packets in the stream as non-valid. I thus propose here a patch in the avcodec/mpeg layer to impose detection of a picture_start_code.
>> That way, it is not possible to assign a PTS where there is no picture.
>
>this wont work, mpeg1/2 in mpeg-ps/ts is not the only case that can be truncated and parsers should not discard data, at least not unless the users tells the parser by some means to >discard data
>
>also keep in mind, the fate checksum can be updated if the new result makes more sense than the old.
>I really dont know what the last timestamp should be for a file with a truncated frame at the end.
>
>
>> 
>> Second thing, there was another break in FATE, because of the CAVS sample (cavs.mpg).
>> It appears that in this sample, most of the PES come with no PTS/DTS. 
>> I am not aware of CAVS and suppose it is normal, but at this end, this must be taken into account in avutils.c/compute_pkt_fields.
>> I discovered there was already a onein_oneout defined, so I just appended CAVS.
>> There is another onein_oneout in 'select_from_pts_buffer' where CAVS 
>> could possibly be appended too, but I really do not know anything about CAVS, so I just made the minimum change to have FATE pass.
>
>the mpeg specs dont require PTS/DTS on all PES packets, onein_oneout is about the relation betwee tne DTS and PTS, not if some are not coded.
>what onein_oneout basically means is that when a decoder is feeded with a packet at its DTS the decoder outputs and presents at the same instant a single frame. This also implies if you >sort PTS they are equal to the DTS. I do not know if this is true for CAVS or not.
>So i cant say if this change is correct, but it disables PTS/DTS interpolation for cavs which isnt good unless it has to be done
>

I will lack time to complete the start_time enhancement because of everything that is involved.
Finally, I decided to split the patch to keep only the duration update.
For the record, it is to replace ff_read_packet that returns packet with "uncomputed" zero duration,
and a reset for last_dts to disable a non relevant warning due to the retry process.
(FATE pass ok)

Nicolas

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 723eff6..94c68d5 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -2511,12 +2511,13 @@ static void estimate_timings_from_pts(AVFormatContext *ic, int64_t old_offset)
 
         avio_seek(ic->pb, offset, SEEK_SET);
         read_size = 0;
+        st->last_dts_for_order_check=0;
         for (;;) {
             if (read_size >= DURATION_MAX_READ_SIZE << (FFMAX(retry - 1, 0)))
                 break;
 
             do {
-                ret = ff_read_packet(ic, pkt);
+                ret = read_frame_internal(ic,pkt);
             } while (ret == AVERROR(EAGAIN));
             if (ret != 0)
                 break;