| Message ID | 20200809105653.93859-2-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] lavfi/avf_concat: add some doxy. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Nicolas George (12020-08-09): > If an input has an undefined timestamp, the computation > for silence at stitches can overflow. > > Partial fix for trac ticket #8843. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/avf_concat.c | 4 ++++ > 1 file changed, 4 insertions(+) Ping? Will apply sometime soon if nobody objects. Regards,
On Sun, Aug 09, 2020 at 12:56:53PM +0200, Nicolas George wrote: > If an input has an undefined timestamp, the computation > for silence at stitches can overflow. > > Partial fix for trac ticket #8843. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/avf_concat.c | 4 ++++ > 1 file changed, 4 insertions(+) > > > "Partial fix" means it does not segfault and exits with an error instead. > > The actual problem in this ticket is amix sending a frame with no PTS. > > > diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c > index 246628498a..f376f72754 100644 > --- a/libavfilter/avf_concat.c > +++ b/libavfilter/avf_concat.c > @@ -179,6 +179,10 @@ static int push_frame(AVFilterContext *ctx, unsigned in_no, AVFrame *buf) > AVFilterLink *outlink = ctx->outputs[out_no]; > struct concat_in *in = &cat->in[in_no]; > > + if (buf->pts == AV_NOPTS_VALUE) { > + av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no); > + return AVERROR(EINVAL); > + } what if the timestamp is "AV_NOPTS_VALUE + 1" ? would that work or overflow ? also could the filter not use the previous frame pts + "duration" when a PTS is unavailable (maybe that would make sense in some cases) (with a warning) thx [...]
Michael Niedermayer (12020-08-23): > > + if (buf->pts == AV_NOPTS_VALUE) { > > + av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no); > > + return AVERROR(EINVAL); > > + } > > what if the timestamp is "AV_NOPTS_VALUE + 1" ? > would that work or overflow ? I have no idea, I suspect it would overflow too. But "every single filter that needs valid timestamps" is not the place to protect against overflows caused by widely invalid timestamps. The framework is; I have considered going at it, some kind of "require_valid_pts" flag on input pads, but it is not my priority right now. This test is already too much, it should be part of the framework, but it is very simple and protects against a real current bug. > also could the filter not use the previous frame pts + "duration" when a PTS > is unavailable (maybe that would make sense in some cases) (with a warning) Not really. First, again, an individual filter is not the place to implement an heuristic. Second, duration is not part of lavfi's working at all, and could not be without a significant redesign of many filters, and increasing the latency. Regards,
On 8/23/20, Nicolas George <george@nsup.org> wrote: > Michael Niedermayer (12020-08-23): >> > + if (buf->pts == AV_NOPTS_VALUE) { >> > + av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input >> > %d\n", in_no); >> > + return AVERROR(EINVAL); >> > + } >> >> what if the timestamp is "AV_NOPTS_VALUE + 1" ? >> would that work or overflow ? > > I have no idea, I suspect it would overflow too. But "every single > filter that needs valid timestamps" is not the place to protect against > overflows caused by widely invalid timestamps. The framework is; I have > considered going at it, some kind of "require_valid_pts" flag on input > pads, but it is not my priority right now. What is valid timestamp? Filter currently crashes because it receives nopts timestamps. Why it crashes at all? Does it crash because it access out of range of allocated memory? If yes, make sure this patch is real fix and not just band-aid. > > This test is already too much, it should be part of the framework, but > it is very simple and protects against a real current bug. > >> also could the filter not use the previous frame pts + "duration" when a >> PTS >> is unavailable (maybe that would make sense in some cases) (with a >> warning) > > Not really. > > First, again, an individual filter is not the place to implement an > heuristic. > > Second, duration is not part of lavfi's working at all, and could not be > without a significant redesign of many filters, and increasing the > latency. > > Regards, > > -- > Nicolas George >
diff --git a/libavfilter/avf_concat.c b/libavfilter/avf_concat.c index 246628498a..f376f72754 100644 --- a/libavfilter/avf_concat.c +++ b/libavfilter/avf_concat.c @@ -179,6 +179,10 @@ static int push_frame(AVFilterContext *ctx, unsigned in_no, AVFrame *buf) AVFilterLink *outlink = ctx->outputs[out_no]; struct concat_in *in = &cat->in[in_no]; + if (buf->pts == AV_NOPTS_VALUE) { + av_log(ctx, AV_LOG_ERROR, "Frame without timestamp on input %d\n", in_no); + return AVERROR(EINVAL); + } buf->pts = av_rescale_q(buf->pts, inlink->time_base, outlink->time_base); in->pts = buf->pts; in->nb_frames++;
If an input has an undefined timestamp, the computation for silence at stitches can overflow. Partial fix for trac ticket #8843. Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/avf_concat.c | 4 ++++ 1 file changed, 4 insertions(+) "Partial fix" means it does not segfault and exits with an error instead. The actual problem in this ticket is amix sending a frame with no PTS.