| Message ID | 20220907200233.21255-1-pal@sandflow.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] format/imfdec: improve error handling when selecting tracks for playback | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Ping. This patchset addresses a coverity issue #1512414. On Wed, Sep 7, 2022 at 1:02 PM <pal@sandflow.com> wrote: > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > Addresses coverity 1512414 > > --- > libavformat/imfdec.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > index 5bbe7a53f8..58b1455c9f 100644 > --- a/libavformat/imfdec.c > +++ b/libavformat/imfdec.c > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > { > IMFContext *c = s->priv_data; > IMFVirtualTrackPlaybackCtx *track; > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > + > + if (!c->track_count) > + return NULL; > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > for (uint32_t i = c->track_count; i > 0; i--) { > av_log(s, AV_LOG_TRACE, "Compare track %d timestamp " AVRATIONAL_FORMAT > " to minimum " AVRATIONAL_FORMAT > @@ -697,8 +700,6 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > } > } > > - av_log(s, AV_LOG_DEBUG, "Found next track to read: %d (timestamp: %lf / %lf)\n", > - track->index, av_q2d(track->current_timestamp), av_q2d(minimum_timestamp)); > return track; > } > > @@ -761,6 +762,14 @@ static int imf_read_packet(AVFormatContext *s, AVPacket *pkt) > > track = get_next_track_with_minimum_timestamp(s); > > + if (!track) { > + av_log(s, AV_LOG_ERROR, "No track found for playback\n"); > + return AVERROR_INVALIDDATA; > + } > + > + av_log(s, AV_LOG_DEBUG, "Found track %d to read at timestamp %lf\n", > + track->index, av_q2d(track->current_timestamp)); > + > ret = get_resource_context_for_timestamp(s, track, &resource); > if (ret) > return ret; > -- > 2.25.1 >
On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > Addresses coverity 1512414 > > --- > libavformat/imfdec.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > index 5bbe7a53f8..58b1455c9f 100644 > --- a/libavformat/imfdec.c > +++ b/libavformat/imfdec.c > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > { > IMFContext *c = s->priv_data; > IMFVirtualTrackPlaybackCtx *track; > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); iam not sure thats a good idea, it would change behavior on different platforms thx [...]
On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > Addresses coverity 1512414 > > > > --- > > libavformat/imfdec.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > index 5bbe7a53f8..58b1455c9f 100644 > > --- a/libavformat/imfdec.c > > +++ b/libavformat/imfdec.c > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > { > > IMFContext *c = s->priv_data; > > IMFVirtualTrackPlaybackCtx *track; > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > iam not sure thats a good idea, it would change behavior on different platforms I was following the earlier observations that "The initialization of minimum_timestamp presumes that int are 32bit which need not be true": http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html If I am not mistaken, the numerator and denominator of AVRational are defined as `int`: https://www.ffmpeg.org/doxygen/trunk/structAVRational.html What would be the alternative? > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > it is not once nor twice but times without number that the same ideas make > their appearance in the world. -- Aristotle > _______________________________________________ > 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".
On Fri, Sep 16, 2022 at 10:39 AM Pierre-Anthony Lemieux <pal@sandflow.com> wrote: > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > Addresses coverity 1512414 > > > > > > --- > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > index 5bbe7a53f8..58b1455c9f 100644 > > > --- a/libavformat/imfdec.c > > > +++ b/libavformat/imfdec.c > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > { > > > IMFContext *c = s->priv_data; > > > IMFVirtualTrackPlaybackCtx *track; > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > iam not sure thats a good idea, it would change behavior on different platforms > > I was following the earlier observations that "The initialization of > minimum_timestamp presumes that int are 32bit which need not be true": > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > If I am not mistaken, the numerator and denominator of AVRational are > defined as `int`: > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > What would be the alternative? ... and see similar code at: https://github.com/FFmpeg/FFmpeg/blob/f05247f6a4698c14f1cd523daa90188f50dcf6ad/libavcodec/mpeg12framerate.c#L51 > > > > > thx > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > it is not once nor twice but times without number that the same ideas make > > their appearance in the world. -- Aristotle > > _______________________________________________ > > 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".
On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > Addresses coverity 1512414 > > > > > > --- > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > index 5bbe7a53f8..58b1455c9f 100644 > > > --- a/libavformat/imfdec.c > > > +++ b/libavformat/imfdec.c > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > { > > > IMFContext *c = s->priv_data; > > > IMFVirtualTrackPlaybackCtx *track; > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > iam not sure thats a good idea, it would change behavior on different platforms > > I was following the earlier observations that "The initialization of > minimum_timestamp presumes that int are 32bit which need not be true": > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > If I am not mistaken, the numerator and denominator of AVRational are > defined as `int`: > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > What would be the alternative? I dont understand why this code uses AVRational for timestamps normally timestamps are int64_t with a AVRational timebase and would use av_compare_ts() for comparing thx [...]
On Fri, Sep 16, 2022 at 1:15 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > > > Addresses coverity 1512414 > > > > > > > > --- > > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > > index 5bbe7a53f8..58b1455c9f 100644 > > > > --- a/libavformat/imfdec.c > > > > +++ b/libavformat/imfdec.c > > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > > { > > > > IMFContext *c = s->priv_data; > > > > IMFVirtualTrackPlaybackCtx *track; > > > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > > > iam not sure thats a good idea, it would change behavior on different platforms > > > > I was following the earlier observations that "The initialization of > > minimum_timestamp presumes that int are 32bit which need not be true": > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > > > If I am not mistaken, the numerator and denominator of AVRational are > > defined as `int`: > > > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > > > What would be the alternative? > > I dont understand why this code uses AVRational for timestamps > normally timestamps are int64_t with a AVRational timebase and would use > av_compare_ts() for comparing Temporal offsets on IMF tracks are typically expressed as rationals, and each track can have its own timebase. > > thx > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > When you are offended at any man's fault, turn to yourself and study your > own failings. Then you will forget your anger. -- Epictetus > _______________________________________________ > 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".
On Fri, Sep 16, 2022 at 01:19:09PM -0700, Pierre-Anthony Lemieux wrote: > On Fri, Sep 16, 2022 at 1:15 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > > > <michael@niedermayer.cc> wrote: > > > > > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > > > > > Addresses coverity 1512414 > > > > > > > > > > --- > > > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > > > index 5bbe7a53f8..58b1455c9f 100644 > > > > > --- a/libavformat/imfdec.c > > > > > +++ b/libavformat/imfdec.c > > > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > > > { > > > > > IMFContext *c = s->priv_data; > > > > > IMFVirtualTrackPlaybackCtx *track; > > > > > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > > > > > iam not sure thats a good idea, it would change behavior on different platforms > > > > > > I was following the earlier observations that "The initialization of > > > minimum_timestamp presumes that int are 32bit which need not be true": > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > > > > > If I am not mistaken, the numerator and denominator of AVRational are > > > defined as `int`: > > > > > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > > > > > What would be the alternative? > > > > I dont understand why this code uses AVRational for timestamps > > normally timestamps are int64_t with a AVRational timebase and would use > > av_compare_ts() for comparing > > Temporal offsets on IMF tracks are typically expressed as rationals, > and each track can have its own timebase. av_compare_ts() can compare 2 timestamps in 2 different timebases the problem with using AVRational for timestamps in 1sec units is that alot of 64bit * 32/32bit timestamps cannot be represented exactly thx [...]
On Sat, Sep 17, 2022 at 7:43 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Fri, Sep 16, 2022 at 01:19:09PM -0700, Pierre-Anthony Lemieux wrote: > > On Fri, Sep 16, 2022 at 1:15 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > > > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > > > > > > > Addresses coverity 1512414 > > > > > > > > > > > > --- > > > > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > > > > index 5bbe7a53f8..58b1455c9f 100644 > > > > > > --- a/libavformat/imfdec.c > > > > > > +++ b/libavformat/imfdec.c > > > > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > > > > { > > > > > > IMFContext *c = s->priv_data; > > > > > > IMFVirtualTrackPlaybackCtx *track; > > > > > > > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > > > > > > > iam not sure thats a good idea, it would change behavior on different platforms > > > > > > > > I was following the earlier observations that "The initialization of > > > > minimum_timestamp presumes that int are 32bit which need not be true": > > > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > > > > > > > If I am not mistaken, the numerator and denominator of AVRational are > > > > defined as `int`: > > > > > > > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > > > > > > > What would be the alternative? > > > > > > I dont understand why this code uses AVRational for timestamps > > > normally timestamps are int64_t with a AVRational timebase and would use > > > av_compare_ts() for comparing > > > > Temporal offsets on IMF tracks are typically expressed as rationals, > > and each track can have its own timebase. > > av_compare_ts() can compare 2 timestamps in 2 different timebases > > the problem with using AVRational for timestamps in 1sec units is > that alot of 64bit * 32/32bit timestamps cannot be represented exactly Ok. It might be possible to refactor the imfdec timing model in terms of (int) time offset + timebase instead of (rational) time offset, or at least detect when the current scheme fails. This will require careful study. Any objections to tackling this refactoring as a separate patchset, since this patchet is intended to address the coverity issue? > > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In fact, the RIAA has been known to suggest that students drop out > of college or go to community college in order to be able to afford > settlements. -- The RIAA > _______________________________________________ > 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".
On Sat, Sep 17, 2022 at 08:57:37AM -0700, Pierre-Anthony Lemieux wrote: > On Sat, Sep 17, 2022 at 7:43 AM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Fri, Sep 16, 2022 at 01:19:09PM -0700, Pierre-Anthony Lemieux wrote: > > > On Fri, Sep 16, 2022 at 1:15 PM Michael Niedermayer > > > <michael@niedermayer.cc> wrote: > > > > > > > > On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > > > > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > > > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > > > > > > > > > Addresses coverity 1512414 > > > > > > > > > > > > > > --- > > > > > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > > > > > index 5bbe7a53f8..58b1455c9f 100644 > > > > > > > --- a/libavformat/imfdec.c > > > > > > > +++ b/libavformat/imfdec.c > > > > > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > > > > > { > > > > > > > IMFContext *c = s->priv_data; > > > > > > > IMFVirtualTrackPlaybackCtx *track; > > > > > > > > > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > > > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > > > > > > > > > iam not sure thats a good idea, it would change behavior on different platforms > > > > > > > > > > I was following the earlier observations that "The initialization of > > > > > minimum_timestamp presumes that int are 32bit which need not be true": > > > > > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > > > > > > > > > If I am not mistaken, the numerator and denominator of AVRational are > > > > > defined as `int`: > > > > > > > > > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > > > > > > > > > What would be the alternative? > > > > > > > > I dont understand why this code uses AVRational for timestamps > > > > normally timestamps are int64_t with a AVRational timebase and would use > > > > av_compare_ts() for comparing > > > > > > Temporal offsets on IMF tracks are typically expressed as rationals, > > > and each track can have its own timebase. > > > > av_compare_ts() can compare 2 timestamps in 2 different timebases > > > > the problem with using AVRational for timestamps in 1sec units is > > that alot of 64bit * 32/32bit timestamps cannot be represented exactly > > Ok. It might be possible to refactor the imfdec timing model in terms > of (int) time offset + timebase instead of (rational) time offset, or > at least detect when the current scheme fails. This will require > careful study. > > Any objections to tackling this refactoring as a separate patchset, > since this patchet is intended to address the coverity issue? you can do it in whichever way works best for you. Iam just saying that AVRational with more than 32/32bit is problematic consider av_add_q() it expects the product of various fields to fit int64_t if int is 64bit AND AVRational uses teh extra range the implementation needs a 128bit type or some other modification thx [...]
On Mon, Sep 19, 2022 at 7:42 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sat, Sep 17, 2022 at 08:57:37AM -0700, Pierre-Anthony Lemieux wrote: > > On Sat, Sep 17, 2022 at 7:43 AM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Fri, Sep 16, 2022 at 01:19:09PM -0700, Pierre-Anthony Lemieux wrote: > > > > On Fri, Sep 16, 2022 at 1:15 PM Michael Niedermayer > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > On Fri, Sep 16, 2022 at 10:39:22AM -0700, Pierre-Anthony Lemieux wrote: > > > > > > On Fri, Sep 16, 2022 at 10:27 AM Michael Niedermayer > > > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > > > > > On Wed, Sep 07, 2022 at 01:02:33PM -0700, pal@sandflow.com wrote: > > > > > > > > From: Pierre-Anthony Lemieux <pal@palemieux.com> > > > > > > > > > > > > > > > > Addresses coverity 1512414 > > > > > > > > > > > > > > > > --- > > > > > > > > libavformat/imfdec.c | 15 ++++++++++++--- > > > > > > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c > > > > > > > > index 5bbe7a53f8..58b1455c9f 100644 > > > > > > > > --- a/libavformat/imfdec.c > > > > > > > > +++ b/libavformat/imfdec.c > > > > > > > > @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma > > > > > > > > { > > > > > > > > IMFContext *c = s->priv_data; > > > > > > > > IMFVirtualTrackPlaybackCtx *track; > > > > > > > > > > > > > > > + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); > > > > > > > > - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); > > > > > > > > > > > > > > iam not sure thats a good idea, it would change behavior on different platforms > > > > > > > > > > > > I was following the earlier observations that "The initialization of > > > > > > minimum_timestamp presumes that int are 32bit which need not be true": > > > > > > > > > > > > http://ffmpeg.org/pipermail/ffmpeg-devel/2022-August/300587.html > > > > > > > > > > > > If I am not mistaken, the numerator and denominator of AVRational are > > > > > > defined as `int`: > > > > > > > > > > > > https://www.ffmpeg.org/doxygen/trunk/structAVRational.html > > > > > > > > > > > > What would be the alternative? > > > > > > > > > > I dont understand why this code uses AVRational for timestamps > > > > > normally timestamps are int64_t with a AVRational timebase and would use > > > > > av_compare_ts() for comparing > > > > > > > > Temporal offsets on IMF tracks are typically expressed as rationals, > > > > and each track can have its own timebase. > > > > > > av_compare_ts() can compare 2 timestamps in 2 different timebases > > > > > > the problem with using AVRational for timestamps in 1sec units is > > > that alot of 64bit * 32/32bit timestamps cannot be represented exactly > > > > Ok. It might be possible to refactor the imfdec timing model in terms > > of (int) time offset + timebase instead of (rational) time offset, or > > at least detect when the current scheme fails. This will require > > careful study. > > > > > Any objections to tackling this refactoring as a separate patchset, > > since this patchet is intended to address the coverity issue? > > you can do it in whichever way works best for you. > Iam just saying that AVRational with more than 32/32bit is problematic Ok. Will revert to initializing minimum_timestamp with INT32_MAX for now so that we can merge this fix. > > consider av_add_q() it expects the product of various fields to fit > int64_t > > if int is 64bit AND AVRational uses teh extra range the implementation > needs a 128bit type or some other modification > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Into a blind darkness they enter who follow after the Ignorance, > they as if into a greater darkness enter who devote themselves > to the Knowledge alone. -- Isha Upanishad > _______________________________________________ > 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".
diff --git a/libavformat/imfdec.c b/libavformat/imfdec.c index 5bbe7a53f8..58b1455c9f 100644 --- a/libavformat/imfdec.c +++ b/libavformat/imfdec.c @@ -681,8 +681,11 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma { IMFContext *c = s->priv_data; IMFVirtualTrackPlaybackCtx *track; + AVRational minimum_timestamp = av_make_q(INT_MAX, 1); + + if (!c->track_count) + return NULL; - AVRational minimum_timestamp = av_make_q(INT32_MAX, 1); for (uint32_t i = c->track_count; i > 0; i--) { av_log(s, AV_LOG_TRACE, "Compare track %d timestamp " AVRATIONAL_FORMAT " to minimum " AVRATIONAL_FORMAT @@ -697,8 +700,6 @@ static IMFVirtualTrackPlaybackCtx *get_next_track_with_minimum_timestamp(AVForma } } - av_log(s, AV_LOG_DEBUG, "Found next track to read: %d (timestamp: %lf / %lf)\n", - track->index, av_q2d(track->current_timestamp), av_q2d(minimum_timestamp)); return track; } @@ -761,6 +762,14 @@ static int imf_read_packet(AVFormatContext *s, AVPacket *pkt) track = get_next_track_with_minimum_timestamp(s); + if (!track) { + av_log(s, AV_LOG_ERROR, "No track found for playback\n"); + return AVERROR_INVALIDDATA; + } + + av_log(s, AV_LOG_DEBUG, "Found track %d to read at timestamp %lf\n", + track->index, av_q2d(track->current_timestamp)); + ret = get_resource_context_for_timestamp(s, track, &resource); if (ret) return ret;
From: Pierre-Anthony Lemieux <pal@palemieux.com> Addresses coverity 1512414 --- libavformat/imfdec.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)