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

[Alex Converse]

On Thu, May 3, 2018 at 10:58 AM, Jan Ekström <jeebjp at gmail.com> wrote:
>
> 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)`?
>

See the note about timestamps below.

> 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.
>

FLV AVC packets have a field called CompositionTime described to match
14496-12. FLV requires this composition offset to be zero for sequence header
type packets. This strongly indicates to me that we want DTS here. In addition
the current muxer and demuxer already use dts pretty consistently for this
value (and use cts = pts - dts).


> >      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 ?
>

The mapping from dts to the flv timestamp we write to the stream is identical
for to what we do for non header packets. [1] I can pull this into a separate
function but it's only two lines so it didn't seem worth while.

> >          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?

The FLV file (on disk) format requires the initial DTS to be zero. For FLV
over RTMP this is not the case.

At one point the flv muxer handled that itself, but that changed to handle
copyts better.[2]

For non-zero starting DTS should header packets have non-zero
timestamp? Maybe but that's the case irrespective of weather or
not we offset late extradata. Meanwhile many popular demuxers
(avformat included) special case initial extradata.

[1] https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/flvenc.c;h=e8af48cb6415a0e5795d7e36d82789ae1699f89f#l981
[2] https://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a0174f67298ba9494c146183dd360e637b03db64

>
> Otherwise, looks like a nice improvement in the implementation.
>