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

Mon May 10 16:35:02 EEST 2021

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.

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

Greetings,
  Alexander

> ---
>  libavformat/flvdec.c     | 7 +++++--
>  tests/ref/fate/flv-demux | 2 +-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index e6c2877a74..ddaceaafe4 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -686,8 +686,11 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream,
>              struct tm t;
>              char datestr[128];
>              time =  date.milliseconds / 1000; // to seconds
> -            localtime_r(&time, &t);
> -            strftime(datestr, sizeof(datestr), "%a, %d %b %Y %H:%M:%S %z", &t);
> +            gmtime_r(&time, &t);
> +
> +            // timezone is ignored, since there is no easy way to offset the UTC
> +            // timestamp into the specified timezone
> +            strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%SZ", &t);
>  
>              av_dict_set(&s->metadata, key, datestr, 0);
>          }
> diff --git a/tests/ref/fate/flv-demux b/tests/ref/fate/flv-demux
> index 30435adeb9..827b56ea09 100644
> --- a/tests/ref/fate/flv-demux
> +++ b/tests/ref/fate/flv-demux
> @@ -605,4 +605,4 @@ packet|codec_type=audio|stream_index=1|pts=11656|pts_time=11.656000|dts=11656|dt
>  packet|codec_type=video|stream_index=0|pts=11678|pts_time=11.678000|dts=11678|dts_time=11.678000|duration=33|duration_time=0.033000|size=1190|pos=510794|flags=__|data_hash=CRC32:a0206c90
>  stream|index=0|codec_name=h264|profile=77|codec_type=video|codec_tag_string=[0][0][0][0]|codec_tag=0x0000|width=426|height=240|coded_width=426|coded_height=240|closed_captions=0|has_b_frames=1|sample_aspect_ratio=1:1|display_aspect_ratio=71:40|pix_fmt=yuv420p|level=21|color_range=unknown|color_space=unknown|color_transfer=unknown|color_primaries=unknown|chroma_location=left|field_order=progressive|refs=1|is_avc=true|nal_length_size=4|id=N/A|r_frame_rate=30000/1001|avg_frame_rate=30/1|time_base=1/1000|start_pts=0|start_time=0.000000|duration_ts=N/A|duration=N/A|bit_rate=393929|max_bit_rate=N/A|bits_per_raw_sample=8|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=351|extradata_hash=CRC32:07b85ca9|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnail
>  s=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0
>  stream|index=1|codec_name=aac|profile=1|codec_type=audio|codec_tag_string=[0][0][0][0]|codec_tag=0x0000|sample_fmt=fltp|sample_rate=22050|channels=2|channel_layout=stereo|bits_per_sample=0|id=N/A|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/1000|start_pts=0|start_time=0.000000|duration_ts=N/A|duration=N/A|bit_rate=67874|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=252|extradata_hash=CRC32:d039c029|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0
> -format|filename=Enigma_Principles_of_Lust-part.flv|nb_streams=2|nb_programs=0|format_name=flv|start_time=0.000000|duration=210.209999|size=512000|bit_rate=19485|probe_score=100|tag:hasKeyframes=true|tag:hasMetadata=true|tag:datasize=11970544|tag:hasVideo=true|tag:canSeekToEnd=false|tag:lasttimestamp=210|tag:lastkeyframetimestamp=210|tag:audiosize=1791332|tag:hasAudio=true|tag:audiodelay=0|tag:videosize=10176110|tag:metadatadate=Sun, 27 Feb 2011 12:00:33 +0100|tag:metadatacreator=inlet media FLVTool2 v1.0.6 - http://www.inlet-media.de/flvtool2|tag:hasCuePoints=false
> +format|filename=Enigma_Principles_of_Lust-part.flv|nb_streams=2|nb_programs=0|format_name=flv|start_time=0.000000|duration=210.209999|size=512000|bit_rate=19485|probe_score=100|tag:hasKeyframes=true|tag:hasMetadata=true|tag:datasize=11970544|tag:hasVideo=true|tag:canSeekToEnd=false|tag:lasttimestamp=210|tag:lastkeyframetimestamp=210|tag:audiosize=1791332|tag:hasAudio=true|tag:audiodelay=0|tag:videosize=10176110|tag:metadatadate=2011-02-27T11:00:33Z|tag:metadatacreator=inlet media FLVTool2 v1.0.6 - http://www.inlet-media.de/flvtool2|tag:hasCuePoints=false
> -- 