On Wed, 30 Jun 2010, Josh Allmann wrote: > diff --git a/Changelog b/Changelog > index bde39d3..5549287 100644 > --- a/Changelog > +++ b/Changelog > @@ -15,7 +15,8 @@ version <next>: > - libfaad2 wrapper removed > - DTS-ES extension (XCh) decoding support > - native VP8 decoder > - > +- RTSP tunneling over HTTP > +- RTP depacketization of SVQ3 >
Added a mention of the RTSP tunneling in a separate commit now. Try to preserve the existing empty lines when adding things here. > +/** > + * @file libavformat/rtpdec_svq3.c > + * @brief RTP support for the SV3V (SVQ3) payload > + * @author Ronald S. Bultje <[email protected]> > + */ Lately, we've been instructed to remove the file names from these @file directives, and just keep "@file". Also, it would be good to mention which RFC this implements, to make it easier for the reader to look up the details. > +#include <string.h> > +#include <libavcodec/get_bits.h> > +#include "rtp.h" > +#include "rtpdec.h" > +#include "rtpdec_svq3.h" > + > +struct PayloadContext { > + ByteIOContext *pktbuf; > + int64_t timestamp; > + int is_keyframe; > +}; > + > +/** return 0 on packet, <1 on partial packet or error... */ Isn't this comment a bit wrong? Iirc, return 0 on packet, 1 on partial packets (that is, more packets can be returned), <0 on error. When checking other depacketizers, others seem to have some kind of confusion, too. > +static int sv3v_parse_packet (AVFormatContext *s, PayloadContext *sv, > + AVStream *st, AVPacket *pkt, > + uint32_t *timestamp, > + const uint8_t *buf, int len, int flags) > +{ > + int config_packet, start_packet, end_packet; > + > + if (len < 2) > + return AVERROR_INVALIDDATA; > + > + config_packet = buf[0] & 0x40; > + start_packet = buf[0] & 0x20; > + end_packet = buf[0] & 0x10; > + buf += 2; > + len -= 2; What's in buf[1], that we apparently ignore? :-) I'm not sure if a comment describing it here is necessary, a RFC reference somewhere would be enough so that I'd find it easily. > + if (config_packet) { > + GetBitContext gb; > + int config; > + > + if (len < 2 || !(st->codec->extradata = av_malloc(len + 8 + > FF_INPUT_BUFFER_PADDING_SIZE))) > + return AVERROR_INVALIDDATA; Do av_freep() of extradata before allocating new data, otherwise we'd leak memory if we'd receive bad or otherwise weird data. Also, try to keep lines below 80 chars if easily possible. > + /* frame size code */ > + init_get_bits(&gb, buf, len << 3); > + config = get_bits(&gb, 3); > + switch (config) { > + case 0: st->codec->width = 160; st->codec->height = 128 /* > FIXME: Wiki says 120? */; break; Hmm, I guess I should try to generate files with any of these dimensions and see if it uses them then, to get clarity in 128 vs 120. (120 sounds much more natural when combined with 160, though.) > + case 1: st->codec->width = 128; st->codec->height = 96; break; > + case 2: st->codec->width = 176; st->codec->height = 144; break; > + case 3: st->codec->width = 352; st->codec->height = 288; break; > + case 4: st->codec->width = 704; st->codec->height = 576; break; > + case 5: st->codec->width = 240; st->codec->height = 180; break; > + case 6: st->codec->width = 320; st->codec->height = 240; break; > + case 7: /**< in case of '111', the next 2x12 bits carry optional > width/height */ > + st->codec->width = get_bits(&gb, 12); > + st->codec->height = get_bits(&gb, 12); > + break; > + } > + st->codec->extradata_size = len + 6; Why is this len + 6, when you obviously write len + 8 bytes into the extradata? > + memcpy(st->codec->extradata, "SEQH", 4); > + AV_WB32(st->codec->extradata + 4, len); > + memcpy(st->codec->extradata + 8, buf, len); > + > + return AVERROR(EAGAIN); > + } > + if (end_packet) { > + av_init_packet(pkt); > + pkt->stream_index = st->index; > + pkt->pts = sv->timestamp; > + pkt->flags = sv->is_keyframe ? AV_PKT_FLAG_KEY : 0; > + pkt->size = url_close_dyn_buf(sv->pktbuf, &pkt->data); Neat, this is exactly what I meant, that could be used to save some memcpys for the xiph depacketizer, too. But this lacks a pkt->destruct = av_destruct_packet - at the moment, you're leaking every packet. Except for these comments, it looks quite good. I'll try to create samples with different sizes with quicktime and see what the config packet looks like. // Martin _______________________________________________ FFmpeg-soc mailing list [email protected] https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
