[FFmpeg-soc] SVQ3 depacketizer

[Ronald S. Bultje]

Hi,

On Wed, Jun 30, 2010 at 4:19 AM, Martin Storsjö <martin at martin.st> wrote:
> On Wed, 30 Jun 2010, Josh Allmann wrote:
>> +/**
>> + * @file libavformat/rtpdec_svq3.c
>> + * @brief RTP support for the SV3V (SVQ3) payload
>> + * @author Ronald S. Bultje <rbultje at ronald.bitfreak.net>
>> + */
>
> Lately, we've been instructed to remove the file names from these @file
> directives, and just keep "@file". Also, it would be good to mention which
> RFC this implements, to make it easier for the reader to look up the
> details.

No RFC, this is RE'ed stuff. :-(.

>> +/** return 0 on packet, <1 on partial packet or error... */
>
> Isn't this comment a bit wrong? Iirc, return 0 on packet, 1 on partial
> packets (that is, more packets can be returned), <0 on error. When
> checking other depacketizers, others seem to have some kind of confusion,
> too.

Depends on spreading. We return 1 if the RTP packet contained multiple
demuxer packets, e.g. two video frames. What happens here is that >=2
RTP packets contain 1 video frame, so we don't have any data after 1
RTP packet (partial packet) and thus return -1.

>> +    config_packet = buf[0] & 0x40;
>> +    start_packet  = buf[0] & 0x20;
>> +    end_packet    = buf[0] & 0x10;
>> +    buf += 2;
>> +    len -= 2;
>
> What's in buf[1], that we apparently ignore? :-) I'm not sure if a comment
> describing it here is necessary, a RFC reference somewhere would be enough
> so that I'd find it easily.

buf[1] is probably padding for future flags extension, which of course
never happened. The binary didn't use buf[1].

>> +        /* frame size code */
>> +        init_get_bits(&gb, buf, len << 3);
>> +        config = get_bits(&gb, 3);
>> +        switch (config) {
>> +            case 0: st->codec->width = 160; st->codec->height = 128 /* FIXME: Wiki says 120? */; break;
>
> Hmm, I guess I should try to generate files with any of these dimensions
> and see if it uses them then, to get clarity in 128 vs 120. (120 sounds
> much more natural when combined with 160, though.)
>
>> +            case 1: st->codec->width = 128; st->codec->height =  96; break;
>> +            case 2: st->codec->width = 176; st->codec->height = 144; break;
>> +            case 3: st->codec->width = 352; st->codec->height = 288; break;
>> +            case 4: st->codec->width = 704; st->codec->height = 576; break;
>> +            case 5: st->codec->width = 240; st->codec->height = 180; break;
>> +            case 6: st->codec->width = 320; st->codec->height = 240; break;
>> +            case 7: /**< in case of '111', the next 2x12 bits carry optional width/height */
>> +                st->codec->width  = get_bits(&gb, 12);
>> +                st->codec->height = get_bits(&gb, 12);
>> +                break;

This whole block can be removed (I removed it locally later, but never
resubmitted), because the decoder does this for us.

Ronald