[FFmpeg-devel] [PATCH] OGG seeking and timestamp

[Stefano Sabatini]

On date Wednesday 2008-11-19 10:43:23 +0100, Yvan Labadie encoded:
> Hi again,
>
> I needed to recover exact packets, so on the first read I save all  
> packets with available timestamp value, then when I need I make an  
> av_seek_frame to these values in order to recover the corresponding  
> packet, but sometimes it worked and sometimes it sent me an other packet  
> (which is the next packet in fact).
>
> and worst, sometimes it was simply impossible to seek to this same  
> timestamp, it returned the packet of the next timestamp :s
>
> So I digg in... and I made a modification so when we make a seek to a  
> saved timestamp we always recover the same packet as the one which  
> generated this timestamp!
>
> I join a patch if it can help!
>
> NB : Sorry for the mail in libav* user I had some problems with my email  
> and  ffmpeg-devel subscription...

> diff -cr ffmpeg-export-2008-10-10/libavformat/oggdec.c ffmpeg-export-2008-10-10_modif/libavformat/oggdec.c

Ouch! Use diff -u patches, as used by svn diff, also you should use
just that to ease your work.

> *** ffmpeg-export-2008-10-10/libavformat/oggdec.c	2008-10-03 12:16:29.000000000 +0200
> --- ffmpeg-export-2008-10-10_modif/libavformat/oggdec.c	2008-11-18 17:36:37.000000000 +0100
> ***************
> *** 119,124 ****
> --- 119,125 ----
>           os->lastgp = -1;
>           os->nsegs = 0;
>           os->segp = 0;
> + 		os->timeStamped = 0;

Weird indent, timeStamped doesn't look like a good name, also
use_time_stamp style is preferred.

>       }
>   
>       ogg->curidx = -1;
> ***************
> *** 291,297 ****
>       os->lastgp = os->granule;
>       os->bufpos += size;
>       os->granule = gp;
> !     os->flags = flags;
>   
>       if (str)
>           *str = idx;
> --- 292,299 ----
>       os->lastgp = os->granule;
>       os->bufpos += size;
>       os->granule = gp;
> !     os->flags = flags;;
> ! 	os->timeStamped=-1; //not yet timestamped, waiting for first full packet
>   
>       if (str)
>           *str = idx;
> ***************
> *** 320,326 ****
>                   return -1;
>           }
>   
> !         os = ogg->streams + idx;
>   
>   #if 0
>           av_log (s, AV_LOG_DEBUG,
> --- 322,331 ----
>                   return -1;
>           }
>   
> !         os = ogg->streams + idx;;
> ! 		if ((os->timeStamped==-1) && (!(os->flags&0x01) || (os->flags&0x01 && os->segp > 0) ) ){
> ! 			os->timeStamped=0; //this is the packet wich need to be timestamped!
> ! 		}		

Weird indent.
   
>   #if 0
>           av_log (s, AV_LOG_DEBUG,
> ***************
> *** 513,521 ****
>           return AVERROR(EIO);
>       pkt->stream_index = idx;
>       memcpy (pkt->data, os->buf + pstart, psize);
> !     if (os->lastgp != -1LL){
>           pkt->pts = ogg_gptopts (s, idx, os->lastgp);
> !         os->lastgp = -1;
>       }
>   
>       pkt->flags = os->pflags;
> --- 518,526 ----
>           return AVERROR(EIO);
>       pkt->stream_index = idx;
>       memcpy (pkt->data, os->buf + pstart, psize);
> ! 	if (os->timeStamped == 0){
>           pkt->pts = ogg_gptopts (s, idx, os->lastgp);
> !         os->timeStamped = 1; //page timestamped
>       }
>   
>       pkt->flags = os->pflags;
> ***************
> *** 552,559 ****
>           if (ogg->streams[i].granule != -1 && ogg->streams[i].granule != 0 &&
>               ogg->streams[i].codec && i == stream_index) {
>               pts = ogg_gptopts(s, i, ogg->streams[i].granule);
> !             // FIXME: this is the position of the packet after the one with above
> !             // pts.
>               *pos_arg = url_ftell(bc);
>               break;
>           }
> --- 557,584 ----
>           if (ogg->streams[i].granule != -1 && ogg->streams[i].granule != 0 &&
>               ogg->streams[i].codec && i == stream_index) {
>               pts = ogg_gptopts(s, i, ogg->streams[i].granule);
> !             // FIXME: this is the position of the packet after the one with above pts.
> ! 			// 		==> if we got a page type "fresh packet" so the firts packet is full (I mean present from its begining) then we will have the real first packet of this page sequence
> ! 			//		==> but if we have a page type "continued packet" then we will have the second packet (because it's too complicated to go recover the begining of the first packet in the previous page)
> ! 			// 	
> ! 			//	Problem is:
> ! 			//		we read a packet with a timestamp, we save this timestamp for future seek
> ! 			//		we want to recover the packet, so we make a seek->timestamp
> ! 			//	  first case:
> ! 			//		it was a packet with the begining on this frame ("fresh packet") 
> ! 			//		then it will return the first FULL packet which is the real first packet of the frame
> ! 			//	  second case:
> ! 			//		it was a packet with the begining on the previous frame ("continued packet") 
> ! 			//		then we cannot recover the begining in the previous frame 
> ! 			//		so it will return the first FULL packet in that frame which is the second packet of the frame!
> ! 			//		And so we will not recover the same packet than the one which gave us the timestamp!
> ! 			// 
> ! 			//	To manage this problem, we always apply a timestamp to the second packet 
> ! 			//	and av_seek_frame->timestamp always put the pointer on the second packet
> ! 			//	so we have a predictible behaviour 
> ! 			//		=> 	when av_read_frame return a timestamp it is the second packet for sure 
> ! 			//			and av_seek_frame will always return the same packet as the one which gave us the timestamp!
> ! 			

Again weird indent.

Regards.
-- 
FFmpeg = Fiendish & Forgiving Minimal Portentous Encoding/decoding Gadget