[FFmpeg-devel] theora depayloader

Martin Storsjö martin
Sat Mar 20 11:36:44 CET 2010


On Sat, 20 Mar 2010, Josh Allmann wrote:

> On 19 March 2010 21:21, Martin Storsj? <martin at martin.st> wrote:
> > On Fri, 19 Mar 2010, Josh Allmann wrote:
> >
> >> On 19 March 2010 05:23, Martin Storsj? <martin at martin.st> wrote:
> >> >
> >> > On Fri, 19 Mar 2010, Josh Allmann wrote:
> >> >
> >> >
> >> >> + ? ?ptr = codec->extradata = av_mallocz(length + length / 255 + 64);
> >> >
> >> > Doesn't extradata need FF_INPUT_BUFFER_PADDING_SIZE at the end?
> >>
> >> I'm not sure, but since extradata is identical to vorbis, I'll
> >> investigate that when I refactor out common code.
> >
> > Let me rephrase - if the vorbis code doesn't add padding to the extradata,
> > it's a bug, no need to replicate it here. The vorbis code can be fixed
> > and/or merged with parts of this later, but don't add more incorrectness
> > just for uniformness :-)
> 
> +    ptr = codec->extradata = av_mallocz(length + FF_INPUT_BUFFER_PADDING_SIZE);
> 
> This version seems to work ok. Although I honestly don't know what the
> old (length/255+64) padding did -- I don't see anything in the
> (vorbis) rfc about it, and nothing from a cursory examination of the
> feng payloaders. The length field alone should cover everything.


> +    ptr = codec->extradata = av_mallocz(length + FF_INPUT_BUFFER_PADDING_SIZE);
> +    if (!ptr) {
> +        av_log(codec, AV_LOG_ERROR, "Out of memory");
> +        return AVERROR_NOMEM;
> +    }
> +    *ptr++ = 2;
> +    ptr += av_xiphlacing(ptr, length1);
> +    ptr += av_xiphlacing(ptr, length2);
> +    memcpy(ptr, packed_headers, length);
> +    ptr += length;
> +    codec->extradata_size = ptr - codec->extradata;

Using the 'length' field isn't totally correct - you write 1 + (a few 
bytes for length1) + (a few bytes for length2) + length bytes, so you're 
off by a few bytes.

A quick look at the av_xiphlacing function shows that it writes (n/255) + 
1 bytes to the pointer. What about a patch adding doxy for this function, 
stating this? The caller has to know how much space to allocate for it...

So a wild guess on the exact amount of bytes written would be 1 + 
length1/255 + 1 + length2/255 + 1 + length - is this correct?

Also, you first zero-initialize the whole buffer and then overwrite most 
of it with new content. You could choose not to zero-initialize and just 
memset() the padding to zero - but that would require you to know exactly 
how many bytes you've written before that, so you don't write outside of 
the allocated space.

// Martin



More information about the ffmpeg-devel mailing list