[FFmpeg-soc] vp8 de/packetizers

[Josh Allmann]

On 3 August 2010 10:04, Martin Storsjö <martin at martin.st> wrote:
> On Mon, 2 Aug 2010, Josh Allmann wrote:
>>
>> Unless I've mis-counted my marker bits yet again, we are in full
>> compliance with the spec, except for the parts that are undefined --
>> the "VP8" encoding name in particular.
>
> One issue in the proposal that I don't think we comply to (and that I
> think should be changed before it is finalized, is this:
>
>> 3.3 Lost VP8 RTP packets
>> If a lost packet is detected in the RTP stream, the transport layer
>> MUST throw out the current partial VP8 frame (if there is one) and
>> all subsequent RTP packets until a VP8 payload descriptor with the
>> key-frame bit is set.
>
> Not returning a single packet up until the next keyframe, if a single
> packet is lost, feels quite horrible to me. Even though it may not look
> good if some data is lost, it's still better than not getting any data at
> all up until the next keyframe.
>

Indeed. Currently, we only drop if the start or end markers are
missing, until the next valid start marker. Even then, we *could*
still pass those into the decoder. I made a comment about this on the
webm ML, feel free to chime in there.

>
> Other than that, it looks quite good to me. A few style nitpicks yet:
>
>> +    start_packet    = *buf & 1;
>> +    end_packet      = flags & RTP_FLAG_MARKER;
>> +    is_keyframe     = *buf & 2;
>
> Why so much extra space before the equals sign?
>

Fixed.

>> +        if ((res = url_open_dyn_buf(&vp8->data)) < 0)
>> +            return res;
>> +        vp8->is_keyframe = is_keyframe;
>> +        vp8->timestamp = *timestamp;
>
> These could be aligned
>

Fixed.

>> +    if (end_packet) {
>> +        av_init_packet(pkt);
>> +        pkt->stream_index = st->index;
>> +        pkt->flags = vp8->is_keyframe ? AV_PKT_FLAG_KEY : 0;
>> +        pkt->size = url_close_dyn_buf(vp8->data, &pkt->data);
>> +        pkt->destruct = av_destruct_packet;
>> +        vp8->data = NULL;
>> +        return 0;
>> +    }
>
> Some (all?) of these could be aligned, too.
>

Fixed.

>
> I'll see if Luca A wants to say something on the packetizer part, other
> than that, I think this one is done.
>

Sweet.

Also made another minor alignment change in rtpenc.

Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-RTP-packetization-of-VP8.patch
Type: text/x-patch
Size: 2884 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100803/600853db/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-RTP-depacketization-of-VP8.patch
Type: text/x-patch
Size: 5915 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20100803/600853db/attachment-0001.bin>