[FFmpeg-devel] [PATCH 15/19] avformat/hls: do not use sequence numbers for packet ordering

[Anssi Hannula]

05.01.2014 07:17, Michael Niedermayer kirjoitti:
> On Sun, Jan 05, 2014 at 06:12:44AM +0200, Anssi Hannula wrote:
>> 05.01.2014 01:21, Michael Niedermayer kirjoitti:
>>> On Fri, Jan 03, 2014 at 04:21:39PM +0200, Anssi Hannula wrote:
>>>> As per spec 3.4.3 ("A client MUST NOT assume that segments with the same
>>>> sequence number in different Media Playlists contain matching content.")
>>>> we cannot use sequence numbers for packet ordering.
>>>>
>>>> This can be seen e.g. in the subtitle streams of
>>>> bipbop_16x9_variant.m3u8 that have considerably longer segments and
>>>> therefore different numbering.
>>>>
>>>> Since the code now exclusively syncs using timestamps that may wrap, add
>>>> some additional checking for that.
>>>>
>>>> According to the HLS spec all the timestamps should be in 33-bit MPEG
>>>> format and synced together.
>>>>
>>>> After this commit (and the preceding commits) HLS WebVTT subtitles
>>>> should work properly (ticket #2833).
>>>>
>>>> v2: cleaner wrap detection with av_compare_mod()
>>>>
>>>> Signed-off-by: Anssi Hannula <anssi.hannula at iki.fi>
>>>> ---
>>>>
>>>> I still skipped pts_wrap_bits scaling since we are not supposed to
>>>> need it as per comment.
>>>>
>>>>  libavformat/hls.c | 30 ++++++++++++++++++------------
>>>>  1 file changed, 18 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>>> index 16e5799..e299cb3 100644
>>>> --- a/libavformat/hls.c
>>>> +++ b/libavformat/hls.c
>>>> @@ -1235,6 +1235,17 @@ static int reopen_subtitle_playlist(HLSContext *c, struct playlist *pls)
>>>>      return ret;
>>>>  }
>>>>  
>>>> +static int compare_ts_with_wrapdetect(int64_t ts_a, AVStream *st_a,
>>>> +                                      int64_t ts_b, AVStream *st_b)
>>>> +{
>>>> +    int64_t scaled_ts_b = av_rescale_q(ts_b, st_b->time_base, st_a->time_base);
>>>> +
>>>> +    /* NOTE: We do not bother to scale pts_wrap_bits here. All the allowed
>>>> +     * formats specified in HLS specification have 33-bit MPEG timestamps
>>>> +     * (which means we should not even have to perform any rescaling...). */
>>>> +    return av_compare_mod(ts_a, scaled_ts_b, 1LL << (FFMIN(st_a->pts_wrap_bits, st_b->pts_wrap_bits)));
>>>
>>> i dont think this works in all cases
>>>
>>> assume theres a mpeg timebase of 90khz going from 0..2^33
>>> and one thats lets randomly pick based on 32khz
>>>
>>> if the 32khz one is scaled to the 90khz one and the 33bits are used
>>> then it could work. (assuming HLS wraps it when the rescaled to 90khz
>>> would wrap)
>>> But if its done the other way around
>>> 0..2^33 * 32000 / 90000
>>> then the wrap around is nowhere near a power of 2
>>
>> Yes, that is not handled. That was what I meant with the comment but I
>> wasn't too clear. Will improve it :)
>>
>>> and when one assumes that timebases different from 90khz can occur
>>> then i guess 2 could occur together and if they wrap at 33bits
>>> when rescaled to 90khz then they wont neccesarily wrap at a power of
>>> 2 either even if they both would have the same timebase
>>
>> Yep.
>>
>>> btw, are there samples where non 90khz based timestamps wrap ?
>>
>> I don't know of any samples that have non-90kHz timestamps, period.
>> It would be indirectly against the spec (since spec allows mpegts,
>> hls_id3_audio, webvtt with timestamp_map, all of which have 90kHz).
>>
>> Do you think it is still worth it to add code to handle those two cases
>> you mentioned? I'm not really against it...
> 
> does it require more than scaling both to 90khz ?

Well, actually that is enough assuming they only wrap at 33-bit 90kHz,
so I guess I'll add that.

I was thinking of other cases, but as you previously noted those can't
really be handled this way anyway...

>> All the scaling code was previously needed because before hls_id3_audio
>> such streams were handled as raw with non-90kHz generated timestamps,
>> but they are probably not needed now.
>>
>> I've kept the scaling code around just in case there are some weird
>> streams, as I don't actually have a too large amount of HLS samples
>> myself... Maybe a "request sample/link" warning could/should be added
>> for those cases.
>>
>> -- 
>> Anssi Hannula
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 


-- 
Anssi Hannula