[FFmpeg-soc] vp8 de/packetizers

[Martin Storsjö]

On Mon, 2 Aug 2010, Josh Allmann wrote:

> On 2 August 2010 16:51, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> > Hi Josh,
> >
> > On Mon, Aug 2, 2010 at 7:46 PM, Josh Allmann <joshua.allmann at gmail.com> wrote:
> >> Also, I talked to the dev responsible for gstreamer's vp8 RTP.
> >> Apparently gstreamer's implementation is made up, so we're going to be
> >> incompatible with them until a more official spec is published.
> >
> > No surprise there. OK, feel free to apply whenever people stop giving
> > comments then.
> >
> >> +    int            is_keyframe; // this is rather unused
> >
> > That's not good, it should set PKT_FLAG_KEY.
> >
> 
> Done. This also fixed stream copying on the receiver side; I didn't
> realize that was a bug.
> 
> Also added doxy to the depacketizer, and a comment in the packetizer
> indicating where to locate the spec.
> 
> 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.


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?

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

These could be aligned

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


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

// Martin