| Message ID | 20191201113952.2909-1-ggarra13@gmail.com |
|---|---|
| State | New |
| Headers | show |
ggarra13@gmail.com: > From: Gonzalo Garramuño <ggarra13@gmail.com> > > Moved the check inside and at the end of the if (pktl) as per Michael Niedermayer's suggestion. > This patch is based on one from Blake Senftner ( bsenftner at earthlink.net ). > > The hanging in av_read_frame can be reproduced with an rtmp stream that is aborted midway and ffplay (for example) playing that stream. > --- > libavformat/utils.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 8196442dd1..653918d5a5 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -1836,6 +1836,11 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) > &s->internal->packet_buffer_end, pkt); > goto return_packet; > } > + > + if (ff_check_interrupt(&s->interrupt_callback)) { > + av_log(s, AV_LOG_DEBUG, "interrupted\n"); > + return AVERROR_EXIT; > + } > } > > ret = read_frame_internal(s, pkt); >This patch makes it possible for pkt to be still uninitialised after the call to av_read_frame() if I am not mistaken (namely if the packet list was initially not empty and the interrupt was triggered before read_frame_internal() has been called). If so, this clashes with a recently proposed patch by me (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253662.html). One could either allow av_read_frame() to return unclean packets on error or you would have to add the necessary initializations in your code (or you would have to make sure that your code is not triggered before the first call to read_frame_internal()). - Andreas
On Sun, Dec 01, 2019 at 12:05:00PM +0000, Andreas Rheinhardt wrote: > ggarra13@gmail.com: > > From: Gonzalo Garramuño <ggarra13@gmail.com> > > > > Moved the check inside and at the end of the if (pktl) as per Michael Niedermayer's suggestion. > > This patch is based on one from Blake Senftner ( bsenftner at earthlink.net ). > > > > The hanging in av_read_frame can be reproduced with an rtmp stream that is aborted midway and ffplay (for example) playing that stream. > > --- > > libavformat/utils.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/utils.c b/libavformat/utils.c > > index 8196442dd1..653918d5a5 100644 > > --- a/libavformat/utils.c > > +++ b/libavformat/utils.c > > @@ -1836,6 +1836,11 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) > > &s->internal->packet_buffer_end, pkt); > > goto return_packet; > > } > > + > > + if (ff_check_interrupt(&s->interrupt_callback)) { > > + av_log(s, AV_LOG_DEBUG, "interrupted\n"); > > + return AVERROR_EXIT; > > + } > > } > > > > ret = read_frame_internal(s, pkt); > >This patch makes it possible for pkt to be still uninitialised after > the call to av_read_frame() if I am not mistaken (namely if the packet > list was initially not empty and the interrupt was triggered before > read_frame_internal() has been called). If so, this clashes with a > recently proposed patch by me > (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253662.html). > One could either allow av_read_frame() to return unclean packets on > error or you would have to add the necessary initializations in your > code (or you would have to make sure that your code is not triggered > before the first call to read_frame_internal()). One could theoretically argue that it would make sense to only call the check after the first read_frame_internal and only before each read_frame_internal. Sadly thats not as simple to achieve as moving the code around, so i didnt suggest it previously thx [...]
On 1/12/19 09:05, Andreas Rheinhardt wrote: > ggarra13@gmail.com: >> From: Gonzalo Garramuño <ggarra13@gmail.com> >> >> Moved the check inside and at the end of the if (pktl) as per Michael Niedermayer's suggestion. >> This patch is based on one from Blake Senftner ( bsenftner at earthlink.net ). >> >> The hanging in av_read_frame can be reproduced with an rtmp stream that is aborted midway and ffplay (for example) playing that stream. >> --- >> libavformat/utils.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/libavformat/utils.c b/libavformat/utils.c >> index 8196442dd1..653918d5a5 100644 >> --- a/libavformat/utils.c >> +++ b/libavformat/utils.c >> @@ -1836,6 +1836,11 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) >> &s->internal->packet_buffer_end, pkt); >> goto return_packet; >> } >> + >> + if (ff_check_interrupt(&s->interrupt_callback)) { >> + av_log(s, AV_LOG_DEBUG, "interrupted\n"); >> + return AVERROR_EXIT; >> + } >> } >> >> ret = read_frame_internal(s, pkt); >> This patch makes it possible for pkt to be still uninitialised after > the call to av_read_frame() if I am not mistaken (namely if the packet > list was initially not empty and the interrupt was triggered before > read_frame_internal() has been called). If so, this clashes with a > recently proposed patch by me > (https://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253662.html). > One could either allow av_read_frame() to return unclean packets on > error or you would have to add the necessary initializations in your > code (or you would have to make sure that your code is not triggered > before the first call to read_frame_internal()). > > - Andreas I am afraid I do not know enough of the ffmpeg internals to make the initialization changes you propose.
diff --git a/libavformat/utils.c b/libavformat/utils.c index 8196442dd1..653918d5a5 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -1836,6 +1836,11 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) &s->internal->packet_buffer_end, pkt); goto return_packet; } + + if (ff_check_interrupt(&s->interrupt_callback)) { + av_log(s, AV_LOG_DEBUG, "interrupted\n"); + return AVERROR_EXIT; + } } ret = read_frame_internal(s, pkt);
From: Gonzalo Garramuño <ggarra13@gmail.com> Moved the check inside and at the end of the if (pktl) as per Michael Niedermayer's suggestion. This patch is based on one from Blake Senftner ( bsenftner at earthlink.net ). The hanging in av_read_frame can be reproduced with an rtmp stream that is aborted midway and ffplay (for example) playing that stream. --- libavformat/utils.c | 5 +++++ 1 file changed, 5 insertions(+)