[FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata

[Anton Khirnov]

Quoting Alexander Strasser (2021-05-10 15:35:02)
> On 2021-05-10 10:22 +0200, Anton Khirnov wrote:
> > Export them in UTC, not the local timezone. This way the output is
> > the same everywhere. The timezone information stored in the file is
> > still ignored, since there seems to be no simple way to export it
> > correctly.
> > 
> > Format them according to ISO 8601, which we generally use for exporting
> > dates.
> > ---
> > If anyone has practical suggestions for exporting the timezone, I would
> > love to hear them. Don't see anything in the standard library for
> > "express this UTC timestamp in a given timezone". Just adding the offset
> > to the timestamp would AFAIU not be correct wrt leap seconds (and maybe
> > something else? dates are hard).
> 
> Surprisingly it seems best to ignore the timezone part on purpose!
> 
> In 2.13 of Adobe's AMF 0 spec
>   https://www.adobe.com/content/dam/acom/en/devnet/pdf/amf0-file-format-specification.pdf
> 
> it is stated that the timezone part should not be filled, nor
> used.
> 
> So the commit message part about it and the comment in the
> added by this patch should probably be removed.
> 
> Though independent of this patch, one could maybe add a
> check that states sample welcome if a file with a timezone
> offset is found.

I have no idea how FLV is related to AMF0, but the sample used in the
test in question here does have a non-zero value of the tz offset. So
the comment seems appropriate to me.

> 
>  
> > Also, the conversion of double to time_t is potentially UB if the source
> > value is out of range, but there seems to be no standard way to check,
> > since the range of time_t is not standard either. Anyone got any ideas?
> 
> Not really. One could use sizeof to obtain the size, but for
> that to be useful one would need to assume signedness. The
> C standard doesn't seem to be explicit about that. And if
> it did, we still don't know if the functions handle negative
> offsets correctly.
> 
> Seems that can't really by handled well locally here only
> Maybe we do already or could add a check in configure
> to find out about the bounds of time_t . Could be useful
> elsewhere in the codebase too. Maybe other have better
> ideas...
> 
> 
> Otherwise your patch looks good to me and fine without
> additional bound checks as it's not a new problem here.
> 
> It could cause problems for people handling the timestamp
> in the currently exported format though. Maybe at least a
> micro bump should be in the final commit?

We are in an ABI instability period right now, so this should not be
necessary.

-- 
Anton Khirnov