[FFmpeg-devel] avformat/flvdec: Change packet loop to return EAGAIN instead of looping until a valid packet is foud

[Nicolas George]

Le decadi 30 fructidor, an CCXXIII, Michael Niedermayer a écrit :
> ffmpeg | branch: master | Michael Niedermayer <michael at niedermayer.cc> | Wed Sep 16 01:39:18 2015 +0200| [3496a20bb92570aaa82849f0d5409f2e29fe2d2b] | committer: Michael Niedermayer
> 
> avformat/flvdec: Change packet loop to return EAGAIN instead of looping until a valid packet is foud
> 
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> 
> > http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=3496a20bb92570aaa82849f0d5409f2e29fe2d2b
> ---
> 
>  libavformat/flvdec.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 826e0d7..cec40e0 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -832,8 +832,10 @@ static int flv_read_packet(AVFormatContext *s, AVPacket *pkt)
>              }
>          }
>  
> -        if (size == 0)
> -            continue;
> +        if (size == 0) {
> +            ret = AVERROR(EAGAIN);
> +            goto leave;
> +        }
>  
>          next = size + avio_tell(s->pb);
>  
> @@ -876,12 +878,15 @@ static int flv_read_packet(AVFormatContext *s, AVPacket *pkt)
>                     type, size, flags);
>  skip:
>              avio_seek(s->pb, next, SEEK_SET);
> -            continue;
> +            ret = AVERROR(EAGAIN);
> +            goto leave;
>          }
>  
>          /* skip empty data packets */
> -        if (!size)
> -            continue;
> +        if (!size) {
> +            ret = AVERROR(EAGAIN);
> +            goto leave;
> +        }
>  
>          /* now find stream */
>          for (i = 0; i < s->nb_streams; i++) {
> @@ -919,7 +924,8 @@ skip:
>              || st->discard >= AVDISCARD_ALL
>          ) {
>              avio_seek(s->pb, next, SEEK_SET);
> -            continue;
> +            ret = AVERROR(EAGAIN);
> +            goto leave;
>          }
>          break;
>      }

The last hunk is wrong, or more precisely causes a regression. Actually, all
the uses of EAGAIN in this file look wrong.

If there is a discarded stream, EAGAIN reaches all the way to
av_read_frame() and ffmpeg.c, it assumes that the input stream is not
available and trigger the usleep(10000) delay. You can observe the issue
with the following test:

ffmpeg -lavfi 'testsrc;sine' -t 100 /tmp/test.flv
time ffmpeg -i /tmp/test.flv -f null -
time ffmpeg -i /tmp/test.flv -f null -vn -

The output is:
... -f null -     0.38s user 0.02s sys 99% cpu 0.407 total
... -f null -vn - 0.44s user 0.04s sys 1% cpu 25.205 total

As you can see, with the video stream disabled, ffmpeg spends a very long
time sleeping.

Reverting this patch and e34ba5e (Remove dead loop) fix the timing issue.

Since we do not have a real polling mechanism for demuxers, sleeping is the
only option to handle EAGAIN, for now. Therefore, demuxers should only
return it for external temporary conditions: network or device not ready.

Can you explain your reasons for introducing these EAGAIN?

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151010/fa5c5afd/attachment.sig>