[libav-devel] [PATCH] avformat/rtmppkt: handle extended timestamp field even for one-byte header

[Martin Storsjö]

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