[FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[pal]

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

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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
>
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1046 bytes --]

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

[...]
-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1910 bytes --]

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

[...]

-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2598 bytes --]

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

[...]
-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3694 bytes --]

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

[...]
-- 
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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] format/imfdec: improve error handling when selecting tracks for playback

[Pierre-Anthony Lemieux]

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".
_______________________________________________
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".