[FFmpeg-devel] [PATCH] SDP muxer

[Stefano Sabatini]

On date Thursday 2008-11-20 10:05:41 +0100, Stefano Sabatini encoded:
> 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?

I'm the mapping code of ffmpeg.c I'm storing the AVFormatOutput
filename in the AVStream filename, which is then used to compute the
destination address. It works but looks very fragile and I'm not very
happy about it.

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

Done.
   
> > > 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?

No, that was plenty wrong, fixed, now there is no SDPContext.
  
> > > 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.

Keeping it in the patch while waiting for your reply.
  
> > > 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.

Quilt series attached.

Please note that I didn't thoroughly test the patches, since I think
we're still at a "consulting for the best solution" phase.

Regards.
-- 
FFmpeg = Free & Faboulous Multimedia Porno Enhanced Geisha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sdp-rename.patch
Type: text/x-diff
Size: 3470 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081120/442ae71b/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-avf-sdp-create2.patch
Type: text/x-diff
Size: 3845 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081120/442ae71b/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-sdp-muxer.patch
Type: text/x-diff
Size: 1439 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081120/442ae71b/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-av-dup-stream.patch
Type: text/x-diff
Size: 1169 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081120/442ae71b/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-ffmpeg-sdp-mapping.patch
Type: text/x-diff
Size: 6019 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081120/442ae71b/attachment-0004.patch>