[FFmpeg-devel] [PATCH] SDP muxer

Stefano Sabatini stefano.sabatini-lala
Sat Nov 22 11:34:14 CET 2008


On date Friday 2008-11-21 08:41:04 +0100, Luca Abeni encoded:
> 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?
>>
>> In 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.

I see the point of your patch, but I cannot see how it could simplify
the SDP muxer.

Also I'm not sure if the use of the stream filename field is
meaningful, if that's the case we could copy the AVOutputStream
filename while creating a new stream as in the attached patch. Then we
should free the filename when closing the stream (and here I miss an
av_close_output_stream() function...).

Also in the code we're assuming that an RTP AVOutputStream may contain
more than one AVStream, does this make sense?

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

See the comment below.

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

What about something like:
sdp_write_header ---> sdp_write_sdp_header

All I want is to avoid semantic conflicts with the names of the
function, now that sdp.c contains now both the code for avf_create_sdp
and the SDP muxer (for which a write_header() function which writes
the whole SDP is needed).

I think that having both:
sdp_write_header() // writes the "header" part of an SDP

and

write_header()     // writes the whole SDP, implemented as part of the muxer API

is confusing.

Furthermore having the sdp_ prefix seems pointless to me, since all
the function are static so they can't conflict with other namespaces
(but I have no strong opinion regarding this prefixing issue).

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

The SDP muxer will work taking an array of streams as found in the
AVOutputStream, and issuing the SDP from them.

Taking the path:

creation of the SDP AVOutputStream  : files -> streams
and then 
writing of the SDP in write_header(): streams -> files

is unnecessarily complicated, while with using the function
avf_sdp_create2 we would avoid the second conversion (while the first
one is necessary in each case).

Then I thought that having avf_create_sdp2() in the public API may be
useful, but we could simply avoid to make it public.

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

Yes, and I suspect the memcpy on the AVCodecContext may cause major
problems, maybe we would need also some sort of avcodec_dup_context().

[...]

Thanks for the review, regards.
-- 
FFmpeg = Fantastic & Funny Meaningful Problematic Elastic Gadget
-------------- next part --------------
A non-text attachment was scrubbed...
Name: strdup-filename-in-av-new-stream.patch
Type: text/x-diff
Size: 456 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081122/fe0e0e08/attachment.patch>



More information about the ffmpeg-devel mailing list