[FFmpeg-soc] SVQ3 depacketizer

[Josh Allmann]

Hi,

On 30 June 2010 09:53, Martin Storsjö <martin at martin.st> wrote:
> On Wed, 30 Jun 2010, Ronald S. Bultje wrote:
>
>> 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. :-(.
>
> I figured that out after googling a bit, but to save me from googling next
> time, it may be good to add a notice that there's no RFC.
>

Removed the filename from the @file directive, and added a note
indicating this depacketizer is RE'ed.

>> >> +/** 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.
>
> Ah, yeah, well even when referring to that, the comment which says "<1 on
> partial packet or error" should be "<0" of course.
>

Yeah, my bad; the original comment Ronald wrote was correct and I
screwed it up while editing.

>> >> +    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].
>
> Ok, if there's no RFC, this is a very good explanation indeed. :-)

Comment added indicating this.

>
>> >> +            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.
>
> Yeah, I was about to suggest that, too.
>

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.

QDM2 patch forthcoming; I'll make a new thread for that.

Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-depacketization-of-SVQ3.patch
Type: text/x-patch
Size: 7596 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100630/e124dff7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-padding-to-every-url_close_dyn_buf-call.patch
Type: text/x-patch
Size: 799 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100630/e124dff7/attachment-0001.bin>