[MPlayer-dev-eng] [PATCH] Timestamp-based mpg seeking

[Aurelien Jacobs]

On Tue, 12 Oct 2004 13:06:17 +0200
Michael Behrisch <behrisch at informatik.hu-berlin.de> wrote:

> On Mon, Oct 04, 2004 at 12:07:16PM +0200, Michael Behrisch wrote:
> > This is "proof of concept"-patch which enables seeking based
> > on the largest timestamp in an mpg stream. It is often more 
> > accurate than the current seeking and it has the additional 
> > benefit of giving the (almost) precise total time of the movie.
> > 
> > It includes a guess on whether the timestamps are correct.
> > 
> > This it not ready to be included beacuse it does not free 
> > all the memory allocated, it is not documented and tested
> > with three files only. But beforing doing more work,
> > I wanted to know whether a patch like that might get accepted.
> > If so, I will fix the issues mentioned and improve seeking
> > further with some kind of "iterative search" just as in demux_ogg.
> > 
> 
> No answer for eight days maybe it needs some more advertisement:
> 
> This patch will reduce the number of "-ss does not work"
> and "dvd length is wrong" messages by more than 80% ;-)!
> It enables indeed precise seeking and retrieves the total
> time of the movie almost always correctly (and it can also 
> walk your dog).

:-)

> Please have at least a look at it.
> It's now far more advanced than the last version
> (perhaps only the doxygen is missing)

I've tested it with some mpeg files and DVDs. It works very well !
I think this is a real improvement, and something like this should
be commited.

About the code:

+/// Open an mpg physical stream
+int demux_mpg_open(demuxer_t* demuxer) {
+  off_t pos = stream_tell(demuxer->stream);
+  if (!ds_fill_buffer(demuxer->video)) return 0;
+  mpg_demuxer_t* mpg_d = (mpg_demuxer_t*)calloc(1,size...

This is generally a bad idea to declare variables in the middle
of a function. gcc-2.95 don't accept this.

-
+    mpg_demuxer_t *mpg_d=(mpg_demuxer_t*)demuxer->priv;
+    int precision = 1;
+    float oldpts = mpg_d->last_pts;
+    off_t oldpos = demuxer->filepos;
+    

Removing a blank line to add a blank line with one tab is cosmetics.
Please remove this.

     off_t newpos=(flags&1)?demuxer->movi_start:demuxer->filepos;
-	
+
     if(flags&2){

Another cosmetic.

     }
-
+    
+    while (1) {

And still another one !

Apart that, this patch seem clean enough.
Unfortunatly, I don't know demux_mpg enough to be sure this is the
right way to do it, and that it won't break anything.
Someone who knows demux_mpg well shoud really have a look at this !

Aurel