diff mbox series

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

Message ID 20210510082222.16691-1-anton@khirnov.net
State Accepted
Headers show
Series [FFmpeg-devel] lavf/flvdec: normalize exporting date metadata | expand

Checks

Context Check Description
andriy/configure warning Failed to apply patch

Commit Message

Anton Khirnov May 10, 2021, 8:22 a.m. UTC 
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(-)

-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

Comments

Alexander Strasser May 10, 2021, 1:35 p.m. UTC | #1 
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
> --
Marton Balint May 10, 2021, 5:36 p.m. UTC | #2 
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".
>
Anton Khirnov May 11, 2021, 3:51 p.m. UTC | #3 
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 May 11, 2021, 3:54 p.m. UTC | #4 
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).
Alexander Strasser May 11, 2021, 11:04 p.m. UTC | #5 
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

[...]
Anton Khirnov May 12, 2021, 6:20 p.m. UTC | #6 
pushed the patch as is, since it fixes FATE

Further comments can be applied on top of it later.
Anton Khirnov May 14, 2021, 8:09 a.m. UTC | #7 
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.
Alexander Strasser May 15, 2021, 6:20 p.m. UTC | #8 
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
Anton Khirnov May 16, 2021, 7:18 p.m. UTC | #9 
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.
Alexander Strasser May 19, 2021, 10:21 p.m. UTC | #10 
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 mbox series

Patch

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