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

[Jan Ekström]

WHAT!?

This LGTM was for the bit mask *only*

ALL OTHER POINTS STAND

Jan

On Fri, May 4, 2018, 06:04 Steven Liu <lq at chinaffmpeg.org> wrote:

>
>
> > On 4 May 2018, at 02:00, Jan Ekström <jeebjp at gmail.com> wrote:
> >
> > 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.
>
> Will apply.
>
> Thanks
> Steven
>
>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>