Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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(, );
> > >
> > > -// 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", );
> > > +strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", );
> > > +
> > > +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.
--
Anton Khirnov
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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(, );
> >
> > -// 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", );
> > +strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", );
> > +
> > +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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
Quoting Alexander Strasser (2021-05-12 01:04:28)
> From 3fd6c8f81baacace49e0a6cc431295dc56a077bc Mon Sep 17 00:00:00 2001
> From: Alexander Strasser
> 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(, );
>
> -// 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", );
> +strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", );
> +
> +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.
--
Anton Khirnov
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
pushed the patch as is, since it fixes FATE Further comments can be applied on top of it later. -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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
[...]
From 3fd6c8f81baacace49e0a6cc431295dc56a077bc Mon Sep 17 00:00:00 2001
From: Alexander Strasser
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(, );
-// 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", );
+strftime(datestr, sizeof(datestr), "%Y-%m-%dT%H:%M:%S", );
+
+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);
+} else
+av_strlcat(datestr, "Z", sizeof(datestr));
av_dict_set(>metadata, key, datestr, 0);
}
--
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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(, ); > > -strftime(datestr, sizeof(datestr), "%a, %d %b %Y %H:%M:%S %z", > > ); > > +gmtime_r(, ); > > + > > +// 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", ); > > > > av_dict_set(>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). -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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(, ); -strftime(datestr, sizeof(datestr), "%a, %d %b %Y %H:%M:%S %z", ); +gmtime_r(, ); + +// 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", ); av_dict_set(>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=0x|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=3/1001|avg_frame_rate=30/1|time_base=1/1000|start_pts=0|start_time=0.00|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=0x|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.00|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.00|duration=210.20|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
Re: [FFmpeg-devel] [PATCH] lavf/flvdec: normalize exporting date metadata
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(, ); > -strftime(datestr, sizeof(datestr), "%a, %d %b %Y %H:%M:%S %z", > ); > +gmtime_r(, ); > + > +// 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", ); > > av_dict_set(>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=0x|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=3/1001|avg_frame_rate=30/1|time_base=1/1000|start_pts=0|start_time=0.00|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 > >
