[vlc-devel] [PATCH] decode an RTP/H264 elementary stream without using SDP

[Tony Vankrunkelsven]

Hi Rémi,

Is the patch already applied? Could you check the comments from previous email?


Thanks,
Tony 

-----Original Message-----
From: Tony Vankrunkelsven 
Sent: woensdag 22 september 2010 12:03
To: 'vlc-devel at videolan.org'
Cc: 'remi at remlab.net'
Subject: RE: [vlc-devel] [PATCH] decode an RTP/H264 elementary stream without using SDP

Hi Remi,

Thanks for the quick review. See some comments inline:


On Tuesday 21 September 2010, Rémi Denis-Courmont wrote:
> Hello,
> 
> Comments inline.
> 
> On Tuesday 21 September 2010, Tony Vankrunkelsven wrote:
>> @@ -454,6 +453,11 @@ static void stream_decode (demux_t *demux, void *data,
>> block_t *block) (void)demux;
>>  }
>> 
>> +static void *demux_init (demux_t *demux)
>> +{
>> +    return stream_init (demux, *demux->psz_demux ? demux->psz_demux :
>> "unknown");
>> +}
> 
> Isn't that "unknown" path impossible? I would rather have an assertion here. 
> Trying the "unknown" demuxer is not going to be any good.

Normally the "unknown" path is impossible since there are currently some 'if' statements 
before demux_init is called. It is added just in case demux_init is used and 
"demux->psz_demux" is not specified. I've check this path with some test code 
and the result is that no demux module can be found.

debug info:
main debug: creating demux: access='' demux='unknown' path=''
main debug: looking for demux module: 0 candidates
main debug: no demux module matched "unknown"
 
>> @@ -679,7 +683,28 @@ int rtp_autodetect (demux_t *demux, rtp_session_t
>> *session, break;
>> 
>>        default:
>> -        return -1;
>> +        /*
>> +         * If the rtp payload type is unknown then check demux if it is
>> specified +         */
>> +        if (*demux->psz_demux)
> 
> Isn't that statement redumdant with regards to the following strcmp()s?

The 'if' statement could be seen as redundant. But if "demux->psz_demux" is not specified
it looks better not to do the strcmp()s. Also more strcmp()s might be added in the future.

> 
>> +        {
>> +          if ((strcmp(demux->psz_demux, "h264") == 0) ||
>> (strcmp(demux->psz_demux, "ts") == 0))
>> +          {
>> +            msg_Dbg (demux, "rtp autodetect specified demux=%s",
>> demux->psz_demux); +            pt.init = demux_init;
>> +            pt.destroy = stream_destroy;
>> +            pt.decode = stream_decode;
>> +            pt.frequency = 90000;
>> +            break;
> 
> I assume you tested that this actually works? It would be nice to patch 
> modules/services/discovery/sap.c accordingly then.

The code is tested in the lab with a custom vlc build and some h264 multicast streams. 
It works good and it doesn't affect the current working of VLC if the demux isn't specified 
in the MRL.

I had a look at the code "modules/services/discovery/sap.c". I'm not so familiar with the
code but this is how I think it could work. There is a function "IsWellKnownPayload". 
The payload types specified in this function should be a static rtp payload type matching 
the types specified in "modules/access/rtp/rtp.c" (rtp_autodetect) and the static rtp types defined 
by IANA see http://www.iana.org/assignments/rtp-parameters). SAP typically uses Session 
Description Protocol (SDP). At some point in the code there is a check if the payload 
type is well known (using IsWellKnownPayload) or if SDP should be further parsed to 
know the payload type, codec info, .... In case of H.264 the payload type is in the dynamic
range, so it is not well known and in this case SDP should be further parsed. Since SDP 
is used in this case there is no neeed to "force" the demux. The values of the SDP file should
be used.


Best Regards,
Tony Vankrunkelsven