Hi Martin,
Thanks for your patch! It looks mostly good - I've added a few comments
below. I'll split up your patch in a few smaller patches and resend it to
[email protected] (and to you so you can verify, I can't test it
myself).
On Wed, 5 Mar 2014, Martin Panter wrote:
From 0a1e2beac5b276f74622ab33fe5245e831de516d Mon Sep 17 00:00:00 2001
From: Martin Panter <vadmium à gmail·com>
I hope you're ok with changing this to the proper address with @ and . -
we don't use obfuscated email addresses in the git history in libav, and
spam shouldn't be an issue with gmail anyway.
Date: Wed, 5 Mar 2014 04:04:39 +0000
Subject: [PATCH] avformat/rtmppkt: handle extended timestamp field even for
one-byte header
Related fix in "rtmpdump":
https://repo.or.cz/w/rtmpdump.git/commitdiff/79459a2
Adobe's RTMP specification (21 Dec 2012), section 5.3.1.3 ("Extended
Timestamp"), says "this field is present in Type 3 chunks". Type 3 chunks are
those with the one-byte header size.
This resolves intermittent hangs and segfaults caused by the read function,
and also includes an untested fix for the write function.
The read function was tested with ABC (Australia) News 24 streams, however
they are probably restricted to only Australian internet addresses. Some of
the packets at the start of these streams seem to contain junk timestamp
fields, often requiring the extended field. Test command:
ffplay rtmp://cp81899.live.edgefcs.net/live/news24-med@28772
---
Original patch: https://github.com/vadmium/FFmpeg/commit/0a1e2be.patch
I fixed the equivalent issue in “rtmpdump” and Martin Storsjö
suggested fixing the internal “libavformat” implementation as well, so
here you are :). I also tried fixing the packet write function,
because it shares the affected “ts_delta” field, but I have not tested
it. (Testing would probably involve generating packets with a
timestamp delta ≥ 0xFFFFFF, or arbitrary junk like I was seeing from
the ABC.)
Also, I would suggest renaming “ts_delta” to “ts_field” or something
in the RTMPPacket structure, because it is not always interpreted as a
delta. But I’m not sure if that is practical; maybe the structure has
a public API scope or is used elsewhere.
It's not a public API, so it should be just fine to rename it. I can
rename it in a patch on top of yours.
libavformat/rtmppkt.c | 59 ++++++++++++++++++++++++++++-----------------------
libavformat/rtmppkt.h | 2 +-
2 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index f99540c..b3a8294 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -169,6 +169,7 @@ static int rtmp_packet_read_one_chunk(URLContext
*h, RTMPPacket *p,
uint8_t buf[16];
int channel_id, timestamp, size;
+ uint32_t ts_field; // non-extended timestamp or delta field
uint32_t extra = 0;
enum RTMPPacketType type;
int written = 0;
@@ -193,14 +194,14 @@ static int rtmp_packet_read_one_chunk(URLContext
*h, RTMPPacket *p,
type = prev_pkt[channel_id].type;
extra = prev_pkt[channel_id].extra;
- hdr >>= 6;
+ hdr >>= 6; // header size indicator
Technically this change is kinda unrelated to the rest - I'll split it out
separately.
if (hdr == RTMP_PS_ONEBYTE) {
- timestamp = prev_pkt[channel_id].ts_delta;
+ ts_field = prev_pkt[channel_id].ts_delta;
} else {
if (ffurl_read_complete(h, buf, 3) != 3)
return AVERROR(EIO);
written += 3;
- timestamp = AV_RB24(buf);
+ ts_field = AV_RB24(buf);
if (hdr != RTMP_PS_FOURBYTES) {
if (ffurl_read_complete(h, buf, 3) != 3)
return AVERROR(EIO);
@@ -217,11 +218,13 @@ static int rtmp_packet_read_one_chunk(URLContext
*h, RTMPPacket *p,
extra = AV_RL32(buf);
}
}
- if (timestamp == 0xFFFFFF) {
- if (ffurl_read_complete(h, buf, 4) != 4)
- return AVERROR(EIO);
- timestamp = AV_RB32(buf);
- }
+ }
+ if (ts_field == 0xFFFFFF) {
+ if (ffurl_read_complete(h, buf, 4) != 4)
+ return AVERROR(EIO);
+ timestamp = AV_RB32(buf);
+ } else {
+ timestamp = ts_field;
}
if (hdr != RTMP_PS_TWELVEBYTES)
timestamp += prev_pkt[channel_id].timestamp;
@@ -232,8 +235,7 @@ static int rtmp_packet_read_one_chunk(URLContext
*h, RTMPPacket *p,
return ret;
p->read = written;
p->offset = 0;
- prev_pkt[channel_id].ts_delta = timestamp -
- prev_pkt[channel_id].timestamp;
+ prev_pkt[channel_id].ts_delta = ts_field;
prev_pkt[channel_id].timestamp = timestamp;
} else {
// previous packet in this channel hasn't completed reading
These parts so far look ok, I'll separate out them to a patch for adding
support for this behaviour in the receiving half of the code.
@@ -303,18 +305,30 @@ int ff_rtmp_packet_write(URLContext *h, RTMPPacket *pkt,
int written = 0;
int ret;
RTMPPacket *prev_pkt;
+ int use_delta; // flag if using timestamp delta, not RTMP_PS_TWELVEBYTES
+ uint32_t timestamp; // full 32-bit timestamp or delta value
if ((ret = ff_rtmp_check_alloc_array(prev_pkt_ptr, nb_prev_pkt,
pkt->channel_id)) < 0)
return ret;
prev_pkt = *prev_pkt_ptr;
-
- pkt->ts_delta = pkt->timestamp - prev_pkt[pkt->channel_id].timestamp;
-
+
Your patch added trailing whitespace here, which is forbidden in libav
git.
//if channel_id = 0, this is first presentation of prev_pkt, send full hdr.
- if (prev_pkt[pkt->channel_id].channel_id &&
+ use_delta = prev_pkt[pkt->channel_id].channel_id &&
pkt->extra == prev_pkt[pkt->channel_id].extra &&
- pkt->timestamp >= prev_pkt[pkt->channel_id].timestamp) {
+ pkt->timestamp >= prev_pkt[pkt->channel_id].timestamp;
+
+ timestamp = pkt->timestamp;
+ if (use_delta) {
+ timestamp -= prev_pkt[pkt->channel_id].timestamp;
+ }
+ if (timestamp >= 0xFFFFFF) {
+ pkt->ts_delta = 0xFFFFFF;
+ } else {
+ pkt->ts_delta = timestamp;
+ }
+
+ if (use_delta) {
if (pkt->type == prev_pkt[pkt->channel_id].type &&
pkt->size == prev_pkt[pkt->channel_id].size) {
mode = RTMP_PS_FOURBYTES;
@@ -334,30 +348,23 @@ int ff_rtmp_packet_write(URLContext *h, RTMPPacket *pkt,
bytestream_put_byte(&p, 1 | (mode << 6));
bytestream_put_le16(&p, pkt->channel_id - 64);
}
+ bytestream_put_be24(&p, pkt->ts_delta);
if (mode != RTMP_PS_ONEBYTE) {
- uint32_t timestamp = pkt->timestamp;
- if (mode != RTMP_PS_TWELVEBYTES)
- timestamp = pkt->ts_delta;
- bytestream_put_be24(&p, timestamp >= 0xFFFFFF ? 0xFFFFFF : timestamp);
I think this change here is wrong. If we're sending an RTMP_PS_ONEBYTE
packet, we're not transmitting the 0xffffff timestamp again.
The rest of it looks good. I'll resend the split patchset with these
details corrected.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
