[libav-devel] [PATCH] avformat/rtmppkt: handle extended timestamp field even for one-byte header
Martin Storsjö
martin at martin.st
Wed Mar 5 14:59:59 CET 2014
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
libav-devel at libav.org (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
More information about the libav-devel
mailing list