[FFmpeg-soc] SVQ3 depacketizer

[Martin Storsjö]

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