[FFmpeg-devel] theora depayloader

[Josh Allmann]

On 20 March 2010 06:47, Martin Storsj? <martin at martin.st> wrote:
> On Sat, 20 Mar 2010, Josh Allmann wrote:
>
>> On 19 March 2010 21:19, Martin Storsj? <martin at martin.st> wrote:
>> > On Fri, 19 Mar 2010, Josh Allmann wrote:
>> >
>> >
>> >> + ? ? ? ? ? ?data_len += pkt_len;
>> >> + ? ? ? ? ? ?len -= pkt_len+2;
>> >> + ? ? ? }
>> >
>> > Here, you could potentially get the case where len reaches exactly zero,
>> > before i has reached num_pkts, so we'd get no warning even if the packet
>> > was invalid...
>>
>> Now that I think of it, a bad packet could potentially give us a large
>> negative pkt_len, which would in turn make len positive, but still
>> invalid... so I changed the type declaration of pkt_len (and related
>> header vars) to uint32_t.

+            pkt_len   = AV_RB16(buf + off);

Overflowing pkt_len is a non-issue since AV_RB16 returns uint16.

>> + ? ? ? ?// fast first pass to calculate total length
>> + ? ? ? ?for (i = 0, data_len = 0; ?(i < num_pkts) && (len >= 0); ?i++) {
>> + ? ? ? ? ? ?int off ? = data_len + (i << 1);
>> + ? ? ? ? ? ?pkt_len ? = AV_RB16(buf + off);
>> + ? ? ? ? ? ?data_len += pkt_len;
>> + ? ? ? ? ? ?len ? ? ?-= pkt_len + 2;
>> + ? ? ? ?}
>> +
>> + ? ? ? ?if (len < 0) {
>> + ? ? ? ? ? ?av_log(ctx, AV_LOG_ERROR, "Length overread by %d\n", -len);
>> + ? ? ? ? ? ?return AVERROR_INVALIDDATA;
>> + ? ? ? ?}
>
> If len == 0 or 1, you will read outside of the buffer (which isn't a
> padded buffer IIRC). What about this loop condition instead:
>
> for (...; i < num_pkts && len >= 2; i++)
>
> and check for errors with:
>
> if (len < 0 || i < num_pkts)
>
> That is, if we've errored out due to the length restriction, i will be
> left at less than num_pkts, and we haven't parsed everything => error.

Done.

>> + ? ?num_packed ? ? ? ? = bytestream_get_be32(&packed_headers);
>> + ? ?theora_data->ident = bytestream_get_be24(&packed_headers);
>> + ? ?length ? ? ? ? ? ? = bytestream_get_be16(&packed_headers);
>
> No checking if there's sufficient bytes to read - check e.g.
> packed_headers_end - packed_headers >= 9 before this.

Fixed.

>
>> +int ff_theora_parse_fmtp_config(AVCodecContext * codec,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?void *theora_data, char *attr, char *value)
>> +{
>> + ?int result = 0;
>> + ?assert(codec->id == CODEC_ID_THEORA);
>> + ?assert(theora_data);
>> +
>> + ?if (!strcmp(attr, "sampling")) {
>> + ? ?return AVERROR_PATCHWELCOME;
>> + ?} else if (!strcmp(attr, "width")) {
>
> The indentation seems to be off in the whole function

Fixed all indentation errors. Time to set my editor to 4-space indents
instead of 2...

>> + ? ?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...

*facepalm*  Separate patch with doxy attached.

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

Sounds about right, I kept the original length/255 rather than divide
by 255 twice to get an exact amount. We may over-allocate a little,
but shouldn't be by much. (length <= length1+length2)

+    /* allocate extra space:
+     * --length/255 +1 for xiphlacing
+     * -- one for the '2' marker
+     * -- FF_INPUT_BUFFER_PADDING_SIZE required */
+    extradata_alloc = length + length/255 + 2 + FF_INPUT_BUFFER_PADDING_SIZE;

>
> 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.

Done.

>> >> Why is that, btw? Does the Theora/Vorbis SDP files contain a=fmtp before
>> >> the corresponding a=rtpmap, or why is this any different from other
>> >> formats having fmtp lines?
>> >
>> > Josh/Ronald - anyone care to explain what differs in Theora/Vorbis SDP
>> > compared to other formats, which makes them need this? I'm curious. :-)
>>
>> I'll have to look it up. ;-). I'll try to test it ASAP and see why it
>> was the case again. :-).
>
> To me it seems that we pass the same rtsp_st->dynamic_protocol_context to
> sdp_parse_fmtp_config that we would have passed to parse_sdp_a_line, so
> it doesn't seem tobe the case that the dynamic payload handler isn't set
> up yet. (Otherwise, ff_theora_parse_fmtp_config wouldn't have anywhere to
> store the parsed results, right?)

I finally got the time to look into this, and implemented the
parse_sdp_a_line function for theora.

I looked over last year's reviews of the vorbis depayloader*, and it
originally used a dynamic payload handler. The original patch was
modified to share code with rtsp.c, but since you guys seem to favor
otherwise, theora uses a dynamic payload handler now.

The fmtp parsers in theora/amr/h264 are starting to look a bit
boilerplate-ish, but they're also nicely decoupled from rtsp.c.
Probably still a good refactoring target though.

*http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-April/thread.html#67879

Great review, thanks.
Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: theora_depayloader.diff
Type: text/x-patch
Size: 16077 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100322/144175df/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: xiphlacing_doxy.diff
Type: text/x-patch
Size: 537 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100322/144175df/attachment-0001.bin>