[FFmpeg-devel] [PATCH] RTSP-MS 13/15: transport context

[Michael Niedermayer]

On Sat, Jan 31, 2009 at 11:18:15AM -0500, Ronald S. Bultje wrote:
> Hi Michael,
> 
> 2009/1/10 Michael Niedermayer <michaelni at gmx.at>:
> > On Tue, Jan 06, 2009 at 08:49:55AM -0500, Ronald S. Bultje wrote:
> >> On Tue, Jan 6, 2009 at 12:17 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> >> > this patch adds a "parent class" to RTPDemuxContext and
> >> > RDTDemuxContext, called transport_context, which contains the only
> >> > shared structure between the two: the RTSP AVFormatContext. I need
> >> > that for my next patch to be able to retrieve the ASF demuxer's
> >> > AVFormatContext from the RTSPState (in the RTSP
> >> > AVFormatContext->priv_data) so I can call ASF demuxer functions in the
> >> > RTP payload parsing function.
> >
> > I dont understand why you need this, your explanation makes no sense,
> > thus i will not approve it.
> 
> Sorry for the bad explanation. The reason I need it is as follows:
> when reading an RTSP stream, the RTSP demuxer will determine whether
> this stream contains RDT or RTP packets (this is called the
> transport). We then call rtp_parse_packet() or ff_rdt_parse_packet()
> depending on the transport. Each of them have a special
> "transport-specific data" structure, RDTDemuxContext and
> RTPDemuxContext. We call functions with these contexts.
> 
> In the Transport are RTP/RDT data "payload" packets, and these are
> sort of orthogonal to the transport. RDT commonly contains Realmedia
> payload packets, whereas RTP can contain anything (ASF packets, MPEG-4
> audio/video, etc.). In theory, RDT can also stream any of these (and
> the Real servers support that). So, ff_rdt_parse_packet() or
> rtp_parse_packet() call the respective payload's parse_packet()
> function to parse the payload data with a payload-specific
> PayloadContext structure.
> 
> So the structures are as follows:
> AVFormatContext->priv_data is a RTSPState
> AVFormatContext->streams[n] are AVStreams
> 
> RTSPState->cur_tx is a RTP/RDTDemuxContext
> RTSPState->rtsp_streams are RTSPStreams
> 
> RTP/RDTDemuxContext->ic is the above-mentioned AVFormatContext
> 
> AVStream->priv_data is RTSPStream
> 
> RTSPStream->stream_index is the AVStream index in the AVFormatContext
> (to allow going back from RTSPStream to AVStream)
> RTSPStream->tx_ctx is a RTP/RDTDemuxContext
> RTSPStream->dynamic_protocol_context is a PayloadContext
> 
> In my parse_packet() implementation for the RTP/ASF payloads, I want
> to use a value from the RTSP demuxer's RTSPState, so I need access to
> the AVFormatContext. In that function, I'm provided a PayloadContext
> and an AVStream, so I'd like to go from PayloadContext back to
> AVFormatContext. I can do two things:
> 
> 1) RTP/RDTDemuxContext *tx_ctx = ((RTSPStream *) st->priv_data)->tx_ctx;
>     AVFormatContext *rtsp_dmx_ctx = tx_ctx->ic;
> 2) I can change the parse_packet() prototype to accept the RTSP
> demuxer's AVFormatContext as a function argument.
> 
> I chose (1) for this solution. The problem is that RTP/RDTDemuxContext
> is an opaque structure because it can be either a RTP or a
> RDTDemuxContext, and I would prefer if the parse_packet()
> implementation wouldn't have to distinguish between the two. Instead,
> I'd like to add a common "parent class" (similar to AVClass in AV*)
> between the two, so that the resulting code ((1) above plus the code
> that distinguishes between RTP/RDTDemuxContext) can be replaced by a
> single line "rtsp_dmx_ctx = (st->priv_data)->tx_ctx->ic;". This patch
> implements that common parent class.
> 
> I hope that's clear, let me know if it is not.

As far as iam concerned this is a mess but iam not maintainer of it
all the parts RTSP/RDT/RTP should be more seperated and have a clear
API between them not this kind of "i now need the context of your brothers
mother"

IMHO the first thing you should do is document RTP/RTSP
draw a few ascii art diagrams showing the relation of the structs
give each of the structs a doxy comment explaining what it represents
semantically.

I dont have a clear understanding or the RTP/RTSP code and so i wont
approve patches to it except for the parts that i do understand.
Your explanation above is lacking badly at many points, example being
"In my parse_packet() implementation for the RTP/ASF payloads, I want
 to use a value from the RTSP demuxer's RTSPState"
which value? and why? the whole just smells even without understanding
the whole picture it feels very hackish.

I think that there should be a clear API between the transpot (RTP/RDT)
and RTSP. And never should either side touch anything but the documented
API.
And again that is my oppinion based on incomplete understanding of the code
so i could be missing some important things and my conclusion could be
wrong.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090201/aaf3b97f/attachment.pgp>