[FFmpeg-devel] [RFC] SDP Generation

[Michael Niedermayer]

Hi

On Mon, Jun 18, 2007 at 04:39:54PM +0200, Luca Abeni wrote:
> 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.

well id like to avoid dependancies if possible, that is IMHO the most
standard stuff should be used if you can, that is if the iso c functions
are enough they should be used and no dynamic ByteIOContext url_fprintf()

iam thiking of someone maybe wanting to use the sdp code outside avformat,
maybe that someone could even be us spliting it out into a libavstream or
libavio, libavprotocol or whatever

so IMHO less dependancies are better, that said code duplication is bad too

> 
> [...]
> >> 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.

foo_fprintf(buff,   "v=%d\r\n"
                    "o=- %d %d IN IPV4 %s\r\n"
                    "t=%d %d\r\n"
                    "s=%s\r\n"
                    , s->sdp_version
                    , s->id, s->version, s->src_addr
                    , s->start_time, s->end_time
                    , s->name);

doesnt look worse IMHO


> 
> [...]
> >>     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)

when the modularity is needed it can be split until then i prefer it in the
simplest way possible


[...]
> [...]
> >>     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

likely true but why duplicate the code just to make cases outside this
harder ;)


> 
> 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 :)

you dont have to support that, i just like you to merge the 2 for loops
somehow if possible and

for all avformat contest
    for all streams in the current context

seemed like a natural way to do it but maybe iam missing some complexities
this would cause

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20070618/c6bf4618/attachment.pgp>