[FFmpeg-devel] [RFC] SDP Generation
Michael Niedermayer
michaelni
Mon Jun 18 23:02:24 CEST 2007
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>
More information about the ffmpeg-devel
mailing list