[FFmpeg-devel] [PATCH] SDP muxer

[Stefano Sabatini]

On date Thursday 2008-11-20 08:22:50 +0100, Luca Abeni encoded:
> Hi Stefano,
> 
> Stefano Sabatini wrote:
> [...]
> >>> Yes, this solution won't break neither URLs syntax neither old API
> >>> interface, while on the other hand I'm extending the interface, in
> >>> order to create a representation of an SDP from a list of RTP files.
> >> Ok, good. Note that avf_sdp_create() should already be able to do what
> >> you need (you just need to prepare a proper "AVFormatContext *ac[]"
> >> before calling it). So, I guess everything you need is a new muxer which
> >> sets up the "ac" array and calls avf_sdp_create().
> > 
> > The problem with avf_sdp_create() is that it takes AVOutputFormat,
> 
> What is the exact problem with this?
> 
> > while I think the expected behaviour for the muxer would be to take a
> > list of *AVStreams* and create from these the header.
> 
> Maybe. But then, is it still possible to have different destination
> multicast groups for different streams? I mean: does an AVStream allow
> to store the destination MC group (and port) somewhere?
> 
> > So I write a write_header() function which is very similar to
> > avf_sdp_create(), but which reads from streams rather than from a list
> > of files.
> 
> I suspect this will result in some code duplication; you should not do
> it. What you should do is to write a write_header() function that prepares
> an array of AVFormatContexts for avf_sdp_create(), and then calls it.

Yes, this can be factorized.
  
> > I'm attaching my implementation, but I'm somehow not satisfied with it).
> > 
> > Patch consists of the part (I'll split it when we'll agree on some
> > solution for the problem I'm going to discuss):
> > 
> > * cosmetics to sdp.c: struct sdp_session_level -> SDPContext and other
> 
> What's the point of this change? Note that it is wrong: sdp_session_level
> is not a generic "SDP context", but it contains the session-level description
> of an SDP.

What about an SDPContext having sdp_session_level as a field?
 
> > renames in order to make them more meaningful and consistent (e.g.:
> > sdp_write_media -> write_sdp_media
> 
> Sorry, but again I do not see the point in this change.
> 
> > sdp_media_attributes -> write_sdp_media_attributes
> > sdp_write_header -> write_sdp_header)
> 
> Idem.
> Sorry, but I'll not approve this kind of changes.

I think they make sense, but I won't insist on them.

sdp_write_header() may be confused with the muxer write_header()
function, also I think it is more clear to call it write_sdp_header
since what it does is to write the SDP header, same for the other
function.

Compare this list:

sdp_write_header()
sdp_write_media()
sdp_media_attributes()

with:

write_sdp_header()
write_sdp_media()
write_sdp_media_attributes()

I think the second list is better, but again I won't waste our time on
this if you prefer the other way.
 
> > The use of av_dup_stream() is necessary for the "special" status of
> > the SDP muxer, which takes streams which have been already defined.
> 
> I am not sure about it; let's see Michael's comments.

Thanks for the review, regards.
-- 
FFmpeg = Fiendish and Freak Magical Prodigious Eretic Gadget