[FFmpeg-devel] [PATCH] flvenc: Fix sequence header update timestamps

[Jan Ekström]

On Thu, May 3, 2018 at 7:50 PM, Alex Converse <alex.converse at gmail.com> wrote:
> From: Alex Converse <alexconv at twitch.tv>
>
> ---
>  libavformat/flvenc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> index e8af48cb64..827d798a61 100644
> --- a/libavformat/flvenc.c
> +++ b/libavformat/flvenc.c
> @@ -480,7 +480,7 @@ static int unsupported_codec(AVFormatContext *s,
>      return AVERROR(ENOSYS);
>  }
>
> -static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par) {
> +static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par, unsigned int ts) {

Hi,

While the timestamp field is 8+24=32 bits, the DTS value is int64_ts.
Thus, while I'm really bad at doing wrap-around logic, wouldn't it be
something a la ´uint32_t wrapped_timestamp = (uint32_t)(ts %
UINT32_MAX)`?

Additionally, I did briefly go through E.4.1 (FLV Tag) in the spec and
it doesn't really seem to define whether this value is PTS or DTS...
Is this defined anywhere proper? Given FLV's age I wouldn't be
surprised that the answer would be "effectively DTS", of course. But
if you've seen what  Adobe's implementation writes with B-frames it'd
be interesting to see.

>      int64_t data_size;
>      AVIOContext *pb = s->pb;
>      FLVContext *flv = s->priv_data;
> @@ -492,8 +492,8 @@ static void flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par) {
>                  par->codec_type == AVMEDIA_TYPE_VIDEO ?
>                          FLV_TAG_TYPE_VIDEO : FLV_TAG_TYPE_AUDIO);
>          avio_wb24(pb, 0); // size patched later
> -        avio_wb24(pb, 0); // ts
> -        avio_w8(pb, 0);   // ts ext
> +        avio_wb24(pb, ts & 0xFFFFFF);
> +        avio_w8(pb, (ts >> 24) & 0x7F);

Looking at the spec this is bottom 24 bits of the timestamp and top 8,
thus why is the second one not & 0xFF ?

>          avio_wb24(pb, 0); // streamid
>          pos = avio_tell(pb);
>          if (par->codec_id == AV_CODEC_ID_AAC) {
> @@ -756,7 +756,7 @@ static int flv_write_header(AVFormatContext *s)
>      }
>
>      for (i = 0; i < s->nb_streams; i++) {
> -        flv_write_codec_header(s, s->streams[i]->codecpar);
> +        flv_write_codec_header(s, s->streams[i]->codecpar, 0);
>      }
>

Is it customary that even if you've got a live stream going, that the
start point of a given ingest is always zero? In that case you might
want to take mention of the initial dts, and then write offsets
compared to that? Otherwise if you have an initial offset of, say, 5s,
and then you a 5s GOP with an update, then while the difference to the
initial timestamp would be 5s, using the original dts field you would
get the second header having a value of 10s there. Or am I
misunderstanding something here?

Otherwise, looks like a nice improvement in the implementation.


Best regards,
Jan