On Thu, May 3, 2018 at 8:58 PM, Jan Ekström <jee...@gmail.com> wrote:
> On Thu, May 3, 2018 at 7:50 PM, Alex Converse <alex.conve...@gmail.com> wrote:
>> From: Alex Converse <alexc...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
