[FFmpeg-devel] [PATCH] RTMP seek support

[Howard Chu]

Michael Niedermayer wrote:
> On Tue, Mar 30, 2010 at 06:42:50PM -0700, Howard Chu wrote:
>> Howard Chu wrote:
>>> Howard Chu wrote:
>>>> Howard Chu wrote:
>>>>> Howard Chu wrote:
>>>>>> This is a first stab at getting seek working on an RTMP stream.
>>
>>>>>> I've been testing with this stream:
>>>>>>
>>>>>> gdb --args ./ffplay_g
>>>>>> "rtmp://fms.scctv.net/annenberg//vod/world_of_chemistry_01
>>>>>> swfurl=http://www.learner.org/vod/swfs/player-licensed.swf
>>>>>> pageurl=http://www.learner.org/vod/vod_window.html?pid=793"
>>>>>
>>>>> Thanks to a tip from uau this is now working.
>>
>>> Note that the did_rseek flag wouldn't even be needed if the rest of flvdec
>>> always read from the start of a frame, instead of reading from the end of
>>> the
>>> previous frame, as it currently does. But I took a look at fixing that and
>>> the
>>> diff would be pretty large, so I chose this approach instead.
>>
>> It turned out to be not too bad to correct that, as this patch shows.
>>
>>> The patch for aviobuf.c is going to be needed regardless - you shouldn't
>>> be
>>> setting "s->pos = s->seek()" unconditionally, since s->seek may not be
>>> implemented. Storing a -32 in s->pos is bound to screw something else up.
>>
>> --
>>    -- Howard Chu
>>    CTO, Symas Corp.           http://www.symas.com
>>    Director, Highland Sun     http://highlandsun.com/hyc/
>>    Chief Architect, OpenLDAP  http://www.openldap.org/project/
>
>> Index: libavformat/flvdec.c
>> ===================================================================
>> --- libavformat/flvdec.c	(revision 22713)
>> +++ libavformat/flvdec.c	(working copy)
>> @@ -270,6 +270,7 @@
>>
>>       offset = get_be32(s->pb);
>>       url_fseek(s->pb, offset, SEEK_SET);
>> +    url_fskip(s->pb, 4);
>>
>>       s->start_time = 0;
>>
>> @@ -295,9 +296,8 @@
>>       int64_t dts, pts = AV_NOPTS_VALUE;
>>       AVStream *st = NULL;
>>
>> - for(;;){
>> + do{
>>       pos = url_ftell(s->pb);
>> -    url_fskip(s->pb, 4); /* size of previous packet */
>>       type = get_byte(s->pb);
>>       size = get_be24(s->pb);
>>       dts = get_be24(s->pb);
>> @@ -359,7 +359,7 @@
>>       if ((flags&  FLV_VIDEO_FRAMETYPE_MASK) == FLV_FRAME_KEY)
>>           av_add_index_entry(st, pos, dts, size, 0, AVINDEX_KEYFRAME);
>>       break;
>
>> - }
>> + } while(url_fskip(s->pb, 4),1);
>>
>
> putting the url_fskip() inside the loop at the end seems cleaner to me

Then the couple "continue"s inside the loop would need to be changed (e.g., to 
"goto end_of_loop"). I usually opt for the patch that touches the least amount 
of existing code and adds the least new code.

> also the url_fskip() movement could be a seperate patch

I guess you're right. At first it seemed to me that would also mean patching 
the flv_read_seek functionality twice, but now I see it could have been split 
out. I'll re-send as a separate patch.

> [...]
>> +static int flv_read_seek(AVFormatContext *s, int stream_index,
>> +    int64_t ts, int flags)
>> +{
>> +    int ret = AVERROR_NOTSUPP;
>> +
>> +    if (url_is_streamed(s->pb))
>> +        ret = av_url_read_fseek(s->pb, stream_index, ts, flags);
>> +    return ret;
>> +}
>
> what if we get the flv from some other non seekable place like a pipe?

Then it will just return AVERROR_NOTSUPP, which is what av_url_read_fseek() 
returns when ByteIOContext->read_seek is not implemented.

>> +
>>   AVInputFormat flv_demuxer = {
>>       "flv",
>>       NULL_IF_CONFIG_SMALL("FLV format"),
>> @@ -449,6 +464,7 @@
>>       flv_probe,
>>       flv_read_header,
>>       flv_read_packet,
>> +    .read_seek = flv_read_seek,
>>       .extensions = "flv",
>>       .value = CODEC_ID_FLV1,
>>   };
>> Index: libavformat/librtmp.c
>> ===================================================================
>> --- libavformat/librtmp.c	(revision 22713)
>> +++ libavformat/librtmp.c	(working copy)
>> @@ -127,7 +127,9 @@
>>           return AVERROR_NOTSUPP;
>>
>>       /* seeks are in milliseconds */
>> -    timestamp = av_rescale(timestamp, AV_TIME_BASE, 1000);
>> +    if (stream_index == -1)
>> +        timestamp = av_rescale(timestamp, 1000, AV_TIME_BASE);
>> +
>>       if (!RTMP_SendSeek(r, timestamp))
>>           return -1;
>>       return timestamp;
>
>> Index: libavformat/aviobuf.c
>> ===================================================================
>> --- libavformat/aviobuf.c	(revision 22713)
>> +++ libavformat/aviobuf.c	(working copy)
>> @@ -698,8 +698,11 @@
>>           return AVERROR(ENOSYS);
>>       ret = s->read_seek(h, stream_index, timestamp, flags);
>>       if(ret>= 0) {
>> +        int64_t pos;
>>           s->buf_ptr = s->buf_end; // Flush buffer
>> -        s->pos = s->seek(h, 0, SEEK_CUR);
>> +        pos = s->seek(h, 0, SEEK_CUR);
>> +        if (pos != AVERROR(ENOSYS))
>> +            s->pos = pos;
>>       }
>>       return ret;
>>   }
>
> shouldnt this return some kind of error if seek fails?

I don't know, you tell me. It obviously never checked for errors before. It 
doesn't seem like this particular seek call can actually accomplish anything; 
since it's referencing (0, SEEK_CUR) then if the underlying layers already 
know where SEEK_CUR is, it's a no-op. And if the underlying layers don't know 
where SEEK_CUR is, this isn't going to help them determine it.

And again, this should probably be a separate patch. Should I split it out?

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/