[FFmpeg-devel] [RFC] SDP Generation

[Luca Abeni]

Hi Michael,

Michael Niedermayer wrote:
[...]
>> 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?
It's a way to address the code quadruplication you noticed in my 
previous version. I could move the buffer size checks in an 
"sdp_print()" function, or use ByteIOContext. I have no problems in 
using another solution.

[...]
>> 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
I did this to have a printf per SDP line (it looks more readable to me), 
but I can switch to a single call, if needed.

[...]
>>     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
Same as above (I think the code looks more modular in this way, but I 
have no problems changing it)

[...]
>>         int is_multicast;
>>
>>         is_multicast = find_info_tag(buff, sizeof(buff), "multicast", p);
> 
> this can be merged with the declaration
Ok

> 
> [...]
>> static void digit_to_char(char *dst, uint8_t src)
>> {
>>     if (src < 10) {
>>         *dst = '0' + src;
>>     }
>>
>>     *dst = 'A' + src - 10;
>> }
> 
> this is nonsense
You are right... I do not even know why it worked when I tested it! 
Sorry for posting this thing :(

[...]
>>     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?
Well, I was under the impression that the only cases that make sense are
- 1 AVFormat with many streams
- Many AVFormats with only one stream per avformat

I'll look at this again, and try to support the "many avformats with 
many streams" case (but first I have to understand in which case a user 
might want to use it :)



				Thanks again,
					Luca