[Ffmpeg-devel] RTP patches & RFC
Michael Niedermayer
michaelni
Fri Oct 27 00:30:09 CEST 2006
Hi
On Thu, Oct 26, 2006 at 04:29:20PM -0500, Ryan Martell wrote:
> Okay, and now back to the original patch.
>
> This enables the h264 stream stuff; Michael, you have looked at a lot
> of this code before.
>
> The big block of stuff that is moved in rtp.c relating to AAC is just
> moved up from a lower switch statement, otherwise it's unchanged.
can this be applied seperatly without breaking the code? if so submit
a seperate patch
> This also has the added benefit of fixing a memory leak where
> av_new_packet was being called twice on the same packet with AAC.
>
> This works great on my stream, I'm working to make one available for
> (limited) testing. With this patch, and my stream, the audio is out
> of whack, but that's because faac upconverts the frequency and
> converts from mono to stereo. I have the frequency and channels
> hardcoded on my working copy, which fixes it temporarily. There will
> be another patch forthcoming to fix that, once i figure out the best
> way to handle it.
>
> After this one gets in, I'll add the statistics stuff, since
> otherwise you will be kicked off after 2 minutes on Darwin Streaming
> server if you are connected via UDP.
ok, IMHO first before anything from the patch below can be applied, the
indention of the current code (which was broken by your last patch) must
be fixed in a seperate patch
[...]
> Index: libavformat/rtp.c
> ===================================================================
> --- libavformat/rtp.c (revision 6798)
> +++ libavformat/rtp.c (working copy)
[...]
> @@ -364,14 +364,18 @@
> {
> unsigned int ssrc, h;
> int payload_type, seq, delta_timestamp, ret;
> - AVStream *st;
> + AVStream *st= s->st;
> uint32_t timestamp;
> + int rv= 0;
>
> if (!buf) {
> /* return the next packets, if any */
> if(s->st && s->parse_packet) {
> - return s->parse_packet(s, pkt, 0, NULL, 0);
> + timestamp= 0; ///< Should not be used if buf is NULL, but should be set to the timestamp of the packet returned....
> + rv= s->parse_packet(s, pkt, ×tamp, NULL, 0);
> + goto calculate_timestamp;
iam against gotos unless they are really needed (=please explain why
that goto cannot be avoided)
> } else {
> + // TODO: Move this into a parse_packet proc...
yes iam very strongly in favor of that, the 2 class system is ugly
[...]
> void rtp_parse_close(RTPDemuxContext *s)
> Index: libavformat/rtp_internal.h
> ===================================================================
> --- libavformat/rtp_internal.h (revision 6798)
> +++ libavformat/rtp_internal.h (working copy)
> @@ -23,7 +23,7 @@
>
> typedef int (*DynamicPayloadPacketHandlerProc) (struct RTPDemuxContext * s,
> AVPacket * pkt,
> - uint32_t timestamp,
> + uint32_t *timestamp,
> const uint8_t * buf,
> int len);
>
> @@ -82,5 +82,7 @@
> };
>
> extern RTPDynamicProtocolHandler *RTPFirstDynamicPayloadHandler;
> +
> +int rtsp_next_attr_and_value(const char **p, char *attr, int attr_size, char *value, int value_size); ///< from rtsp.c, but used in dynamic packet handlers
> #endif /* RTP_INTERNAL_H */
>
> Index: libavformat/rtsp.c
> ===================================================================
> --- libavformat/rtsp.c (revision 6798)
> +++ libavformat/rtsp.c (working copy)
> @@ -200,6 +200,8 @@
> i = atoi(buf);
> if (i > 0)
> codec->channels = i;
> + // TODO: there is a bug here; if it is a mono stream, and less than 22000Hz, faad upconverts to stereo and twice the
> + // frequency. No problem, but the sample rate is being set here by the sdp line. Upcoming patch forthcoming. (rdm)
> }
> av_log(codec, AV_LOG_DEBUG, " audio samplerate set to : %i\n", codec->sample_rate);
> av_log(codec, AV_LOG_DEBUG, " audio channels set to : %i\n", codec->channels);
> @@ -287,6 +289,22 @@
> {NULL, -1, -1},
> };
>
> +int rtsp_next_attr_and_value(const char **p, char *attr, int attr_size, char *value, int value_size)
> +{
> + skip_spaces(p);
> + if(**p)
> + {
> + get_word_sep(attr, attr_size, "=", p);
> + if (**p == '=')
> + (*p)++;
> + get_word_sep(value, value_size, ";", p);
> + if (**p == ';')
> + (*p)++;
> + return 1;
> + }
> + return 0;
> +}
> +
> /* parse a SDP line and save stream attributes */
> static void sdp_parse_fmtp(AVStream *st, const char *p)
> {
> @@ -298,7 +316,8 @@
> AVCodecContext *codec = st->codec;
> rtp_payload_data_t *rtp_payload_data = &rtsp_st->rtp_payload_data;
>
> - /* loop on each attribute */
> + // TODO (Replace with rtsp_next_attr_and_value)
> + /* loop on each attribute */
> for(;;) {
> skip_spaces(&p);
> if (*p == '\0')
> @@ -471,6 +490,19 @@
> }
> }
> }
> + } else if(strstart(p, "framesize:", &p)) {
> + // let dynamic protocol handlers have a stab at the line.
> + get_word(buf1, sizeof(buf1), &p);
> + payload_type = atoi(buf1);
> + for(i = 0; i < s->nb_streams;i++) {
> + st = s->streams[i];
> + rtsp_st = st->priv_data;
> + if (rtsp_st->sdp_payload_type == payload_type) {
> + if(rtsp_st->dynamic_handler && rtsp_st->dynamic_handler->parse_sdp_a_line) {
> + rtsp_st->dynamic_handler->parse_sdp_a_line(st, rtsp_st->dynamic_protocol_context, buf);
> + }
> + }
> + }
> }
> break;
> }
many of the things above look quite generic and not h.264 specific, they
could be split into seperate patch(es)
[...]
> /**
> Internal h264_packet structure.
> Used for reordering of packets, for accumulation of packets, and general maintenance prior to passing them on to an AVPacket
> */
> typedef struct h264_packet {
> uint8_t *data; ///< The data. (duh!)
> int length; ///< Actual data size
> int maximum_length; ///< The size of the data allocated...
> uint64_t timestamp; ///< this is the rtp timestamp; only needs 32 bits of precision.
>
> struct h264_packet *next;
> } h264_packet;
>
> /**
> Generic packet queue for h264 packets.
> */
> typedef struct packet_queue {
> int packet_header_length; ///< needed for looking for packets of a given type...
> struct h264_packet *first_packet;
> struct h264_packet *last_packet;
> } packet_queue;
why dont you use AVPacketList?
>
> /**
> RTP/H264 specific private data.
> */
> typedef struct h264_rtp_extra_data {
> unsigned long cookie; ///< sanity check, to make sure we get the pointer we're expecting.
>
> struct packet_queue network_packets; ///< network packets are in this list...
> struct packet_queue frame_packets; ///< linked list of all the packets with the same timestamp.
> struct packet_queue out_of_band_packets; ///< pps and sps... (trasmitted via sdp)
> struct packet_queue partial_packets; ///< fragmentation unit packets.
> struct packet_queue packet_pool; ///< preallocated packet pool; we get them from here first if we need them...
one thing ive been curious about is why does h.264 need this mess while the
other codecs dont? what is the problem with just removing the extra headers
adding the 001 startcode prefixes and then passing the packets through the
AVParser? are the packets out of order in some way or not? maybe my question
is stupid but i plain dont understand why this complex buffering system is
needed for h.264 ...
[...]
> /* ---------------- private code */
> // ported from java (LGPL implementation at http://www.source-code.biz/snippets/java/Base64Coder.java.txt) (but it was wrong. joy.)
> // static uint8_t map1[] = { "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/" };
> static uint8_t map2[] =
> { 0x40, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> 0xff, 0xff,
> 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> 0xff, 0xff,
> 0xff, 0xff, 0xff, 0xff, 0x3e, 0xff, 0xff, 0xff, 0x3f, 0x34, 0x35, 0x36,
> 0x37, 0x38,
> 0x39, 0x3a, 0x3b, 0x3c, 0x3d, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> 0x00, 0x01,
> 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
> 0x0e, 0x0f,
> 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0xff, 0xff,
> 0xff, 0xff,
> 0xff, 0xff, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23,
> 0x24, 0x25,
> 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x31,
> 0x32, 0x33,
> 0xff, 0xff, 0xff, 0xff, 0xff
> };
>
> 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;
> // av_log(NULL, AV_LOG_ERROR, "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!");
> }
> }
> // av_log(NULL, AV_LOG_ERROR, "OP: %d\n", op);
> } else {
> av_log(NULL, AV_LOG_ERROR, "Base64 must be a multiple of 4!");
> }
>
> return oLen;
> }
base64_decode() should be in a seperate patch
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list