[MPlayer-dev-eng] [PATCH] Better way to calculate current audio pts

[Corey Hickey]

Uoti Urpala wrote:
> MPlayer calculates current audio pts by taking the last known pts in
> packets read by the audio decoder and adding bytes read after that
> divided by decoder input bitrate. This method is inaccurate by design
> because the input bitrate need not be constant, and even when it is
> constant the correct value is not always known. This patch adds a more
> accurate method where the audio decoder updates the last known pts value
> and decoded bytes written after the pts position, and adds support for
> this method to the libvorbis, faad and ffmpeg audio decoders. Combined
> with the earlier patches I've posted this finally makes the av sync
> calculations accurate enough; on my system the sync value shown on the
> status line mostly stays below 2 ms.
> 
> The patch assumes my earlier patches have already been applied. As
> mentioned in the comments the definitions of MP_NOPTS_VALUE should come
> from a header file, but as noted by Michael when he added the definition
> to libmpcodecs/vf.h there currently doesn't seem to be a good header for
> miscellaneous defines that should be visible in most other files.

Ok, it's been a few days since I applied your previous A-V sync patch 
and there haven't been any users hollering, so it's probably time to 
start looking at this one.

I had a look through the code. There are others here who could review it 
better but they don't seem to be interested yet.

> +// Change this once MP_NOPTS_VALUE is moved to a better header
> +#define MP_NOPTS_VALUE (-1LL<<63)

Does somebody have this planned? I see it defined in two places already 
(libmpcodecs/vf.h and libmpdemux/muxer.h). I don't know if it can be 
moved to a top-level header (each lib* directory should theoretically be 
autonomous, or so I heard), but there must be a better way than 
#defining the same value in several files which live in one of only two 
directories.

Creating libmpdemux/mpdemux.h and libmpcodecs/mpcodecs.h seems a bit 
silly for one macro, but there may be other stuff that would be better 
suited for such files.

...anyone?

> +int ds_get_packet_pts(demux_stream_t *ds,unsigned char **start, double *pts)
> +{
> +    int len;
> +    *pts = MP_NOPTS_VALUE;
> +    if(ds->buffer_pos>=ds->buffer_size){
> +	if (!ds_fill_buffer(ds)) {
> +            // EOF
> +            *start = NULL;
> +            return -1;
> +	}
> +	// Should use MP_NOPTS_VALUE for "unknown pts" in the packets too
> +	if (ds->current->pts)
> +	    *pts = ds->current->pts;
> +    }
> +    len=ds->buffer_size-ds->buffer_pos;
> +    *start = &ds->buffer[ds->buffer_pos];
> +    ds->buffer_pos+=len;
> +    return len;
> +}

This looks similar to yet different from ds_get_packet(). Would it be 
practical and usable to merge the functionality of your 
ds_get_packet_pts() into the original?

The original ds_get_packet() and ds_get_packet_sub() have everything in 
a 'while(1)'. What's the reason for that? There's no 'continue' and some 
kind of a 'return' is unconditional. I know that's not part of your 
patch, but if someone could explain that to me I'd be grateful.

-Corey