Hi, Here is a packetizer and a depacketizer for VP8 RTP. Playback is only semi-smooth; it drops way too many frames. The depacketizer likes to claim it gets an extra packet after the frame-end (which is typically one byte smaller than the actual frame-end packet). Still a work in progress. On a side note, I think it is slightly excessive that we have so many rtpdec_* header files that only declare one or two structs. Would it make sense to combine all those declarations into a single depacketizers.h file or something? Josh
On Thu, 29 Jul 2010, Josh Allmann wrote:
On a side note, I think it is slightly excessive that we have so many rtpdec_* header files that only declare one or two structs. Would it make sense to combine all those declarations into a single depacketizers.h file or something?
Yes, that would make sense - rtpenc does it like this, too. I've actually got a patch for doing about this hanging around - the diffstat for it says this: 15 files changed, 16 insertions(+), 247 deletions(-) So I think it would be worthwhile. What do you think, Luca B and Ronald? Where should we put them - rtpdec.h? // Martin
Hi, On Jul 29, 2010, at 7:49 AM, Martin Storsjö <martin@martin.st> wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
On a side note, I think it is slightly excessive that we have so many rtpdec_* header files that only declare one or two structs. Would it make sense to combine all those declarations into a single depacketizers.h file or something?
Yes, that would make sense - rtpenc does it like this, too. I've actually got a patch for doing about this hanging around - the diffstat for it says this:
15 files changed, 16 insertions(+), 247 deletions(-)
So I think it would be worthwhile. What do you think, Luca B and Ronald? Where should we put them - rtpdec.h?
Sounds good to me, although I'd create a fresh .h-file for all decls. Maybe rtpdec_depacketizers.h? Ronald
On 07/29/2010 02:04 PM, Ronald S. Bultje wrote:
Sounds good to me, although I'd create a fresh .h-file for all decls. Maybe rtpdec_depacketizers.h?
A bit too long IMHO. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Thu, 29 Jul 2010, Ronald S. Bultje wrote:
On Jul 29, 2010, at 7:49 AM, Martin Storsjö <martin@martin.st> wrote:
So I think it would be worthwhile. What do you think, Luca B and Ronald? Where should we put them - rtpdec.h?
Sounds good to me, although I'd create a fresh .h-file for all decls. Maybe rtpdec_depacketizers.h?
Initial patch doing this attached. The key point of the patch: 16 files changed, 47 insertions(+), 247 deletions(-) // Martin
On Thu, 29 Jul 2010, Martin Storsjö wrote:
On Thu, 29 Jul 2010, Ronald S. Bultje wrote:
On Jul 29, 2010, at 7:49 AM, Martin Storsjö <martin@martin.st> wrote:
So I think it would be worthwhile. What do you think, Luca B and Ronald? Where should we put them - rtpdec.h?
Sounds good to me, although I'd create a fresh .h-file for all decls. Maybe rtpdec_depacketizers.h?
Initial patch doing this attached. The key point of the patch:
16 files changed, 47 insertions(+), 247 deletions(-)
Updated patch attached, that also removes rtpdec_asf.h and moves the function declaration into the common header. The diffstat is now: 19 files changed, 58 insertions(+), 292 deletions(-) Ronald and Luca, please finish the discussion on the naming of the file, so that we can get this applied. :-) The arguments currenly stand: - Not in rtpdec.h, to keep it separate and not clutter the generic file - rtpdec_depacketizers.h is quite a long name... // Martin
On 07/30/2010 09:08 AM, Martin Storsjö wrote:
Ronald and Luca, please finish the discussion on the naming of the file, so that we can get this applied. :-)
The arguments currenly stand: - Not in rtpdec.h, to keep it separate and not clutter the generic file - rtpdec_depacketizers.h is quite a long name...
rtpdec_common.h? if you like rtpdec_depacketizers.h better I'm fine with it as well. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Fri, 30 Jul 2010, Luca Barbato wrote:
On 07/30/2010 09:08 AM, Martin Storsjö wrote:
Ronald and Luca, please finish the discussion on the naming of the file, so that we can get this applied. :-)
The arguments currenly stand: - Not in rtpdec.h, to keep it separate and not clutter the generic file - rtpdec_depacketizers.h is quite a long name...
rtpdec_common.h?
Hmm, doesn't really fit the use I think.
if you like rtpdec_depacketizers.h better I'm fine with it as well.
I don't really like it either, it's quite long and ugly. What about rtpdec_formats.h? // Martin
On 07/30/2010 09:32 AM, Martin Storsjö wrote:
if you like rtpdec_depacketizers.h better I'm fine with it as well.
I don't really like it either, it's quite long and ugly. What about rtpdec_formats.h?
Ok lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Fri, 30 Jul 2010, Luca Barbato wrote:
On 07/30/2010 09:32 AM, Martin Storsjö wrote:
if you like rtpdec_depacketizers.h better I'm fine with it as well.
I don't really like it either, it's quite long and ugly. What about rtpdec_formats.h?
Ok
Good, I'll apply it in that form if Ronald is ok with that name, too. // Martin
Hi, On Jul 30, 2010, at 3:49 AM, Martin Storsjö <martin@martin.st> wrote:
On Fri, 30 Jul 2010, Luca Barbato wrote:
On 07/30/2010 09:32 AM, Martin Storsjö wrote:
if you like rtpdec_depacketizers.h better I'm fine with it as well.
I don't really like it either, it's quite long and ugly. What about rtpdec_formats.h?
Ok
Good, I'll apply it in that form if Ronald is ok with that name, too.
Sounds good to me too. Ronald
On Fri, 30 Jul 2010, Ronald S. Bultje wrote:
On Jul 30, 2010, at 3:49 AM, Martin Storsjö <martin@martin.st> wrote:
On Fri, 30 Jul 2010, Luca Barbato wrote:
On 07/30/2010 09:32 AM, Martin Storsjö wrote:
if you like rtpdec_depacketizers.h better I'm fine with it as well.
I don't really like it either, it's quite long and ugly. What about rtpdec_formats.h?
Ok
Good, I'll apply it in that form if Ronald is ok with that name, too.
Sounds good to me too.
Great, applied. // Martin
On 07/29/2010 01:49 PM, Martin Storsjö wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
On a side note, I think it is slightly excessive that we have so many rtpdec_* header files that only declare one or two structs. Would it make sense to combine all those declarations into a single depacketizers.h file or something?
Yes, that would make sense - rtpenc does it like this, too. I've actually got a patch for doing about this hanging around - the diffstat for it says this:
15 files changed, 16 insertions(+), 247 deletions(-)
So I think it would be worthwhile. What do you think, Luca B and Ronald? Where should we put them - rtpdec.h?
Sounds a good place. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On Thu, 29 Jul 2010, Josh Allmann wrote:
Here is a packetizer and a depacketizer for VP8 RTP.
Playback is only semi-smooth; it drops way too many frames. The depacketizer likes to claim it gets an extra packet after the frame-end (which is typically one byte smaller than the actual frame-end packet). Still a work in progress.
Didn't feel any significant frame dropping here (btw, if playing with ffplay, make sure you use -noframedrop, sometimes ffplay constantly feels it's behind and needs to drop some frames to catch up, but since the source is realtime, it still has to block for more frames, so it constantly drops frames), but the playback console is spammed with messages like this: [vp8 @ 0x10188fe00] Unknown profile 6 [vp8 @ 0x10188fe00] Header size larger than data provided [rtsp @ 0x10202dc00] Received no start marker; dropping frame In general, this looks quite ok (I haven't read the spec, but from the code it looks quite simple), here's a quick initial review:
+static void rtp_send_vp8(AVFormatContext *s1, const uint8_t *buf, int size) +{ + RTPMuxContext *s = s1->priv_data; + int len, max_packet_size, keyframe; + + s->buf_ptr = s->buf; + s->timestamp = s->cur_timestamp; + max_packet_size = s->max_payload_size - 1; // minus one for header byte + keyframe = ( !(*buf & 1) ) << 3; +
I guess this could be written a little less convoluted as keyframe = *buf & 1 ? 0 : 8; but that's mostly bikeshedding. :-)
+ *s->buf_ptr++ = keyframe | 4; // 0b100 indicates start of frame + while (size > 0) { + len = size > max_packet_size ? max_packet_size : size;
len = FFMIN(size, max_packet_size);
+ + memcpy(s->buf_ptr, buf, len); + ff_rtp_send_data(s1, s->buf, len+1, size == len); // marker bit is last packet in frame + + size -= len; + buf += len; + s->buf_ptr = s->buf; + *s->buf_ptr++ = keyframe; + } +} +
+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 = *buf & 4; + int end_packet = flags & RTP_FLAG_MARKER; + int is_keyframe = *buf & 8;
Keep in mind that buf can be NULL Except for that, it mostly looked quite good. // Martin
On 30 July 2010 00:22, Martin Storsjö <martin@martin.st> wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
Here is a packetizer and a depacketizer for VP8 RTP.
Playback is only semi-smooth; it drops way too many frames. The depacketizer likes to claim it gets an extra packet after the frame-end (which is typically one byte smaller than the actual frame-end packet). Still a work in progress.
Didn't feel any significant frame dropping here (btw, if playing with ffplay, make sure you use -noframedrop, sometimes ffplay constantly feels it's behind and needs to drop some frames to catch up, but since the source is realtime, it still has to block for more frames, so it constantly drops frames), but the playback console is spammed with messages like this:
[vp8 @ 0x10188fe00] Unknown profile 6 [vp8 @ 0x10188fe00] Header size larger than data provided [rtsp @ 0x10202dc00] Received no start marker; dropping frame
-noframedrop helped, but the last message is what I am concerned with; it really should not happen. Those seem like phantom packets.
In general, this looks quite ok (I haven't read the spec, but from the code it looks quite simple), here's a quick initial review:
It is basically raw RTP with the addition of two bits, one to mark keyframes, and another to mark the start of a frame. I made a few assumptions here that were not covered by the spec:* -Those bits are located at the first byte prior to the data payload. -The sdp rtpmap codec ID is "VP8." *https://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread...
+ keyframe = ( !(*buf & 1) ) << 3; +
I guess this could be written a little less convoluted as
keyframe = *buf & 1 ? 0 : 8;
but that's mostly bikeshedding. :-)
Fixed, since it is clearer that way.
+ *s->buf_ptr++ = keyframe | 4; // 0b100 indicates start of frame + while (size > 0) { + len = size > max_packet_size ? max_packet_size : size;
len = FFMIN(size, max_packet_size);
Fixed
+ int start_packet = *buf & 4; + int end_packet = flags & RTP_FLAG_MARKER; + int is_keyframe = *buf & 8;
Keep in mind that buf can be NULL
Fixed. Josh
On 07/30/2010 10:36 AM, Josh Allmann wrote:
On 30 July 2010 00:22, Martin Storsjö <martin@martin.st> wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
Here is a packetizer and a depacketizer for VP8 RTP.
Playback is only semi-smooth; it drops way too many frames. The depacketizer likes to claim it gets an extra packet after the frame-end (which is typically one byte smaller than the actual frame-end packet). Still a work in progress.
Didn't feel any significant frame dropping here (btw, if playing with ffplay, make sure you use -noframedrop, sometimes ffplay constantly feels it's behind and needs to drop some frames to catch up, but since the source is realtime, it still has to block for more frames, so it constantly drops frames), but the playback console is spammed with messages like this:
[vp8 @ 0x10188fe00] Unknown profile 6 [vp8 @ 0x10188fe00] Header size larger than data provided [rtsp @ 0x10202dc00] Received no start marker; dropping frame
-noframedrop helped, but the last message is what I am concerned with; it really should not happen. Those seem like phantom packets.
Assert on it and see what's the content of buf. You end up there if you miss the start packet. btw do we cleanup when the seqno jumps? 1 2 3 4 5 6 7 8 9 S _ _ _ E S _ _ E What happens when we miss both 5 and 6 ? lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 30 July 2010 02:12, Luca Barbato <lu_zero@gentoo.org> wrote:
btw do we cleanup when the seqno jumps?
1 2 3 4 5 6 7 8 9 S _ _ _ E S _ _ E
What happens when we miss both 5 and 6 ?
Fixed this by checking the timestamp. In this case both frames will be dropped. If the timestamps between two consecutive packets differ without a start/end marker, then all packets are ignored until the next start marker is encountered. Josh
On Fri, 30 Jul 2010, Josh Allmann wrote:
On 30 July 2010 00:22, Martin Storsjö <martin@martin.st> wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
[vp8 @ 0x10188fe00] Unknown profile 6 [vp8 @ 0x10188fe00] Header size larger than data provided [rtsp @ 0x10202dc00] Received no start marker; dropping frame
-noframedrop helped, but the last message is what I am concerned with; it really should not happen. Those seem like phantom packets.
@@ -393,6 +418,8 @@ static int rtp_write_packet(AVFormatContext *s1, AVPacket *pkt) case CODEC_ID_H263P: ff_rtp_send_h263(s1, pkt->data, size); break; + case CODEC_ID_VP8: + rtp_send_vp8(s1, pkt->data, size); default: /* better than nothing : send the codec raw data */ rtp_send_raw(s1, pkt->data, size);
Take a look at this chunk and guess why you're getting phanton packets. :-) I'll take another look at the new version of the patches sometimes soon, but adding a break there seems to fix most of the issues :-) // Martin
On 2 August 2010 06:20, Martin Storsjö <martin@martin.st> wrote:
On Fri, 30 Jul 2010, Josh Allmann wrote:
On 30 July 2010 00:22, Martin Storsjö <martin@martin.st> wrote:
On Thu, 29 Jul 2010, Josh Allmann wrote:
[vp8 @ 0x10188fe00] Unknown profile 6 [vp8 @ 0x10188fe00] Header size larger than data provided [rtsp @ 0x10202dc00] Received no start marker; dropping frame
-noframedrop helped, but the last message is what I am concerned with; it really should not happen. Those seem like phantom packets.
@@ -393,6 +418,8 @@ static int rtp_write_packet(AVFormatContext *s1, AVPacket *pkt) case CODEC_ID_H263P: ff_rtp_send_h263(s1, pkt->data, size); break; + case CODEC_ID_VP8: + rtp_send_vp8(s1, pkt->data, size); default: /* better than nothing : send the codec raw data */ rtp_send_raw(s1, pkt->data, size);
Take a look at this chunk and guess why you're getting phanton packets. :-)
1000l for not realizing this sooner.
I'll take another look at the new version of the patches sometimes soon, but adding a break there seems to fix most of the issues :-)
Indeed, updated patches attached, along with the fixes requested by Luca. Valgrind checks out cleanly. Josh
On 2 August 2010 14:11, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 2 August 2010 13:37, Josh Allmann <joshua.allmann@gmail.com> wrote:
Indeed, updated patches attached, along with the fixes requested by Luca. Valgrind checks out cleanly.
Updated the offsets for start/keyframe markers; they were incorrect in the last round.
Fixed the offsets for good this time. Note to self: always read specs in a monospaced font. 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.
Josh
Hi Josh, On Mon, Aug 2, 2010 at 7:46 PM, Josh Allmann <joshua.allmann@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. Ronald
On 2 August 2010 16:51, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi Josh,
On Mon, Aug 2, 2010 at 7:46 PM, Josh Allmann <joshua.allmann@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. Josh
On 08/03/2010 02:25 AM, 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.
Then you should drop a line on the ml following up the email with the proposed spec, (iana sections are always boring) lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 3 August 2010 01:37, Luca Barbato <lu_zero@gentoo.org> wrote:
On 08/03/2010 02:25 AM, 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.
Then you should drop a line on the ml following up the email with the proposed spec, (iana sections are always boring)
Done. https://groups.google.com/a/webmproject.org/group/webm-discuss/browse_thread... Josh
On Mon, 2 Aug 2010, Josh Allmann wrote:
On 2 August 2010 16:51, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi Josh,
On Mon, Aug 2, 2010 at 7:46 PM, Josh Allmann <joshua.allmann@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
On Aug 3, 2010, at 10:04 AM, Martin Storsjö <martin@martin.st> wrote:
On Mon, 2 Aug 2010, Josh Allmann wrote:
On 2 August 2010 16:51, Ronald S. Bultje <rsbultje@gmail.com> wrote:
Hi Josh,
On Mon, Aug 2, 2010 at 7:46 PM, Josh Allmann <joshua.allmann@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.
VP8's probability contexts and segment map aren't reset between interframes, so if an entire frame is lost the rest of the GOP will likely decode to total junk. If you want to do better than the proposal, you can read the vp8 frame header and ignore the rfc if the first data partition is intact; that's the only guarantee that arithmetic probabilities and the segment map remain correct.
On Tue, 3 Aug 2010, David Conrad wrote:
On Aug 3, 2010, at 10:04 AM, Martin Storsjö <martin@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.
VP8's probability contexts and segment map aren't reset between interframes, so if an entire frame is lost the rest of the GOP will likely decode to total junk.
If you want to do better than the proposal, you can read the vp8 frame header and ignore the rfc if the first data partition is intact; that's the only guarantee that arithmetic probabilities and the segment map remain correct.
Oh, that's good to know, then I understand the points made in the RTP format proposal. Some parts of that is quite hard to implement - it's hard to detect that a whole frame never was delivered to us. For that, we'd probably need to pass the RTP sequence numbers to the depacketizer, so that it can notice that something was missing, even if the start/end markers look ok. David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it? // Martin
On 08/04/2010 09:59 AM, Martin Storsjö wrote:
David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it?
Even better: is there a way to get this information? If is common for every packet/there is a known upper bound, we can pass it through the sdp, or if not we could think about storing the value in an extension header. Still I guess we could extend the depacketizer interface to get all the standard rtp information as well. lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
On 4 August 2010 03:32, Luca Barbato <lu_zero@gentoo.org> wrote:
On 08/04/2010 09:59 AM, Martin Storsjö wrote:
David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it?
Even better: is there a way to get this information?
If is common for every packet/there is a known upper bound, we can pass it through the sdp, or if not we could think about storing the value in an extension header.
Not sure; I'd have to dig through the bitstream spec for this. Ronald, do you happen to know off the top of your head?
Still I guess we could extend the depacketizer interface to get all the standard rtp information as well.
Attached patches should work against the updated VP8 rtp proposal*. Also pulled from latest trunk, so they also work with the Xiph stuff. The depacketizer now supports multiple VP8AUs in one packet but I haven't tested that codepath, because VP8AUs are not used by the packetizer. * https://groups.google.com/a/webmproject.org/group/webm-discuss/msg/eef6d312ac5c09c3?dmode=source&output=gplain Josh
On 11 August 2010 00:46, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 4 August 2010 03:32, Luca Barbato <lu_zero@gentoo.org> wrote:
On 08/04/2010 09:59 AM, Martin Storsjö wrote:
David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it?
Even better: is there a way to get this information?
If is common for every packet/there is a known upper bound, we can pass it through the sdp, or if not we could think about storing the value in an extension header.
Not sure; I'd have to dig through the bitstream spec for this. Ronald, do you happen to know off the top of your head?
Still I guess we could extend the depacketizer interface to get all the standard rtp information as well.
Attached patches should work against the updated VP8 rtp proposal*. Also pulled from latest trunk, so they also work with the Xiph stuff.
The depacketizer now supports multiple VP8AUs in one packet but I haven't tested that codepath, because VP8AUs are not used by the packetizer.
Attaching...
Josh
On Wed, 11 Aug 2010, Josh Allmann wrote:
On 11 August 2010 00:46, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 4 August 2010 03:32, Luca Barbato <lu_zero@gentoo.org> wrote:
On 08/04/2010 09:59 AM, Martin Storsjö wrote:
David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it?
Even better: is there a way to get this information?
If is common for every packet/there is a known upper bound, we can pass it through the sdp, or if not we could think about storing the value in an extension header.
Not sure; I'd have to dig through the bitstream spec for this. Ronald, do you happen to know off the top of your head?
Still I guess we could extend the depacketizer interface to get all the standard rtp information as well.
Attached patches should work against the updated VP8 rtp proposal*. Also pulled from latest trunk, so they also work with the Xiph stuff.
The depacketizer now supports multiple VP8AUs in one packet but I haven't tested that codepath, because VP8AUs are not used by the packetizer.
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? 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.
+ 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?)
+ + // 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:
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. Perhaps a warning (either code comment or runtime warning) in this codepath in some way, that the VP8AU code is untested? // Martin
On 11 August 2010 02:01, Martin Storsjö <martin@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
On Wed, 11 Aug 2010, Josh Allmann wrote:
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.
The packetizer looks good to me now, do you agree, Luca A?
I think I addressed this in a sensible way. I haven't actually tested this behavior, though.
It looks quite good to me. One minor nit:
+static void prepare_packet(AVPacket *pkt, PayloadContext *vp8, int stream) +{ + av_init_packet(pkt); + pkt->stream_index = stream; + 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; +} + +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, ret = AVERROR(EAGAIN); + + if (!buf) { + // only called when vp8_handle_packet returns 1 + prepare_packet(pkt, vp8, st->index); + *timestamp = vp8->timestamp; + return 0; + }
You need to check vp8->data here, too, otherwise you'd segfault if the caller called us erroneously. Except for that, it looks quite ok. I haven't tested the new parts yet, I'll see if I can get it tested tomorrow. // Martin
On Thu, 12 Aug 2010, Martin Storsjö wrote:
Except for that, it looks quite ok. I haven't tested the new parts yet, I'll see if I can get it tested tomorrow.
Tested this now, and it seems to work just as intended. Tested it by modding the packetizer to send only the first packet of a frame for every n:th frame, and when decoding it, every nth frame had an error, but the rest of the GOP decoded just fine. // Martin
Hi, On Wed, Aug 11, 2010 at 3:46 AM, Josh Allmann <joshua.allmann@gmail.com> wrote:
On 4 August 2010 03:32, Luca Barbato <lu_zero@gentoo.org> wrote:
On 08/04/2010 09:59 AM, Martin Storsjö wrote:
David, how large is this first data partition approximately? The frames are fragmented into RTP packets of around ~1450 bytes (usually) - so if we've got the first packet of a frame but not the rest, would we be better off returning this than just skipping it?
Even better: is there a way to get this information?
If is common for every packet/there is a known upper bound, we can pass it through the sdp, or if not we could think about storing the value in an extension header.
Not sure; I'd have to dig through the bitstream spec for this. Ronald, do you happen to know off the top of your head?
1450 bytes should surely be enough for frame header, but the problem is that this would depend on frame size (since things like mbmode are per-MB), so for an infinitely-sized frame, this'd be larger. I'd recommend to just not bother and discard until the next keyframe. Ronald Ronald
On 3 August 2010 10:04, Martin Storsjö <martin@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
participants (5)
-
David Conrad -
Josh Allmann -
Luca Barbato -
Martin Storsjö -
Ronald S. Bultje