Hi guys, Here is the depacketizer for SVQ3 that Ronald wrote some time ago. To test, you'll need to disable audio until we get QDM2 merged -- thanks Martin for the tip. (s1->skip_media = 1; return; in line 272 of rtsp.c, and add "-an" to your ffmpeg/ffplay parameters). I also updated the Changelog entry with a reference to RTSP-HTTP. Josh
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 <rbultje@ronald.bitfreak.net> + */
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
On Wed, 30 Jun 2010, Martin Storsjö wrote:
+ /* 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.)
A 160x120 SVQ3 stream seems to have config 0, and a 160x128 file had config 7 with a custom size, so I guess this issue is resolved then. This also matches the code in libavcodec/svq3.c. // Martin
On Wed, 30 Jun 2010, Martin Storsjö wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+ 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.
Not only bad or weird data - these config packets are sent before each keyframe (which makes sense, so we're not totally screwed if we miss one, we just have to wait for the next one), so we'd be accumulating leaks here.
+ 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.
Ah, yes, as I mentioned in the status mail comments yesterday (but forgot when I reviewed this, but valgrind reminded me) - you need to add padding at the end of the packets, e.g. put_buffer with FF_INPUT_BUFFER_PADDING_SIZE zero bytes, then subtract the same amount from the size returned by url_close_dyn_buf. For this, I guess it would be handy with a put_zero function (or similar) that writes a certain amount of zero bytes, so we wouldn't have to allocate an array just to get some padding to write. Or perhaps we should just loop with put_byte(, 0)? // Martin
Hi, On Wed, Jun 30, 2010 at 4:45 AM, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Martin Storsjö wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+ 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.
Not only bad or weird data - these config packets are sent before each keyframe (which makes sense, so we're not totally screwed if we miss one, we just have to wait for the next one), so we'd be accumulating leaks here.
+ 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.
Ah, yes, as I mentioned in the status mail comments yesterday (but forgot when I reviewed this, but valgrind reminded me) - you need to add padding at the end of the packets, e.g. put_buffer with FF_INPUT_BUFFER_PADDING_SIZE zero bytes, then subtract the same amount from the size returned by url_close_dyn_buf.
For this, I guess it would be handy with a put_zero function (or similar) that writes a certain amount of zero bytes, so we wouldn't have to allocate an array just to get some padding to write. Or perhaps we should just loop with put_byte(, 0)?
Generic problem with the dyn_buf API. I'd say, please fix close_dyn_buf() to automatically add padding to the returned buffer, that's much more user-friendly. Ronald
On Wed, 30 Jun 2010, Ronald S. Bultje wrote:
On Wed, Jun 30, 2010 at 4:45 AM, Martin Storsjö <martin@martin.st> wrote:
+ 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.
Ah, yes, as I mentioned in the status mail comments yesterday (but forgot when I reviewed this, but valgrind reminded me) - you need to add padding at the end of the packets, e.g. put_buffer with FF_INPUT_BUFFER_PADDING_SIZE zero bytes, then subtract the same amount from the size returned by url_close_dyn_buf.
For this, I guess it would be handy with a put_zero function (or similar) that writes a certain amount of zero bytes, so we wouldn't have to allocate an array just to get some padding to write. Or perhaps we should just loop with put_byte(, 0)?
Generic problem with the dyn_buf API. I'd say, please fix close_dyn_buf() to automatically add padding to the returned buffer, that's much more user-friendly.
True, that would be a neat way of handling it. It isn't needed in all cases where one uses dyn buffers though, so I'm not sure if one may want to skip doing it in those cases, or just always add padding to dyn buffers, to keep the API small and concise. // Martin
Hi, On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file libavformat/rtpdec_svq3.c + * @brief RTP support for the SV3V (SVQ3) payload + * @author Ronald S. Bultje <rbultje@ronald.bitfreak.net> + */
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.
No RFC, this is RE'ed stuff. :-(.
+/** 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.
Depends on spreading. We return 1 if the RTP packet contained multiple demuxer packets, e.g. two video frames. What happens here is that >=2 RTP packets contain 1 video frame, so we don't have any data after 1 RTP packet (partial packet) and thus return -1.
+ 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.
buf[1] is probably padding for future flags extension, which of course never happened. The binary didn't use buf[1].
+ /* 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;
This whole block can be removed (I removed it locally later, but never resubmitted), because the decoder does this for us. Ronald
On Wed, 30 Jun 2010, Ronald S. Bultje wrote:
On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file libavformat/rtpdec_svq3.c + * @brief RTP support for the SV3V (SVQ3) payload + * @author Ronald S. Bultje <rbultje@ronald.bitfreak.net> + */
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.
No RFC, this is RE'ed stuff. :-(.
I figured that out after googling a bit, but to save me from googling next time, it may be good to add a notice that there's no RFC.
+/** 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.
Depends on spreading. We return 1 if the RTP packet contained multiple demuxer packets, e.g. two video frames. What happens here is that >=2 RTP packets contain 1 video frame, so we don't have any data after 1 RTP packet (partial packet) and thus return -1.
Ah, yeah, well even when referring to that, the comment which says "<1 on partial packet or error" should be "<0" of course.
+ 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.
buf[1] is probably padding for future flags extension, which of course never happened. The binary didn't use buf[1].
Ok, if there's no RFC, this is a very good explanation indeed. :-)
+ 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;
This whole block can be removed (I removed it locally later, but never resubmitted), because the decoder does this for us.
Yeah, I was about to suggest that, too. // Martin
Hi, On 30 June 2010 09:53, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Ronald S. Bultje wrote:
On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file libavformat/rtpdec_svq3.c + * @brief RTP support for the SV3V (SVQ3) payload + * @author Ronald S. Bultje <rbultje@ronald.bitfreak.net> + */
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.
No RFC, this is RE'ed stuff. :-(.
I figured that out after googling a bit, but to save me from googling next time, it may be good to add a notice that there's no RFC.
Removed the filename from the @file directive, and added a note indicating this depacketizer is RE'ed.
+/** 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.
Depends on spreading. We return 1 if the RTP packet contained multiple demuxer packets, e.g. two video frames. What happens here is that >=2 RTP packets contain 1 video frame, so we don't have any data after 1 RTP packet (partial packet) and thus return -1.
Ah, yeah, well even when referring to that, the comment which says "<1 on partial packet or error" should be "<0" of course.
Yeah, my bad; the original comment Ronald wrote was correct and I screwed it up while editing.
+ 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.
buf[1] is probably padding for future flags extension, which of course never happened. The binary didn't use buf[1].
Ok, if there's no RFC, this is a very good explanation indeed. :-)
Comment added indicating this.
+ 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;
This whole block can be removed (I removed it locally later, but never resubmitted), because the decoder does this for us.
Yeah, I was about to suggest that, too.
Removed, but there's still the same problem as QDM2 -- extradata in RTP rather than SDP -- so to delay decoder initialization, I used the CODEC_ID_NONE trick that Ronald proposed. Tested on all the SVQ3 samples that Martin provided. Works well, and I didn't get any "slice after bitstream end" errors. Those were probably artifacts from the slight broken-ness of the last patch. Also attached is a patch that adds FF_INPUT_BUFFER_PADDING_SIZE zero bytes to every url_close_dyn_buf call. QDM2 patch forthcoming; I'll make a new thread for that. Josh
On Wed, 30 Jun 2010, Josh Allmann wrote:
Removed, but there's still the same problem as QDM2 -- extradata in RTP rather than SDP -- so to delay decoder initialization, I used the CODEC_ID_NONE trick that Ronald proposed.
Tested on all the SVQ3 samples that Martin provided. Works well, and I didn't get any "slice after bitstream end" errors. Those were probably artifacts from the slight broken-ness of the last patch.
Also attached is a patch that adds FF_INPUT_BUFFER_PADDING_SIZE zero bytes to every url_close_dyn_buf call.
+/** + * @file + * @brief RTP support for the SV3V (SVQ3) payload (RE'ed)
I think this could afford a few more words for clarity, e.g. "no RFC, reverse engineered".
+ + if (config_packet) { + + if (sv->timestamp) + av_free(st->codec->extradata);
Umm, why the check? If some extradata already exists, it should be freed here, if not, av_free() does nothing, so the check can just as well be left out. Also, av_freep() would be even better, since if we'd happen to be in the len < 2 case, we'd leave the freed pointer dangling.
+ +static PayloadContext * +sv3v_extradata_new(void) +{
Why the weird line breaking here, instead of just keeping it all on one line? Also, if writing it on one line, the * belongs next to the function name.
+RTPDynamicProtocolHandler ff_svq3_dynamic_handler = { + "X-SV3V-ES", + CODEC_TYPE_VIDEO, + CODEC_ID_NONE,
A comment explaining why the codec id is none would very much be needed here.
+ NULL, // parse sdp line + sv3v_extradata_new, + sv3v_extradata_free, + sv3v_parse_packet, +};
The file is named _svq3, the dynamic handler struct is named ff_svq3_, but the internal functions are named sv3v, this feels a little inconsistent. I'd prefer one single naming used throughout the file, unless Ronald has a good explanation of why this is better.
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 8684903..8225639 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -893,7 +893,10 @@ int url_open_dyn_packet_buf(ByteIOContext **s, int max_packet_size) int url_close_dyn_buf(ByteIOContext *s, uint8_t **pbuffer) { DynBuffer *d = s->opaque; - int size; + int size, i; + + for (i = 0; i < FF_INPUT_BUFFER_PADDING_SIZE; i++) + put_byte(s, 0);
This is not right. If we implicitly pad the buffer, the size returned for the buffer should be of the data that the caller had fed in, not including the padding as it does now. Also, for buffers opened with url_open_dyn_packet_buf, this is not ok, only for those opened with url_open_dyn_buf. When using url_open_dyn_packet_buf, the data buffer has framing for the individual packets, which would expose the padding data as part of one of the packets, which is not what we want, and which would break many things. Also, documentation should be added to url_open_dyn_buf saying that padding is added and that the caller can rely on it. If not, the depacketizer shouldn't rely on it being done either. Except for that, Michael, are you ok with adding implicit padding at the end for all buffers opened with url_open_dyn_buf? // Martin
On 1 July 2010 00:47, Martin Storsjö <martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file + * @brief RTP support for the SV3V (SVQ3) payload (RE'ed)
I think this could afford a few more words for clarity, e.g. "no RFC, reverse engineered".
Added a link to the multimediawiki page per Luca's request.
+ + if (config_packet) { + + if (sv->timestamp) + av_free(st->codec->extradata);
Umm, why the check? If some extradata already exists, it should be freed here, if not, av_free() does nothing, so the check can just as well be left out. Also, av_freep() would be even better, since if we'd happen to be in the len < 2 case, we'd leave the freed pointer dangling.
Removed the check and changed to av_freep.
+ +static PayloadContext * +sv3v_extradata_new(void) +{
Why the weird line breaking here, instead of just keeping it all on one line? Also, if writing it on one line, the * belongs next to the function name.
Fixed.
+RTPDynamicProtocolHandler ff_svq3_dynamic_handler = { + "X-SV3V-ES", + CODEC_TYPE_VIDEO, + CODEC_ID_NONE,
A comment explaining why the codec id is none would very much be needed here.
+ NULL, // parse sdp line + sv3v_extradata_new, + sv3v_extradata_free, + sv3v_parse_packet, +};
Fixed.
The file is named _svq3, the dynamic handler struct is named ff_svq3_, but the internal functions are named sv3v, this feels a little inconsistent. I'd prefer one single naming used throughout the file, unless Ronald has a good explanation of why this is better.
Fixed.
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 8684903..8225639 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -893,7 +893,10 @@ int url_open_dyn_packet_buf(ByteIOContext **s, int max_packet_size) int url_close_dyn_buf(ByteIOContext *s, uint8_t **pbuffer) { DynBuffer *d = s->opaque; - int size; + int size, i; + + for (i = 0; i < FF_INPUT_BUFFER_PADDING_SIZE; i++) + put_byte(s, 0);
This is not right. If we implicitly pad the buffer, the size returned for the buffer should be of the data that the caller had fed in, not including the padding as it does now.
D'oh, you mentioned that in an earlier email. Fixed. Also changed to the style preferred by Ronald.
Also, for buffers opened with url_open_dyn_packet_buf, this is not ok, only for those opened with url_open_dyn_buf. When using url_open_dyn_packet_buf, the data buffer has framing for the individual packets, which would expose the padding data as part of one of the packets, which is not what we want, and which would break many things.
Fixed.
Also, documentation should be added to url_open_dyn_buf saying that padding is added and that the caller can rely on it. If not, the depacketizer shouldn't rely on it being done either.
Done.
Except for that, Michael, are you ok with adding implicit padding at the end for all buffers opened with url_open_dyn_buf?
On Thu, 1 Jul 2010, Josh Allmann wrote:
[lots of things fixed]
Looks good to me, both of them. The long line in the doxy at the beginning could be wrapped, and if we'd want to be really pedantic, we perhaps should set extradata_size to 0 after av_freep(), since otherwise we could end up with extradata == NULL, but extradata_size > 0 in some situations, which perhaps could make other codepaths crash. But no need to resend another patch with those changes now, I'll make the changes when I apply them. Luca, Ronald, any further comments? // Martin
Hi, On Thu, Jul 1, 2010 at 3:46 PM, Martin Storsjö <martin@martin.st> wrote:
Luca, Ronald, any further comments?
No, looks good to me. Ronald
On Thu, 1 Jul 2010, Martin Storsjö wrote:
On Thu, 1 Jul 2010, Josh Allmann wrote:
[lots of things fixed]
Looks good to me, both of them. The long line in the doxy at the beginning could be wrapped, and if we'd want to be really pedantic, we perhaps should set extradata_size to 0 after av_freep(), since otherwise we could end up with extradata == NULL, but extradata_size > 0 in some situations, which perhaps could make other codepaths crash.
But no need to resend another patch with those changes now, I'll make the changes when I apply them.
Actually, the url_close_dyn_buf fix needs a minor adjustment, as in the attached patch, I'll squash that in when applying. (I see you didn't valgrind your code this time. ;P) // Martin
On Thu, 1 Jul 2010, Martin Storsjö wrote:
On Thu, 1 Jul 2010, Martin Storsjö wrote:
On Thu, 1 Jul 2010, Josh Allmann wrote:
[lots of things fixed]
Looks good to me, both of them. The long line in the doxy at the beginning could be wrapped, and if we'd want to be really pedantic, we perhaps should set extradata_size to 0 after av_freep(), since otherwise we could end up with extradata == NULL, but extradata_size > 0 in some situations, which perhaps could make other codepaths crash.
But no need to resend another patch with those changes now, I'll make the changes when I apply them.
Actually, the url_close_dyn_buf fix needs a minor adjustment, as in the attached patch, I'll squash that in when applying. (I see you didn't valgrind your code this time. ;P)
All applied, good job! // Martin
On Thu, Jul 01, 2010 at 10:47:16AM +0300, Martin Storsjö wrote: [...]
Except for that, Michael, are you ok with adding implicit padding at the end for all buffers opened with url_open_dyn_buf?
yes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
Hi, On Wed, Jun 30, 2010 at 11:01 PM, Josh Allmann <joshua.allmann@gmail.com> wrote: [..]
@@ -893,7 +893,10 @@ int url_open_dyn_packet_buf(ByteIOContext **s, int max_packet_size) int url_close_dyn_buf(ByteIOContext *s, uint8_t **pbuffer) { DynBuffer *d = s->opaque; - int size; + int size, i; + + for (i = 0; i < FF_INPUT_BUFFER_PADDING_SIZE; i++) + put_byte(s, 0);
put_flush_packet(s);
static const padbuf[FF_INPUT_BUFFER_PADDING_SIZE] = { 0 }; put_buffer(s, padbuf, sizeof(padbuf)); Ronald
On 6/30/10 6:38 PM, Ronald S. Bultje wrote:
Hi,
On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö<martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file libavformat/rtpdec_svq3.c + * @brief RTP support for the SV3V (SVQ3) payload + * @author Ronald S. Bultje<rbultje@ronald.bitfreak.net> + */
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.
No RFC, this is RE'ed stuff. :-(.
then it should be put in the multimedia wiki and we should reference it
+/** 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.
Depends on spreading. We return 1 if the RTP packet contained multiple demuxer packets, e.g. two video frames. What happens here is that>=2 RTP packets contain 1 video frame, so we don't have any data after 1 RTP packet (partial packet) and thus return -1.
Doesn't sound nice if <0 is also used for errors. lu
On Thu, 1 Jul 2010, Luca Barbato wrote:
On 6/30/10 6:38 PM, Ronald S. Bultje wrote:
Hi,
On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö<martin@martin.st> wrote:
On Wed, 30 Jun 2010, Josh Allmann wrote:
+/** + * @file libavformat/rtpdec_svq3.c + * @brief RTP support for the SV3V (SVQ3) payload + * @author Ronald S. Bultje<rbultje@ronald.bitfreak.net> + */
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.
No RFC, this is RE'ed stuff. :-(.
then it should be put in the multimedia wiki and we should reference it
+/** 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.
Depends on spreading. We return 1 if the RTP packet contained multiple demuxer packets, e.g. two video frames. What happens here is that>=2 RTP packets contain 1 video frame, so we don't have any data after 1 RTP packet (partial packet) and thus return -1.
Doesn't sound nice if <0 is also used for errors.
It's no problem actually, it uses AVERROR(EAGAIN) for these cases, which most calling code should handle just fine. // Martin
participants (5)
-
Josh Allmann -
Luca Barbato -
Martin Storsjö -
Michael Niedermayer -
Ronald S. Bultje