[FFmpeg-devel] avformat/utils.c: allows av_read_frame to return after a timeout period.

Submitted by Gonzalo Garramuño on Dec. 1, 2019, 11:39 a.m.

Details

Message ID 20191201113952.2909-1-ggarra13@gmail.com
State New
Headers show

Commit Message

Gonzalo Garramuño Dec. 1, 2019, 11:39 a.m.
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(+)

Comments

Andreas Rheinhardt Dec. 1, 2019, 12:05 p.m.
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
Michael Niedermayer Dec. 1, 2019, 2:37 p.m.
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

[...]
Gonzalo Garramuño Dec. 2, 2019, 3:34 p.m.
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.

Patch hide | download patch | download mbox

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