[FFmpeg-devel] [RFC] SDP Generation

[Michael Niedermayer]

Hi

On Fri, Jun 15, 2007 at 03:05:43PM +0200, Luca Abeni wrote:
> Hi all,
> 
> Luca Abeni wrote:
> [...]
> >>multi stream AVFormatContext + several AVFormatContext seems more correct
> >>as a different address = different file more or less
> >Ok. I'll work on this solution, and post an updated patch.
> Here is an updated version of the SDP generator for libavformat.
> I think I addressed the previous review, and I tried to do my best for 
> implementing a reasonable interface (but I do not know how successful 
> I've been in this).
> 
> The SDP generating function is still
> char *avf_sdp_create(AVFormatContext *ac[], int n_files)
> but now n_files == 1 means that all the streams are stored in a single 
> AVFormatContext (in this case, I assumed the destination ports are 
> consecutive).

[...]
> 
> static void attribute_write(ByteIOContext *buff, const char *key, const char *val)
> {
>     url_fprintf(buff, "a=%s:%s\r\n", key, val);
> }

is the dependancy on ByteIOContext really needed? what is its advantage?


[...]
> static void sdp_write_header(ByteIOContext *buff, struct sdp_session_level *s) 
> {
>     url_fprintf(buff, "v=%d\r\n", s->sdp_version);
>     url_fprintf(buff, "o=- %d %d IN IPV4 %s\r\n", s->id, s->version, s->src_addr);
>     url_fprintf(buff, "t=%d %d\r\n", s->start_time, s->end_time);
>     url_fprintf(buff, "s=%s\r\n", s->name);

theres no need to do 4 calls



>     dest_write(buff, s->dst_addr, s->ttl);
>     attribute_write(buff, "tool", "libavformat");

these too are just url_fprintf( ) calls which should be merged with the
above 4


> }
> 
> static int get_address(char *dest_addr, int size, int *ttl, const char *url)
> {
>     int port;
>     const char *p;
> 
>     url_split(NULL, 0, NULL, 0, dest_addr, size, &port,
>               NULL, 0, url);
>     

trailing whitespace


[...]
>         int is_multicast;
> 
>         is_multicast = find_info_tag(buff, sizeof(buff), "multicast", p);

this can be merged with the declaration


[...]
> static void digit_to_char(char *dst, uint8_t src)
> {
>     if (src < 10) {
>         *dst = '0' + src;
>     }
> 
>     *dst = 'A' + src - 10;
> }

this is nonsense

maybe you meant: ?

if(src<10) *dst = src + '0';
else       *dst = src + 'A'-10;


[...]
>     if (n_files == 1) {
>         for (i = 0; i < ac[0]->nb_streams; i++) {
>             sdp_write_media(&buff, ac[0]->streams[i]->codec, NULL, (port > 0) ? port + i * 2 : 0, 0);
>             /* This is needed by RTSP servers... FIXME: make it optional? */
>             snprintf(attr, 64, "%s%d", "streamid=", i);
>             attribute_write(&buff, "control", attr);
>         }
>     } else {
>         for (i = 0; i < n_files; i++) {
>             port = get_address(dst, sizeof(dst), &ttl, ac[i]->filename);
>             sdp_write_media(&buff, ac[i]->streams[0]->codec, dst[0] ? dst : NULL, port, ttl);
>             /* This is needed by RTSP servers... FIXME: make it optional? */
>             snprintf(attr, 64, "%s%d", "streamid=", i);
>             attribute_write(&buff, "control", attr);
>         }
>     }

why not go over all streams in all contexts?



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070616/387cbd9e/attachment.pgp>