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

[Jan Ekström]

On Thu, May 3, 2018 at 8:58 PM, 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)`?
>
> 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 ?
>

D'oh, I should read better. "...to form a SI32 value". Thus, this is LGTM.

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

Jan