Message ID | 20210510082222.16691-1-anton@khirnov.net |
---|---|
State | Accepted |
Headers | show |
Series | [FFmpeg-devel] lavf/flvdec: normalize exporting date metadata | expand |
Context | Check | Description |
---|---|---|
andriy/configure | warning | Failed to apply patch |
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_thumbna il > 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 > --
On Mon, 10 May 2021, 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). > > 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? > --- > 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); Maybe avpriv_dict_set_timestamp() should be used instead. That way milisecond precision can also be respected. Thanks, Marton > } > 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_thumbnai l > 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 > -- > 2.30.2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
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.
Quoting Marton Balint (2021-05-10 19:36:59) > > > On Mon, 10 May 2021, 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). > > > > 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? > > --- > > 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); > > Maybe avpriv_dict_set_timestamp() should be used instead. That way > milisecond precision can also be respected. Huh, I did not know about that function. But since we are converting from double, I am somewhat concerned about the result remaining the same on all platforms (which is the point of this patch).
On 2021-05-11 17:51 +0200, Anton Khirnov wrote: > 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, FLV reuses AMF0 spec for serialization of the metadata we talk about? > 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. Interesting. In case of that sample it is probably FLVTool2 that wrote that wrote the metadata. Then this should be its Ruby code for handling the AMF date type: def write__AMF_date(time) write__UI8 11 write [(time.to_f * 1000.0)].pack('G') write__SI16( (Time.now.gmtoff / 60).to_i ) end def read__AMF_date utc_time = Time.at((read__AMF_double / 1000).to_i) utc_time + (read__SI16 * 60) - Time.now.gmtoff end I have a attached patch, that on top of your patch, adds output of timezone information according to the quoted interpretation of FLVTool2. Best regards, Alexander [...]
pushed the patch as is, since it fixes FATE Further comments can be applied on top of it later.
Quoting Alexander Strasser (2021-05-12 01:04:28) > From 3fd6c8f81baacace49e0a6cc431295dc56a077bc Mon Sep 17 00:00:00 2001 > From: Alexander Strasser <eclipse7@gmx.net> > Date: Wed, 12 May 2021 00:46:54 +0200 > Subject: [PATCH] lavf/flvdec: metadata date: respect timezone information if > present > > If the timezone data of an AMF 0 date type is non-zero, include that > information as UTC offset in hours and minutes. > --- > libavformat/flvdec.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > index ddaceaafe4..c941e62e23 100644 > --- a/libavformat/flvdec.c > +++ b/libavformat/flvdec.c > @@ -688,9 +688,21 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream, > time = date.milliseconds / 1000; // to seconds > 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); > + strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", &t); > + > + if (date.timezone) { > + int off_tz = date.timezone; // offset in minutes > + char ch_sign = '+'; > + if (off_tz < 0) { > + off_tz = -off_tz; > + ch_sign = '-'; > + } > + if (off_tz > 99*60 + 59) > + off_tz = 99*60 + 59; > + > + av_strlcatf(datestr, sizeof(datestr), "%c%02d%02d", ch_sign, off_tz / 60, off_tz % 60); I still believe this is wrong, since the spec says the timestamp is in UTC. The code you quoted seems to conform to that.
Hi Anton! On 2021-05-14 10:09 +0200, Anton Khirnov wrote: > Quoting Alexander Strasser (2021-05-12 01:04:28) > > > > If the timezone data of an AMF 0 date type is non-zero, include that > > information as UTC offset in hours and minutes. > > --- > > libavformat/flvdec.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > > index ddaceaafe4..c941e62e23 100644 > > --- a/libavformat/flvdec.c > > +++ b/libavformat/flvdec.c > > @@ -688,9 +688,21 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream, > > time = date.milliseconds / 1000; // to seconds > > 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); > > + strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", &t); > > + > > + if (date.timezone) { > > + int off_tz = date.timezone; // offset in minutes > > + char ch_sign = '+'; > > + if (off_tz < 0) { > > + off_tz = -off_tz; > > + ch_sign = '-'; > > + } > > + if (off_tz > 99*60 + 59) > > + off_tz = 99*60 + 59; > > + > > + av_strlcatf(datestr, sizeof(datestr), "%c%02d%02d", ch_sign, off_tz / 60, off_tz % 60); > > I still believe this is wrong, since the spec says the timestamp is in > UTC. The code you quoted seems to conform to that. Not to my understanding and testing. This Ruby program t1 = Time.now() t2 = Time.now.utc() print t1, " - ", t1.to_f, "\n" print t2, " - ", t2.to_f, "\n" yields for example: 2021-05-15 20:05:19 +0200 - 1621101919.509961 2021-05-15 18:05:19 UTC - 1621101919.509966 Returning to the code I quoted before now and stating my understanding of if now. def write__AMF_date(time) write__UI8 11 write [(time.to_f * 1000.0)].pack('G') write__SI16( (Time.now.gmtoff / 60).to_i ) end This writes the time in micro seconds without offset as double. The GMT offset in minutes is written afterwards as signed 16 bit integer. def read__AMF_date utc_time = Time.at((read__AMF_double / 1000).to_i) utc_time + (read__SI16 * 60) - Time.now.gmtoff end This first reads the double and converts it into a Time object. In the following line it reads and adds the stored offset and subtracts the current offset to get rid of its influence. Alexander
Quoting Alexander Strasser (2021-05-15 20:20:30) > Hi Anton! > > On 2021-05-14 10:09 +0200, Anton Khirnov wrote: > > Quoting Alexander Strasser (2021-05-12 01:04:28) > > > > > > If the timezone data of an AMF 0 date type is non-zero, include that > > > information as UTC offset in hours and minutes. > > > --- > > > libavformat/flvdec.c | 18 +++++++++++++++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c > > > index ddaceaafe4..c941e62e23 100644 > > > --- a/libavformat/flvdec.c > > > +++ b/libavformat/flvdec.c > > > @@ -688,9 +688,21 @@ static int amf_parse_object(AVFormatContext *s, AVStream *astream, > > > time = date.milliseconds / 1000; // to seconds > > > 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); > > > + strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", &t); > > > + > > > + if (date.timezone) { > > > + int off_tz = date.timezone; // offset in minutes > > > + char ch_sign = '+'; > > > + if (off_tz < 0) { > > > + off_tz = -off_tz; > > > + ch_sign = '-'; > > > + } > > > + if (off_tz > 99*60 + 59) > > > + off_tz = 99*60 + 59; > > > + > > > + av_strlcatf(datestr, sizeof(datestr), "%c%02d%02d", ch_sign, off_tz / 60, off_tz % 60); > > > > I still believe this is wrong, since the spec says the timestamp is in > > UTC. The code you quoted seems to conform to that. > > Not to my understanding and testing. > > This Ruby program > > t1 = Time.now() > t2 = Time.now.utc() > print t1, " - ", t1.to_f, "\n" > print t2, " - ", t2.to_f, "\n" > > yields for example: > > 2021-05-15 20:05:19 +0200 - 1621101919.509961 > 2021-05-15 18:05:19 UTC - 1621101919.509966 This is just proving my point: the timestamp is UTC in both cases. The only difference is that one of them has a non-zero timezone offset attached. > > > Returning to the code I quoted before now and stating my > understanding of if now. > > def write__AMF_date(time) > write__UI8 11 > write [(time.to_f * 1000.0)].pack('G') > write__SI16( (Time.now.gmtoff / 60).to_i ) > end > > This writes the time in micro seconds without offset as double. > The GMT offset in minutes is written afterwards as signed 16 bit > integer. > > > def read__AMF_date > utc_time = Time.at((read__AMF_double / 1000).to_i) > utc_time + (read__SI16 * 60) - Time.now.gmtoff > end > > This first reads the double and converts it into a Time object. > In the following line it reads and adds the stored offset and > subtracts the current offset to get rid of its influence. The writing code looks correct, but the reading code seems suspicious. To test it, I used flvtool2 to write metadata with TZ=America/New_York (-04:00) at 19:08 UTC. Then reading it with TZ=Europe/Paris (+02:00) gives me: - ffmpeg 4.3.2 -- correct, prints date in local timezone metadatadate : Sun, 16 May 2021 21:08:41 +0200 - current ffmpeg git master -- correct, prints date in UTC metadatadate : 2021-05-16T19:08:41Z - flvtool2 -- WRONG metadatadate: Sun May 16 15:08:41 GMT+0200 2021 From which I conclude that flvtool2's own reading code is incorrect.
On 2021-05-16 21:18 +0200, Anton Khirnov wrote: > Quoting Alexander Strasser (2021-05-15 20:20:30) [...] > > > > Returning to the code I quoted before now and stating my > > understanding of if now. > > > > def write__AMF_date(time) > > write__UI8 11 > > write [(time.to_f * 1000.0)].pack('G') > > write__SI16( (Time.now.gmtoff / 60).to_i ) > > end > > > > This writes the time in micro seconds without offset as double. > > The GMT offset in minutes is written afterwards as signed 16 bit > > integer. > > > > > > def read__AMF_date > > utc_time = Time.at((read__AMF_double / 1000).to_i) > > utc_time + (read__SI16 * 60) - Time.now.gmtoff > > end > > > > This first reads the double and converts it into a Time object. > > In the following line it reads and adds the stored offset and > > subtracts the current offset to get rid of its influence. > > The writing code looks correct, but the reading code seems suspicious. Actually I think both are wrong. I was fooled before because the resulting output of flvtool2 is wrong if the timezone data is zero. > To test it, I used flvtool2 to write metadata with TZ=America/New_York > (-04:00) at 19:08 UTC. Then reading it with TZ=Europe/Paris (+02:00) > gives me: > - ffmpeg 4.3.2 -- correct, prints date in local timezone > metadatadate : Sun, 16 May 2021 21:08:41 +0200 > - current ffmpeg git master -- correct, prints date in UTC > metadatadate : 2021-05-16T19:08:41Z > - flvtool2 -- WRONG > metadatadate: Sun May 16 15:08:41 GMT+0200 2021 > > From which I conclude that flvtool2's own reading code is incorrect. I'm not convinced that interpreting the timezone data will be a good thing. Therefore I think the new comment in the code is probably not good advice. I don't mind it much though, as at least for the files written by flvtool2 it isn't wrong. I don't have any more examples of this, despite tools writing a UTC timestamp and setting timezone to zero always. That the timezone offset written next to a UTC timestamp should indicate the timezone of the system that wrote it seems unusual. What useful information would that convey? Probably some vague location information. On the bright side: Current and previous FFmpeg code always did the right thing for metadata dates written by flvtool2 :) I withdraw my patch as from what we know now it would give wrong output for metadata files that were generated by flvtool2. FWIW I have opened a bug report for flvtool2: https://github.com/unnu/flvtool2/issues/10 Though it seems not to be maintained since around 2014. Thanks for looking into this, Anton. Best regards, Alexander
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