[FFmpeg-devel] [PATCH] SDP muxer

[Luca Abeni]

Hi,

Stefano Sabatini wrote:
[...]
>>>> 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.

I did not know about AVStream->filename. What's its expected behaviour?
The comments says "source filename of the stream", which does not make
too much sense in this case... Is it ok to use AVStream->filename in this
way? Where is it used in libavformat, and who sets it?
Depending on the answers, maybe a patch like the attached one (completely
untested, and maybe needs some fixes/cleanup) would make sense. This patch
would greatly simplify the SDP muxer.

>>>> 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 do not like the way you did it... In particular, I think that
avf_sdp_create2() is not needed: you can just move the needed code in
the SDP muxer "write_header" method, and then call avf_sdp_create()
directly.


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

I am still  against this rename. My preference is for a
<subsystem>_<verb>_<object> naming scheme.

Summing up:
- sdp-rename.patch: NACK. Some of the renames might make sense (I am
   still undecided, but I do not like the ones like
   "sdp_write_header ---> write_sdp_header".
- implement-avf-sdp-create2.patch: I think this is not needed (and we
   do not need a new function).
- implement-sdp-muxer-2.patch: This must be modified to use the attached
   patch (if it makes sense - it depends on what AVStream->filename is, and
   on how it is used), or to forge a proper array of AVFormatContexts and
   then call avf_sdp_create()
I am not sure about implement-av-dup-stream-1.patch (I leave it to
Michael, since it touches the core of libavformat), and
implement-ffmpeg-sdp-mapping-2.patch looks ok, but Michael should comment
on it.


			Thanks,
				Luca
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sdp.diff
Type: text/x-diff
Size: 1249 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081121/c34ad10c/attachment.diff>