[FFmpeg-devel] [PATCH] ffplay's primary function is not to trigger OOM killer

[Michael Niedermayer]

On Fri, Feb 20, 2009 at 01:57:13AM +0100, Aurelien Jacobs wrote:
> Michael Niedermayer wrote:
> 
> > On Fri, Feb 20, 2009 at 12:29:35AM +0100, Aurelien Jacobs wrote:
> > > Michael Niedermayer wrote:
> > > 
> > > > On Thu, Feb 19, 2009 at 11:42:18PM +0100, Aurelien Jacobs wrote:
> > > > > Michael Niedermayer wrote:
> > > > > 
> > > > > > On Thu, Feb 19, 2009 at 10:00:10PM +0100, Aurelien Jacobs wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Commit r16484 introduced an infinite loop allocating memory in ffplay.
> > > > > > > This is very efficient at filling memory and triggering the OOM killer.
> > > > > > > The relevant part of the main loop now looks like this:
> > > > > > > 
> > > > > > >     for(;;) {
> > > > > > >         [...]
> > > > > > >         if(url_feof(ic->pb)) {
> > > > > > >             av_init_packet(pkt);
> > > > > > >             [...]
> > > > > > >             packet_queue_put(&is->videoq, pkt);
> > > > > > >             continue;
> > > > > > >         }
> > > > > > >         ret = av_read_frame(ic, pkt);
> > > > > > >         if (ret < 0) {
> > > > > > >             [...]
> > > > > > >             break;
> > > > > > >         }
> > > > > > >         [...]
> > > > > > >         packet_queue_put(&is->videoq, pkt);
> > > > > > >     }
> > > > > > > 
> > > > > > > So when the demuxer read the last frame of the stream, the ByteIOContext
> > > > > > > reaches EOF but av_read_frame() correctly return a positive value as it
> > > > > > > actually read a full frame.
> > > > > > > Then at next iteration of the main loop url_feof() is triggered, which
> > > > > > > allocate a new packet and go back to the start of the loop, and
> > > > > > > triggers url_feof() again, and so on.
> > > > > > 
> > > > > > try to RTFS again and harder, these kind of bug analysis and fixes is what
> > > > > > leds projects to become what mplayer did ...
> > > > > 
> > > > > try to RTFS a bit more before writing such reply ;-)
> > > > 
> > > > indeed i should
> > > > 
> > > > 
> > > > > 
> > > > > > see, straight before the eof case:
> > > > > > 
> > > > > >         /* if the queue are full, no need to read more */
> > > > > >         if (is->audioq.size > MAX_AUDIOQ_SIZE ||
> > > > > >             is->videoq.size > MAX_VIDEOQ_SIZE ||
> > > > > >             is->subtitleq.size > MAX_SUBTITLEQ_SIZE) {
> > > > > >             /* wait 10 ms */
> > > > > >             SDL_Delay(10);
> > > > > >             continue;
> > > > > >         }
> > > > > > 
> > > > > > thus the NULL packets are only inserted if the que is not full,
> > > > > 
> > > > > Right. But the definition of "queue is not full" may not be what you
> > > > > would expect.
> > > > > 
> > > > > > this effectively limits the size and cannot trigger OOM (in theory ...)
> > > > > 
> > > > > No. This only limits the sum of all pkt.size. But this don't prevent
> > > > > adding infinite amount of 0 sized packets. The total size of actual
> > > > > data is indeed 0, but the size of the structures containing those
> > > > > empty data is enough to fill memory...
> > > > 
> > > > what about adding sizeof(the struct) to the size used in _put and _get?
> > > 
> > > That's simple and clean, and that work as well. So it's probably a bit
> > > better.
> > > 
> > > > should also be conceptually be more correct as it better represents the
> > > > memory actually used by the que
> > > 
> > > Indeed.
> > > 
> > > Patch attached.
> > > 
> > > Aurel
> > > Index: ffplay.c
> > > ===================================================================
> > > --- ffplay.c	(revision 17459)
> > > +++ ffplay.c	(working copy)
> > > @@ -282,7 +282,7 @@
> > >          q->last_pkt->next = pkt1;
> > >      q->last_pkt = pkt1;
> > >      q->nb_packets++;
> > > -    q->size += pkt1->pkt.size;
> > > +    q->size += pkt1->pkt.size + sizeof(*pkt1);
> > >      /* XXX: should duplicate packet data in DV case */
> > >      SDL_CondSignal(q->cond);
> > >  
> > 
> > you are missing packet_queue_get() that needs to be updated as well or
> > the thing will grow until it locks
> 
> Ah yes, good catch. I only tested with a small file, so I didn't hit
> this situation. Updated patch attached.

ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090220/d7987f5e/attachment.pgp>