[FFmpeg-devel] theora depayloader

[Martin Storsjö]

Hi Josh,

Now this is starting to look very good indeed. :-) What's left is mostly 
nitpicks.

On Mon, 22 Mar 2010, Josh Allmann wrote:

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

Not quite all of them.

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

xiphlacing_doxy.diff looks good to me, but someone with more knowledge on 
this stuff should probably comment on it, too (and apply it if they're ok 
with 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?
> 
> 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)

> Erm, I meant 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;

I'm ok with this.

Although, if I'd be pedantic, I'd point out that even if length >= 
length1+length2, you should count length/255 + 2 for the xiphlacing, since 
each invocation gives you +1. :-)

But anyway - the code is quite good now, if the simplified calculation 
were to be off by a byte or two, the only thing that happens is that 
there's a few bytes less padding.

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

Ah, I see. Ronald, any opinion on which way to go for this one?

The code duplication argument still holds, but nevertheless, there should 
be as little format-specific code as possible in rtsp.c. A patch for 
refactoring the fmtp parsers would be very much welcome. Likewise for 
fixing the extradata padding things that we've addressed here.

As long as it actually is fixed later, it doesn't matter much which way it 
is done now.


What's left (in my opinion) is mostly a few cosmetic issues:

> +        if (len < 0 || i < num_pkts) {
> +            av_log(ctx, AV_LOG_ERROR,
> +            "Bad packet: %d bytes left at frame %d of %d\n", len, i, num_pkts);
> +            return AVERROR_INVALIDDATA;

Align "Bad packet..." with ctx instead of with av_log

> +        // concatenate frames
> +        for (i = 0, write_len = 0; write_len < data_len; i++) {
> +            pkt_len = AV_RB16(buf);
> +            buf += 2;
> +            memcpy(pkt->data+write_len, buf, pkt_len);

Space around the + operator wouldn't hurt

> +    } else if (fragmented == 1) {
> +        // start of theora data fragment
> +        int res;
> +        assert(!num_pkts);

Hmm, doesn't this depend on the actual data of the packet? That is, this 
assert could be triggered by a bad packet, and therefore should be a 
warning/error with a suitable return, instead of an assert.

> +static int theora_parse_fmtp_pair(AVCodecContext * codec,
> +                           PayloadContext *theora_data,
> +                           char *attr, char *value)

The indentation is off

> +{
> +    int result = 0;
> +
> +    if (!strcmp(attr, "sampling")) {
> +    return AVERROR_PATCHWELCOME;

Indentation off again


Except for these details, this looks good. :-)

// Martin