On Mon, May 4, 2020 at 1:48 PM Michael Niedermayer <mich...@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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
