[Ffmpeg-devel] RTP patches & RFC
Ryan Martell
rdm4
Tue Oct 17 23:05:28 CEST 2006
On Oct 16, 2006, at 7:25 PM, Michael Niedermayer wrote:
> On Thu, Oct 12, 2006 at 04:29:09PM -0500, Ryan Martell wrote:
>> On Oct 12, 2006, at 5:29 AM, Michael Niedermayer wrote:
>>> On Wed, Oct 11, 2006 at 10:41:36PM -0500, Ryan Martell wrote:
>>> [...]
>>>>>
>>>>> either way you should set AVStream->need_parsing=1 and leave the
>>>>> merging
>>>>> of packets to the AVParser
>>>>
>>>> Okay, sorry to be dense about this, but I'm still confused. The
>>>> packets that come over rtp have their size indicated in various
>>>> ways,
>>>> depending on the packet. That information is NOT in the NAL,
>>>> and the
>>>> NAL is NOT preceeded by the start sequence. So what I was doing
>>>> was
>>>> converting them to AvC style (preceded by length) packets. This is
>>>
>>> iam more in favor of adding the startcode prefix instead of the
>>> length
>>> i dont think the avc style length stuff is supported by our AVParser
>>>
>>
>> I changed the code, so that in my init routine you can specify if you
>> want the bitstream version or the avcc version. Both work, as long
>> as I don't set needs_parse= 1 for the stream. When I do that, I
>> never get past the setup code (I don't know why). (Maybe it's
>> because I only had 001 instead of 0001 on the sps/pps packets, from
>> derk above?). This is the same issue i was having when i first
>> started this, where there was no data in the extradata field, it
>> wouldn't get to the point where it showed the stream types.
>
> does this also happen if you simply dump the data to a file and then
> try to play it with ffmpeg or ffplay? if yes then please upload such
> a file ill take a look
I've sorted that out now; I'm using the AVParser, using the
bytestream start sequence...
> [...]
>>> stuff starting with underscores is reserved in c
>>> and comments should be doxygen compatible
>>> and the enum should get a name which is used in the types which are
>>
>> doxygen is pretty cool; never used it before.
>>
>> Here's all of my code; I didn't really want to get into rtp/rtsp that
>> much, but I had to. It's still a work in progress (it times out
>> after 2 minutes on Darwin Streaming Server, because i don't send the
>> rtcp packets). But it's a start. I'd like to get this out there, in
>> case others need it, but I have some other requirements that might
>> pull me away from this for the next few days/week.
>>
>> All the jitter calculation/rtp sequence updating algorithms come from
>> the rtp rfc.
>>
>> Michael, thanks for the code review, and hopefully this is more
>> inline with the other code.
>
> its too big, i tried to review it (see below) but 80k is simply too
> big
> for a single selfcontained patch, also the rt*p code isnt really my
> area
> which doesnt simplify this
>
> so please try to split this somehow into small and independant patches
Um... okay. I'll see what I can do. I hadn't wanted to do anything
to rtp/rtsp until I had the h264 stuff in, but it was required to
make the inclusion somewhat clean. (Plus, I have to get the rtsp
receiver reports in to make it work).
> [...]
>> @@ -179,43 +182,28 @@
>> {-1, "", CODEC_TYPE_UNKNOWN, CODEC_ID_NONE, -1, -1}
>> };
>>
>> -AVRtpDynamicPayloadType_t AVRtpDynamicPayloadTypes[]=
>> +/* statistics functions */
>> +RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler= NULL;
>> +
>> +static RTPDynamicProtocolHandler mp4v_es_handler= {"MP4V-ES",
>> CODEC_TYPE_VIDEO, CODEC_ID_MPEG4};
>> +static RTPDynamicProtocolHandler mpeg4_generic_handler= {"mpeg4-
>> generic", CODEC_TYPE_AUDIO, CODEC_ID_MPEG4AAC};
>> +
>> +static void register_dynamic_payload_handler
>> (RTPDynamicProtocolHandler *handler)
>> {
>> - {"MP4V-ES", CODEC_TYPE_VIDEO, CODEC_ID_MPEG4},
>> - {"mpeg4-generic", CODEC_TYPE_AUDIO, CODEC_ID_MPEG4AAC},
>> - {"", CODEC_TYPE_UNKNOWN, CODEC_ID_NONE}
>> -};
>> + handler->next= RTPFirstDynamicPayloadHandler;
>> + RTPFirstDynamicPayloadHandler= handler;
>> +}
>
> why do you change the AVRtpDynamicPayloadType_t to a linked list based
> system? and this should be a seperate patch
I did this to make all prototypes static to the rtp_h264 code. I was
basing this off of how the codecs worked. Eventually (IMHO), all of
the other codecs listed should be broken out of rtp.c and put into
separate files, so that all the codec specific stuff for the dynamic
protocols can be isolated from the generic (For all protocols)
stuff. Currently, rtp.c is a bit of a mess, with hacks to support
the mpeg4-generic stuff throughout.
> [...]
>> +void rtp_configure_dynamic_payloads()
>> +{
>> + if(RTPFirstDynamicPayloadHandler==NULL)
>> + {
>> + register_dynamic_payload_handler(&mp4v_es_handler);
>> + register_dynamic_payload_handler(&mpeg4_generic_handler);
>> + register_dynamic_payload_handler(&ff_h264_dynamic_handler);
>> + }
>> +}
>
> this doesnt look thread safe
I couldn't find anything in the rtp code that was called at
initialization time. What would you recommend for this?
> [...]
>
>> @@ -264,13 +252,94 @@
>> if (s->first_rtcp_ntp_time == AV_NOPTS_VALUE)
>> s->first_rtcp_ntp_time = s->last_rtcp_ntp_time;
>> s->last_rtcp_timestamp = decode_be32(buf + 16);
>> +
>> return 0;
>
> cosmetic change (=whitespace only change) these should be in a
> seperate patch
> if they are usefull for readablity, either way they must not be in non
> cosmetic patches
>
>
> [...]
>> +
>> +// returns 1 if we should handle this packet.
>
> this should be a doxygen compatible comment /** @return ... */
>
>
>> +static int rtp_valid_packet_in_sequence(RTPStatistics *s,
>> uint16_t seq)
>> +{
>> + uint16_t udelta= seq - s->max_seq;
>
> is there any specific reason why this one and many other variables are
> (u)intXY_t and not simply int? (u)intXY_t types are exact size and
> can be
> slower then int on some architectures, if you want to be pedantic
> and want
> the fasterst type with at least X bits then uint_fast16_t, ... are the
> correct ones
> for arrays of course (u)intXY_t is ok due to the space saving
No problem; I starting using all the u/int stuff because I thought
that was the way to go; meaning that by specifying byte size it was
most cross platform compatible. I knew about the slower issue, but
couldn't tell from just looking what concepts were followed
throughout the rest of the code. Thanks for clarifying this.
> [...]
>> +static void rtcp_update_jitter(RTPStatistics *s, uint32_t
>> sent_timestamp, uint32_t arrival_timestamp)
>> +{
>> + uint32_t transit= arrival_timestamp - sent_timestamp;
>> + int d= transit - s->transit;
>> + s->transit= transit;
>> + if(d<0) d= -d;
>
> d= FFABS(transit - s->transit);
>
>
> [...]
>> - default:
>> +
>> + default:
>> break;
>
> cosmetic change
>
>
>> }
>> }
>> @@ -372,19 +444,27 @@
>> AVStream *st;
>> uint32_t timestamp;
>>
>> - if (!buf) {
>> - /* return the next packets, if any */
>> - if (s->read_buf_index >= s->read_buf_size)
>> - return -1;
>> - ret = mpegts_parse_packet(s->ts, pkt, s->buf + s-
>> >read_buf_index,
>> - s->read_buf_size - s-
>> >read_buf_index);
>> - if (ret < 0)
>> - return -1;
>> - s->read_buf_index += ret;
>> - if (s->read_buf_index < s->read_buf_size)
>> - return 1;
>> - else
>> - return 0;
>> + if (!buf)
>> + {
>> + if(s->st && s->dynamic_packet_handler)
>> + {
>> + return s->dynamic_packet_handler(s, pkt, 0, NULL, 0);
>> + }
>> + else
>> + {
>> + /* return the next packets, if any */
>> + if (s->read_buf_index >= s->read_buf_size)
>> + return -1;
>> +
>> + ret = mpegts_parse_packet(s->ts, pkt, s->buf + s-
>> >read_buf_index, s->read_buf_size - s->read_buf_index);
>> + if (ret < 0)
>> + return -1;
>> + s->read_buf_index += ret;
>> + if (s->read_buf_index < s->read_buf_size)
>> + return 1;
>> + else
>> + return 0;
>> + }
>
> unneeded reindention, quoting development policy:
> "... If you had to put if(){ .. } over a large (> 5 lines) chunk of
> code, then either do NOT change the indentation of the inner part
> within (don't move it to the right)! or do so in a separate
> commit" (http://ffmpeg.mplayerhq.hu/ffmpeg-doc.html)
Hmm.. i was trying not to do that; I missed this one.
I had gone back through and tried to undiff all cosmetic changes, but
obviously missed a few.
>
> [...]
>> + {
>> + int width, height;
>> +
>> + rtsp_get_word_sep(buf1, sizeof(buf1), "-", &p);
>> + width= atoi(buf1);
>> + height= atoi(p+1); // skip the -
>
> the width and height variables are unused
> [...]
>> @@ -1263,11 +1369,11 @@
>>
>> static int sdp_probe(AVProbeData *p1)
>> {
>> - const char *p = p1->buf, *p_end = p1->buf + p1->buf_size;
>> + const unsigned char *p = p1->buf, *p_end = p1->buf + p1-
>> >buf_size;
>>
>> /* we look for a line beginning "c=IN IP4" */
>> while (p < p_end && *p != '\0') {
>> - if (p + sizeof("c=IN IP4") - 1 < p_end && strstart(p,
>> "c=IN IP4", NULL))
>> + if (p + sizeof("c=IN IP4") - 1 < p_end && strstart((char
>> *) p, "c=IN IP4", NULL))
>> return AVPROBE_SCORE_MAX / 2;
>>
>> while(p < p_end - 1 && *p != '\n') p++;
>> @@ -1294,7 +1400,7 @@
>> /* read the whole sdp file */
>> /* XXX: better loading */
>> content = av_malloc(SDP_MAX_SIZE);
>> - size = get_buffer(&s->pb, content, SDP_MAX_SIZE - 1);
>> + size = get_buffer(&s->pb, (unsigned char *)content,
>> SDP_MAX_SIZE - 1);
>> if (size <= 0) {
>> av_free(content);
>> return AVERROR_INVALIDDATA;
>
> unrelated signed/unsigned type change
Yeah, i was just trying to remove the compiler warnings. Can submit
as separate patch.
> [...]
>> static int base64_decode(unsigned char in[], unsigned char out[],
>> int len)
>> {
>> int iLen = len;
>> int oLen = 0;
>>
>> if ((iLen & 3) == 0) {
>> int ip = 0;
>> int op = 0;
>>
>> while (iLen > 0 && in[iLen - 1] == '=')
>> iLen--;
>> //oLen= ((iLen*3)/4) + (((iLen*6)%8)?1:0); // this is the
>> correction to that java code.
>> oLen = (iLen * 3 + 3) >> 2;
>> // fprintf(stderr, "oLen: %d iLen: %d\n", oLen, iLen);
>>
>> while (ip < iLen) {
>> int i0 = in[ip++];
>> int i1 = in[ip++];
>> int i2 = ip < iLen ? in[ip++] : 'A';
>> int i3 = ip < iLen ? in[ip++] : 'A';
>> if ((i0 | i1 | i2 | i3) <= 127) {
>> int b0 = map2[i0];
>> int b1 = map2[i1];
>> int b2 = map2[i2];
>> int b3 = map2[i3];
>> if (b0 != 0xff && b1 != 0xff && b2 != 0xff && b3 !
>> = 0xff) {
>> int o0 = (b0 << 2) | (b1 >> 4); // these
>> were unsigned right shift.
>> int o1 = (b1 << 4) | (b2 >> 2); // these
>> were unsigned right shift in java
>> int o2 = (b2 << 6) | b3;
>> out[op++] = o0;
>> if (op < oLen)
>> out[op++] = o1;
>> if (op < oLen)
>> out[op++] = o2;
>> } else {
>> av_log(NULL, AV_LOG_ERROR,
>> "Ilegal character in base64 data!");
>> }
>> } else {
>> av_log(NULL, AV_LOG_ERROR,
>> "Ilegal character in base64 data!");
>> }
>> }
>> // fprintf(stderr, "OP: %d\n", op);
>> } else {
>> av_log(NULL, AV_LOG_ERROR, "Base64 must be a multiple of
>> 4!");
>> }
>>
>> return oLen;
>
> something like the following is simpler (but untested!), though
> slower but i
> think this is never used in any speed critical code
>
> static int base64_decode(uint8_t *out, uint8_t *in, int len)
> int i, v;
> for(i=0; i<len && in[i]!='='; i++){
> if(in[i] > 127 || map2[in[i]]==0xff)
> return -1;
> v= (v<<6) + map2[in[i]];
> if(i&3)
> *out++= v>>(6-2*(i&3));
> }
> }
I'll look at this; I wasn't too excited on spending time on it,
because like you noted it's not used in any speed critical code, and
what I had works (I didn't even look at the algorithm or think about
it, except to fix the java version to actually work right).
> [...]
>> } else if (strstart(p, "fmtp:", &p)) {
>> char attr[256];
>> char value[4096];
>>
>> /* loop on each attribute */
>> for (;;) {
>> rtsp_skip_spaces(&p);
>> if (*p == '\0')
>> break;
>> rtsp_get_word_sep(attr, sizeof(attr), "=", &p);
>> if (*p == '=')
>> p++;
>> rtsp_get_word_sep(value, sizeof(value), ";", &p);
>> if (*p == ';')
>> p++;
>> /* grab the codec extra_data from the config parameter
>> of the fmtp line */
>> sdp_parse_fmtp_config_h264(stream, h264_data, attr,
>> value);
>
> this code is duplicated from rtsp:sdp_parse_fmtp()
This is true, but I have to parse that stuff out becfore I can get to
the values, which are then used internally. Rather than have a
function pointer for parsing sdp streams, and a different function
pointer for parsing some of the other sdp lines, I figured it would
be better to have one function pointer for all sdp strings.
Unfortunately, it means that there would be slight duplication of
code (as per here). I could add something like a single function to
rtp.c that would give me the attr & values for this function; would
that be better?
So it sounds like my best solution (from all this) would be to submit
several incremental patches, instead of one monolithic one?
I'll try and break this up somewhat and submit as smaller patches.
Thanks!
-R
More information about the ffmpeg-devel
mailing list