[FFmpeg-devel] [PATCH] [libavutil] Add saturated add/sub operations for int64_t.

[Dale Curtis]

On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> this could be done, but iam unsure this API is optimal
>
> Maybe its best to show an example, why iam unsure about the API
>

Thanks, but maybe a more concrete case to look at would be the patch I sent
for fixing skip samples: "Avoid integer overflow on start_time with
skip_samples."  Here's the current proposed fix:

@@ -1155,8 +1155,11 @@ static void
update_initial_timestamps(AVFormatContext *s, int stream_index,

         if (st->start_time == AV_NOPTS_VALUE && pktl_it->pkt.pts !=
AV_NOPTS_VALUE) {
             st->start_time = pktl_it->pkt.pts;
-            if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-                st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+            if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate) {
+                int64_t skip_time = av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+                if (st->start_time > 0 ? skip_time <= INT64_MAX -
st->start_time : skip_time >= INT64_MIN - st->start_time)
+                    st->start_time += skip_time;
+            }
         }
     }

With the APIs we're discussing the new fix would be either:
            if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-                st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+                st->start_time = av_sat_add64(st->start_time,
av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate},
st->time_base))

or with checked overflow:

-            if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)
-                st->start_time += av_rescale_q(st->skip_samples,
(AVRational){1, st->codecpar->sample_rate}, st->time_base);
+            if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO &&
st->codecpar->sample_rate)  {
+                int64_t tmp;
+                if (!av_checked_sat_add64(st->start_time,
av_rescale_q(st->skip_samples, (AVRational){1, st->codecpar->sample_rate},
st->time_base), &tmp))
+                    st->start_time = tmp;
+            }


> lets consider a simple random expression
> a*x + b*y
>
> overflow  = av_checked_sat_mul64(a, x, &T0);
> overflow |= av_checked_sat_mul64(b, y, &T1);
> overflow |= av_checked_sat_add64(T0, T1, &result);
> if (overflow)
>     ...
>
> vs.
>
> int64_t result = av_add_eint64( av_mul_eint64(a, x),
>                                 av_mul_eint64(b, y) );
> if (!av_is_finite_eint64(result))
>     ....
>
> To me the 2nd variant looks easier to read, (eint here is supposed to mean
> extended integer, that is extended by +/- infinity and NaN with IEEE like
> semantics)


> also the NaN element should have the same value as AV_NOPTS_VALUE, that
> would
> likely be most usefull.
> This could also allow the removial of alot of AV_NOPTS_VALUE special
> casing ...
>

Are you just proposing sentinel values for those extensions? E.g., +inf =
INT64_MAX, -inf=-INT64_MAX, nan=INT64_MIN? It seems like you could just
layer that on top of the saturated versions I'm proposing here. I don't
think I'd recommend that solution though, instead it seems more legible and
familiar to just use a float/double type in those cases where it'd be
relevant. Once you start introducing sentinel checks everywhere, I doubt
you'd keep much if any performance over a known type like float/double.


>
> But this is independant of the pure integer saturation API and should
> probably
> not hold it up when that itself is needed.
>
> thx
>