| Message ID | 20181222184429.25080-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 8b53d1322fefa8c88c5f22645f6c4777a549cad5 |
| Headers | show |
On Sat, 22 Dec 2018, Michael Niedermayer wrote: > This causes windows to fail as the timestamp is outside its supported range > Fixes regression & fate LGTM, exactly my patch. Thanks, Marton > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mxfdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index f5e3a736e5..6e96107498 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t timestamp) > > #define SET_TS_METADATA(pb, name, var, str) do { \ > var = avio_rb64(pb); \ > - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ > + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ > return ret; \ > } while (0) > > -- > 2.20.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On 12/22/2018 3:44 PM, Michael Niedermayer wrote: > This causes windows to fail as the timestamp is outside its supported range > Fixes regression & fate > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/mxfdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > index f5e3a736e5..6e96107498 100644 > --- a/libavformat/mxfdec.c > +++ b/libavformat/mxfdec.c > @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t timestamp) > > #define SET_TS_METADATA(pb, name, var, str) do { \ > var = avio_rb64(pb); \ > - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ > + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ > return ret; \ > } while (0) This fixes the crash in the demuxer, but what about the muxing behavior? The mxf files with zero as modified_date timestamp were created by our muxer, also because gmtime_r() returns NULL in it. Is 0 within the valid range for it? Can the modified_date tag be skipped if gmtime_r() fails, or is an obligatory metadata tag in the spec?
On Sat, 22 Dec 2018, James Almer wrote: > On 12/22/2018 3:44 PM, Michael Niedermayer wrote: >> This causes windows to fail as the timestamp is outside its supported range >> Fixes regression & fate >> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/mxfdec.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> index f5e3a736e5..6e96107498 100644 >> --- a/libavformat/mxfdec.c >> +++ b/libavformat/mxfdec.c >> @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t timestamp) >> >> #define SET_TS_METADATA(pb, name, var, str) do { \ >> var = avio_rb64(pb); \ >> - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ >> + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ >> return ret; \ >> } while (0) > > This fixes the crash in the demuxer, but what about the muxing behavior? > The mxf files with zero as modified_date timestamp were created by our > muxer, also because gmtime_r() returns NULL in it. Is 0 within the valid > range for it? Can the modified_date tag be skipped if gmtime_r() fails, > or is an obligatory metadata tag in the spec? According to spec, 0 means unknown, and yes, it is required. Regards, Marton
On 12/22/2018 4:00 PM, Marton Balint wrote: > > > On Sat, 22 Dec 2018, James Almer wrote: > >> On 12/22/2018 3:44 PM, Michael Niedermayer wrote: >>> This causes windows to fail as the timestamp is outside its supported >>> range >>> Fixes regression & fate >>> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavformat/mxfdec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >>> index f5e3a736e5..6e96107498 100644 >>> --- a/libavformat/mxfdec.c >>> +++ b/libavformat/mxfdec.c >>> @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t >>> timestamp) >>> >>> #define SET_TS_METADATA(pb, name, var, str) do { \ >>> var = avio_rb64(pb); \ >>> - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, >>> mxf_timestamp_to_int64(var))) < 0) \ >>> + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, >>> mxf_timestamp_to_int64(var))) < 0) \ >>> return ret; \ >>> } while (0) >> >> This fixes the crash in the demuxer, but what about the muxing behavior? >> The mxf files with zero as modified_date timestamp were created by our >> muxer, also because gmtime_r() returns NULL in it. Is 0 within the valid >> range for it? Can the modified_date tag be skipped if gmtime_r() fails, >> or is an obligatory metadata tag in the spec? > > According to spec, 0 means unknown, and yes, it is required. > > Regards, > Marton Ok, thanks. Patch LGTM as well then.
On Sat, Dec 22, 2018 at 04:23:54PM -0300, James Almer wrote: > On 12/22/2018 4:00 PM, Marton Balint wrote: > > > > > > On Sat, 22 Dec 2018, James Almer wrote: > > > >> On 12/22/2018 3:44 PM, Michael Niedermayer wrote: > >>> This causes windows to fail as the timestamp is outside its supported > >>> range > >>> Fixes regression & fate > >>> > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavformat/mxfdec.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c > >>> index f5e3a736e5..6e96107498 100644 > >>> --- a/libavformat/mxfdec.c > >>> +++ b/libavformat/mxfdec.c > >>> @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t > >>> timestamp) > >>> > >>> #define SET_TS_METADATA(pb, name, var, str) do { \ > >>> var = avio_rb64(pb); \ > >>> - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, > >>> mxf_timestamp_to_int64(var))) < 0) \ > >>> + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, > >>> mxf_timestamp_to_int64(var))) < 0) \ > >>> return ret; \ > >>> } while (0) > >> > >> This fixes the crash in the demuxer, but what about the muxing behavior? > >> The mxf files with zero as modified_date timestamp were created by our > >> muxer, also because gmtime_r() returns NULL in it. Is 0 within the valid > >> range for it? Can the modified_date tag be skipped if gmtime_r() fails, > >> or is an obligatory metadata tag in the spec? > > > > According to spec, 0 means unknown, and yes, it is required. > > > > Regards, > > Marton > > Ok, thanks. Patch LGTM as well then. will apply thanks to everyone [...]
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index f5e3a736e5..6e96107498 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -2590,7 +2590,7 @@ static int64_t mxf_timestamp_to_int64(uint64_t timestamp) #define SET_TS_METADATA(pb, name, var, str) do { \ var = avio_rb64(pb); \ - if ((ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ + if (var && (ret = avpriv_dict_set_timestamp(&s->metadata, name, mxf_timestamp_to_int64(var))) < 0) \ return ret; \ } while (0)
This causes windows to fail as the timestamp is outside its supported range Fixes regression & fate Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mxfdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)