[FFmpeg-soc] SVQ3 depacketizer

[Josh Allmann]

On 1 July 2010 00:47, Martin Storsjö <martin at 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?
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-depacketization-of-SVQ3.patch
Type: text/x-patch
Size: 8003 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100701/aaaf7e72/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Pad-the-buffer-in-url_close_dyn_buf.patch
Type: text/x-patch
Size: 1793 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100701/aaaf7e72/attachment-0001.bin>