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?