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

[Michael Niedermayer]

On Sat, Oct 10, 2015 at 11:37:04AM +0200, Nicolas George wrote:
> 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?

Yes, they simplified the loop and EAGAIN was used before these changes
in flvdec. Also other demuxers use EAGAIN to no other reason then to
simplify their main loop.

They also provide much more fine grained access for
applications (like when there is a long string of skiped packets)
and avoid long locking/waiting for a demuxer & protocol to return.
but that was not the reason for these IIRC

maybe some other error code should be added and or used for these
cases than EAGAIN, but iam also not against replacing the EAGAIN
by a loop (this other error code can be intercepted by the calling
utils.c  code and the loop performed there so user apps dont need
to deal with an additional special error code)

note, simply reverting the commits will break the loop, theres extra
code at the end which checks if packet synchronization has been lost
and resync is needed, this would be skiped by simply calling continue


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151010/d6cdeebd/attachment.sig>