[FFmpeg-soc] vp8 de/packetizers

[Josh Allmann]

On 11 August 2010 02:01, Martin Storsjö <martin at martin.st> wrote:
> On Wed, 11 Aug 2010, Josh Allmann wrote:
>
>> Attaching...
>
> The packetizer looks ok to me. Perhaps still adding a warning message at
> startup that the spec isn't finalized yet and may still change, as Luca A
> pointed out?
>

Done. The best place to print this was in sdp.c. Also added a similar
note when initializing the depacketizer.

I will take responsibility for maintaining this through spec updates,
even after gsoc. Hopefully it will be officially ratified as a rfc
soon enough.

> For the depacketizer:
>
>> +static int vp8_handle_packet(AVFormatContext *ctx,
>> +                             PayloadContext *vp8,
>> +                             AVStream *st,
>> +                             AVPacket *pkt,
>> +                             uint32_t *timestamp,
>> +                             const uint8_t *buf,
>> +                             int len, int flags)
>> +{
>> +    int start_packet, end_packet, has_au;
>> +    if (!buf)
>> +        return AVERROR_INVALIDDATA;
>> +
>> +    start_packet = *buf & 1;
>> +    end_packet   = flags & RTP_FLAG_MARKER;
>> +    has_au = *buf & 2;
>> +    buf++;
>> +    len--;
>> +
>> +    if (start_packet) {
>> +        int res;
>> +        if (vp8->data) { // drop previous frame if needed
>> +            uint8_t *tmp;
>> +            url_close_dyn_buf(vp8->data, &tmp);
>> +            vp8->data = NULL;
>> +            av_free(tmp);
>> +        }
>
> Instead of dropping the previous frame, perhaps we should return the data
> we have buffered so far? If this contains the whole first data partition,
> we will be able to decode the rest of the GOP even if that particular
> frame is broken.
>
> If end_packet isn't set, this is relatively simple - close the previous
> dyn buffer and set the data into pkt, then buffer the new data into a new
> dyn buf. You also need to swap vp8->timestamp and *timestamp in that case
> - the timestamp for the data you've returned is in vp8->timestamp, which
> you need to set into *timestamp so that the returned packet gets the right
> timestamp, but for the new data you buffer up, you need to store the
> current *timestamp.
>
> If end_packet is set, we'd need to return two packets - both the previous,
> incomplete one, and the new one. In that case, you'd need to do something
> like what I described above, but return 1 so that the code outside knows
> to request another packet, then make the code handle the case when called
> with buf = NULL.
>

I think I addressed this in a sensible way. I haven't actually tested
this behavior, though.

>> +        if ((res = url_open_dyn_buf(&vp8->data)) < 0)
>> +            return res;
>> +        vp8->is_keyframe = *buf & 1;
>> +        vp8->timestamp   = *timestamp;
>> +    }
>> +
>> +    if (!vp8->data || vp8->timestamp != *timestamp) {
>> +        av_log(ctx, AV_LOG_WARNING,
>> +               "Received no start marker; dropping frame\n");
>> +        return AVERROR(EAGAIN);
>> +    }
>
> If the timestamps differ and we actually have some old data buffered, do
> we want to return it here? Since the timestamps differ in that case, we
> wouldn't be interested in returning the currently processed data, though,
> since we have missed the start of that frame anyway. (On the other hand,
> in that case, we've already screwed up the GOP, so is there any point in
> returning the previous packet at all?)
>

In this case, we will have definitely lost the first data partition in
the frame, so I don't see the point in returning old data.

>> +
>> +    // cycle through VP8AU headers if needed
>> +    while (len) {
>> +        int au_len = len;
>> +        if (has_au && len > 2) {
>> +            au_len = AV_RB16(buf) & 0x7ff; // use lower 11 bits
>> +            buf += 2;
>> +            len -= 2;
>> +            if (buf + au_len > buf + len) {
>> +                av_log(ctx, AV_LOG_ERROR, "Invalid VP8AU length\n");
>> +                return AVERROR_INVALIDDATA;
>> +            }
>> +        }
>
> I'm not totally sure of the intent behind VP8 aggregate units - if each
> unit is a separate frame, we should split them here, I think. But from the
> discussion on the spec:
>

I don't either, apparently VP8AUs are for real time/low latency
communications. Error detection/correction is my best guess.

>> > If I understand correctly, it means that you can only aggregate
>> > packets from the same video frame ? If it is the case, it should be
>> > clearer. That said, can't they be just merged ?
>>
>> Yes. When people asked for having the ability to aggregate VP8 data with
>> same timestamp, I think they were looking to the future. For different
>> applications it could be advantageous.
>
> So in that case, merging the AUs like this probably is ok.
>

Also, from Section 1.1 of the proposal (timestamp part):
"There is at most one media sample or partial media sample per RTP packet."

Which implies that multiple VP8AUs in a packet must belong to the same frame.

> Perhaps a warning (either code comment or runtime warning) in this
> codepath in some way, that the VP8AU code is untested?
>

Fixed, put it in comments to avoid spamming the console in the event
we do encounter a stream with VP8AUs.

Also, I changed the VP8AU header to use the full 16 bits of the field
as length, since that seems to be the consensus on the list.

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