[Ffmpeg-devel] r7794 broke roqvideo

[Michael Niedermayer]

Hi

On Tue, Feb 20, 2007 at 11:42:28PM +0100, Aurelien Jacobs wrote:
> On Tue, 20 Feb 2007 17:32:33 +0100
> Michel Bardiaux <mbardiaux at mediaxim.be> wrote:
> 
> > Diego Biurrun wrote:
> > > Hi,
> > > 
> > > r7794 broke roqvideo:
> > > 
> > > ------------------------------------------------------------------------
> > > r7794 | takis | 2007-02-01 10:45:05 +0100 (Thu, 01 Feb 2007) | 3 lines
> > > Changed paths:
> > >    M /trunk/libavcodec/utils.c
> > > 
> > > Activate guards in avcodec_default_get_buffer. Patch by Michel Bardiaux,
> > > mbardiaux mediaxim dot be.
> > > ------------------------------------------------------------------------
> > > 
> > > Try playing any sample from
> > > 
> > > http://samples.mplayerhq.hu/game-formats/idroq/
> > > 
> > > and you will get a lot of
> > > 
> > > [roqvideo @ 0x83cb740]pic->data[0]!=NULL in avcodec_default_get_buffer
> > > [roqvideo @ 0x83cb740]  RoQ: get_buffer() failed
> > > 
> > > and no video.
> > > 
> > > Diego
> > 
> > What I did was replace 2 assert(), which were not checked in vanilla 
> > builds, by if(). The 2nd was relevant, since it led to the realization 
> > the release_buffer was missing in some image codecs. I had no way to 
> > discoverr the 1st assert(pic->data[0]==NULL) was wrong in some rare 
> > cases (after all regression test passed). That's a job for someone who 
> > really understands all the subtleties of buffer get/release.
> 
> The attached patch fix this issue.
> But I wonder if the check is really wise at first (in get_buffer):
> 
> if(pic->data[0]!=NULL) {
>     av_log(s, AV_LOG_ERROR, "pic->data[0]!=NULL in avcodec_default_get_buffer\n");
>     return -1;
> }
> 
> I think this test should simply be removed. There is no reason to
> force get_buffer() caller to set pic->data[0] to NULL before calling it.
> We have already vp5/vp6 and roq which got hit by this issue but there
> may be other obscure codecs which are affected and wasn't tested since
> then.
> Would anyone care if I remove this test ?

well yes
release buffer sets it to NULL, if its not NULL in get_buffer that indicates
a problem, in roqvideo.c that is the risky buffer management, copy AVFrame
struct instead of pointers to it, the copy is a perfect recipe for bugs
AVFrame contains many pointers which may or may not be allocated
duplicating the struct will lead to confusion abount what has been allocated
and what has been freed but obviously not set to NULL

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

Democracy is the form of government in which you can choose your dictator
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070221/4d8b74a5/attachment.pgp>