[FFmpeg-devel] [PATCH] RDT/Realmedia patches #2

[Luca Abeni]

Ronald S. Bultje wrote:
> Hi Luca,
> 
> On Wed, Oct 1, 2008 at 3:38 PM, Luca Abeni <lucabe72 at email.it> wrote:
>> The concept is (IMHO) fine, but I have some comments on the implementation:
> [..]
>>>  /**< return 0 on packet, no more left, 1 on packet, 1 on partial packet... */
>>>  static int
>>> -rdt_parse_packet (RTPDemuxContext *s, AVPacket *pkt, uint32_t *timestamp,
>>> +rdt_parse_packet (void *priv_data, AVStream *st,
>> I think "void *" is not a good way to have an opaque data type. Maybe we
>> can rename "struct rdt_data" to "struct TransportContext" or something similar,
>> and have rdt_parse_packet(struct TransportContext *priv_data ...).
>> (and in rtp_internal.h you just have "struct TransportContext;" without
>> actually defining the structure, that is defined in rdt.c, so it is opaque).
>> This will be safer and will avoid a cast.
> 
> I suppose you don't like unnamed structs then, right? :-).

I do not know, I never wrote code using them ;-)


> OK, I will
> go and try this. I oppose the name TransportContext, because it
> suggests that it contains data related to RTP or RDT.

You are right here, I was confused (I saw the "rdt_data" name and I
did not check the structure contents).


> You'll notice
> that I'm not making RTPDemuxContext opaque here, but rather the access
> to it. void* here isn't actually a RTPDemuxContext anymore after this
> patch, but rather a datatype context (e.g. h264 or mpeg; see how the
> caller changes from passing "s" to "s->dynamic_protocol_context"). I
> propose the name RTPPacketDataContext, is that OK?

I do not like the "RTP" part of the name. "PacketDataContext" is ok, I
thin. Or "PayloadContext" (since the structure contains data needed to
parse the payload, right?)



				Luca