[MPlayer-dev-eng] Re: [PATCH]H264 over rtsp

[Reimar Döffinger]

Hello,
On Thu, Aug 17, 2006 at 09:56:36AM +0000, Carl Eugen Hoyos wrote:
> Carl Eugen Hoyos <cehoyos <at> ag.or.at> writes:
> > This new version of the patch works for all H264 rtsp streams I found:
> > I hope it works for you too. (parseH264ConfigStr() copied from vlc)
> 
> Updated patch (Reimar changed demux_rtp_codec.cpp) in case somebody is
> interested in watching Steve Jobs:

Can you attach the patch? I have horrible line breaks here...

> @@ -217,6 +225,7 @@
>    int fps = (int)(subsession->videoFPS());
>    if (fps != 0) {
>      sh_video->fps = fps;
> +    sh_video->frametime=1.0f/fps;

Is this really necessary? It looks to me like it isn't really used at
important point. It probably is correct though, maybe this should be
applied first in a separate patch.

> -      fps = (int)(1/(curPTS-lastPTS) + 0.5); // rounding
> -      fprintf(stderr, "demux_rtp: Guessed the video frame rate as %d
> frames-per-second.\n\t(If this is wrong, use the \"-fps <frame-rate>\" option
> instead.)\n", fps);
> -      sh_video->fps = fps;
> -      return;
> +      fps = (int)(1/fabs(curPTS-lastPTS) + 0.5); // rounding
> +      if (fps == lastfps) {
> +        mp_msg(MSGT_DEMUX, MSGL_INFO, "demux_rtp: Guessed the video frame rate
> as %d frames-per-second.\n\t(If this is wrong, use the \"-fps <frame-rate>\"
> option instead.)\n", fps);
> +        sh_video->fps = fps;
> +        sh_video->frametime=1.0f/fps;
> +        return;

Please do "cosmetics" (here: indentation changes) in a separate patch,
it just make things hard to review.

> +static int b64_decode( char *dest, char *src )

should be "const char *src".

> +    int  b64[256] = {

Should be "static const uint8_t b64", and maybe even only 128 entries.

> +    for( i_level = 0; *src != '\0'; src++ )
> +    {
> +        int  c;
> +
> +        c = b64[(unsigned int)*src];
> +        if( c == -1 )
> +        {
> +            continue;
> +        }
> +
> +        switch( i_level )
> +        {
> +            case 0:
> +                i_level++;
> +                break;
> +            case 1:
> +                *dest++ = ( last << 2 ) | ( ( c >> 4)&0x03 );
> +                i_level++;
> +                break;
> +            case 2:
> +                *dest++ = ( ( last << 4 )&0xf0 ) | ( ( c >> 2 )&0x0f );
> +                i_level++;
> +                break;
> +            case 3:
> +                *dest++ = ( ( last &0x03 ) << 6 ) | c;
> +                i_level = 0;
> +        }
> +        last = c;
> +    }

I find this really ugly and overcomplicated, but maybe that's just me..

> +    *dest = '\0';

Does this make sense? base64 encoded stuff might contain null character,
so treating the result of decoding as a C string is broken anyway, so
I'm somewhat against encouraging that this way.

> +    unsigned char *cfg = new unsigned char[5 * strlen(psz)];

Is using new for allocating something that is later used in C code a
good idea? Btw. I guess I missed it, where is this freed?

> +        configSize += b64_decode( (char*)&cfg[configSize], psz );

I'd prefer if the b64_decode argument was made unsigned instead...

> @@ -453,6 +453,10 @@
>    bufferQueue->blockingFlag = ~0;
>  }
> 
> +static demux_packet_t* seconddp = NULL;
> +static int packetsneeded = 0;
> +static float lastpts;
> +

Using global variables in a demuxer is a really bad idea, there might be
two instances of it at the same time (see -audiofile).

>    // Allocate a new packet buffer, and arrange to read into it:
> -  dp = new_demux_packet(MAX_RTP_FRAME_SIZE);
> -  bufferQueue->dp = dp;
> -  if (dp == NULL) return NULL;
> +  if (!seconddp || !sh_video) {
> +    dp = new_demux_packet(MAX_RTP_FRAME_SIZE);
> +    bufferQueue->dp = dp;
> +    if (dp == NULL) return NULL;
> +  }

Cosmetics, too.

Greetings,
Reimar Döffinger