| Message ID | 20210228030122.3101246-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/wtvdec: Fix reading OLE dates on BE | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Sun, Feb 28, 2021 at 04:01:21AM +0100, Andreas Rheinhardt wrote: > The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64 > and reinterprets it as a double via av_int2double. This does not > work on big endian systems. So swap it to native endianness before > av_int2double. > > law-and-order-partial.wtv from the FATE-suite exhibited the problem. > > Thanks-to: Andriy Gelman <andriy.gelman@gmail.com> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/wtvdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c > index 7def9d2348..bb84e5dc9f 100644 > --- a/libavformat/wtvdec.c > +++ b/libavformat/wtvdec.c > @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value) > */ > static int oledate_to_iso8601(char *buf, int buf_size, int64_t value) > { > - time_t t = (av_int2double(value) - 25569.0) * 86400; > + time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400; > struct tm tmbuf; > struct tm *tm= gmtime_r(&t, &tmbuf); > if (!tm) > -- > 2.27.0 ok, please apply. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
On Sun, 28. Feb 04:01, Andreas Rheinhardt wrote: > The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64 > and reinterprets it as a double via av_int2double. This does not > work on big endian systems. So swap it to native endianness before > av_int2double. > > law-and-order-partial.wtv from the FATE-suite exhibited the problem. > > Thanks-to: Andriy Gelman <andriy.gelman@gmail.com> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/wtvdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c > index 7def9d2348..bb84e5dc9f 100644 > --- a/libavformat/wtvdec.c > +++ b/libavformat/wtvdec.c > @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value) > */ > static int oledate_to_iso8601(char *buf, int buf_size, int64_t value) > { > - time_t t = (av_int2double(value) - 25569.0) * 86400; > + time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400; > struct tm tmbuf; > struct tm *tm= gmtime_r(&t, &tmbuf); > if (!tm) I don't think that's correct because the output of avio_rl64() is already native endian. The result of: double a = (av_int2double(value) - 25569.0) * 86400; is consistent on my x86_64 and PPC64. The failing fate test seems to be because sizeof(time_t) = 4 on the BE PPC64 docker instance, but is 8 on my x86_64.
Peter Ross: > On Sun, Feb 28, 2021 at 04:01:21AM +0100, Andreas Rheinhardt wrote: >> The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64 >> and reinterprets it as a double via av_int2double. This does not >> work on big endian systems. So swap it to native endianness before >> av_int2double. >> >> law-and-order-partial.wtv from the FATE-suite exhibited the problem. >> >> Thanks-to: Andriy Gelman <andriy.gelman@gmail.com> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/wtvdec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c >> index 7def9d2348..bb84e5dc9f 100644 >> --- a/libavformat/wtvdec.c >> +++ b/libavformat/wtvdec.c >> @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value) >> */ >> static int oledate_to_iso8601(char *buf, int buf_size, int64_t value) >> { >> - time_t t = (av_int2double(value) - 25569.0) * 86400; >> + time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400; >> struct tm tmbuf; >> struct tm *tm= gmtime_r(&t, &tmbuf); >> if (!tm) >> -- >> 2.27.0 > > ok, please apply. > Unfortunately, I was wrong. avio_rl64 already implicitly performs the necessary byteswap. The actual problem is that the date in law-and-order-partial.wtv (9999-12-31 23:59:59) is outside the range representable by a 32bit time_t. So we have to write our own gmtime_r to fix this. Btw: A non-representable float->int conversion is undefined behaviour. (The Matroska fate test which started all this could of course also be fixed by simply deleting the relevant metadata entry, but I don't want to do this.) - Andreas PS: Thanks to Andriy for his continued involvement in the investigation of this. And for pointing out my error.
diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c index 7def9d2348..bb84e5dc9f 100644 --- a/libavformat/wtvdec.c +++ b/libavformat/wtvdec.c @@ -418,7 +418,7 @@ static int crazytime_to_iso8601(char *buf, int buf_size, int64_t value) */ static int oledate_to_iso8601(char *buf, int buf_size, int64_t value) { - time_t t = (av_int2double(value) - 25569.0) * 86400; + time_t t = (av_int2double(av_le2ne64(value)) - 25569.0) * 86400; struct tm tmbuf; struct tm *tm= gmtime_r(&t, &tmbuf); if (!tm)
The WTV demuxer's oledata_to_iso8601 reads a value via avio_rl64 and reinterprets it as a double via av_int2double. This does not work on big endian systems. So swap it to native endianness before av_int2double. law-and-order-partial.wtv from the FATE-suite exhibited the problem. Thanks-to: Andriy Gelman <andriy.gelman@gmail.com> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/wtvdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)