[MPlayer-dev-eng] [rff:patch] streamposition & length with demuxer_control

[Arpi]

Hi,

> What doesn't work, and I don't know how it should be done:
> - - length of mpg without bitrate is just guessed
> - - length of asf wma/video - the packet rate is different, but how should it 
> be found automatically?
no way to get these - this is why i said it must be optional

> +	case DEMUXER_CTRL_GET_PERCENT_LENGTH:
> +	    *((unsigned long *)arg)=100;
> +	    return DEMUXER_CTRL_OK;
what's the sense of length in percents? it's always 100, isn't it?

> +	case DEMUXER_CTRL_GET_PERCENT_POS:
> +	    if (demuxer->movi_end==demuxer->movi_start) *((unsigned long *)arg)=0; 
> +    	    else *((int *)arg)=(int)(demuxer->filepos/((demuxer->movi_end-demuxer->movi_start)/100));
> +	    return DEMUXER_CTRL_OK;
this is bad
return DEMUXER_CTRL_UNKNOWN if you don't know the value, but no zero.
so the caller will know if the info it gets is reliable or not!
so, return DEMUXER_CTRL_OK only if you could calculate the requested value.

> +#define NUMBEROFFRAMES   (int)(((demuxer->movi_end-demuxer->movi_start- \
> +				priv->idx_size*8-sh_audio->audio.dwLength)/ \
> +				(float)sh_video->i_bps)*(float)sh_video->fps) 

heh
this is totaly wrong.
btw the AVI file has the number of total frames in the header, in
sh_video->video.dwlength

but it may be zero, if unknown (rare)

> +	case DEMUXER_CTRL_GET_PERCENT_POS:
> +	    *((int *)arg)=(int)(d_video->pack_no*100/NUMBEROFFRAMES);

priv->video_pack_no is better for this

> +	case DEMUXER_CTRL_GET_TIME_LENGTH:
> +	    if(!sh_video->i_bps) // unspecified or VBR {
> +		*((unsigned long *)arg)=(demuxer->movi_end-demuxer->movi_start)/174300; // 174.3 kbyte/sec
> +	    else
> +        	*((unsigned long *)arg)=(demuxer->movi_end-demuxer->movi_start)/sh_video->i_bps;
> +	    return DEMUXER_CTRL_OK;

maybe we should introduce DEMUXER_CTRL_ACCURATE and DEMUXER_CTRL_INACCURATE
as return value. so demuxers operating with bytepos+bitrate could return
INACCURATE, while ones knowing teh exact timestamp or frame number could
return ACCURATE value.

> +	case DEMUXER_CTRL_GET_PERCENT_POS:
> +	    if (demuxer->movi_end==demuxer->movi_start) *((int *)arg)=0;
again
don't return false (0) value with _OK

> +// DEMUXER control commands/answers
> +#define DEMUXER_CTRL_OK 0
> +#define DEMUXER_CTRL_NOTIMPL 666
i suggest -1=notimpl 0=error 1=ok

> diff -Naur main.original/libmpdemux/demuxer_convenience.c main/libmpdemux/demuxer_convenience.c
> --- main.original/libmpdemux/demuxer_convenience.c	Thu Jan  1 01:00:00 1970
> +++ main/libmpdemux/demuxer_convenience.c	Thu Nov  7 00:07:15 2002
> @@ -0,0 +1,23 @@
> +#include "stream.h"
> +#include "demuxer.h"
> +#include "stheader.h"
> +#include "mf.h"
> +
> +
> +unsigned long demuxer_get_time_length(demuxer_t *demuxer){     
> +    unsigned long get_time_ans;     
> +    if (demux_control(demuxer, DEMUXER_CTRL_GET_TIME_LENGTH,(void *)&get_time_ans)!=DEMUXER_CTRL_OK)  {
> +        get_time_ans=0;     
> +    }
> +    return get_time_ans;
> +}

no need for new files (with such looooong filename) for these functions, put
them to demuxer.c

> -  if(!quiet) mp_msg(MSGT_AVSYNC,MSGL_STATUS,"A:%6.1f %4.1f%% %d%%   \r"
> +  if(!quiet) mp_msg(MSGT_AVSYNC,MSGL_STATUS,"S:%3d A:%6.1f %4.1f%% %d%%   \r"

this should be rethinking, the current statusline is already close to 80
chars width, so with this new fields it won't fit to 80xXX terminals.

i suggest a 'frontend mode' switch, or even enable it with the -svale
option, and it would output usefull infos in a machine-parseable form.


A'rpi / Astral & ESP-team

--
Developer of MPlayer, the Movie Player for Linux - http://www.MPlayerHQ.hu