[Ffmpeg-devel] r7794 broke roqvideo

[Aurelien Jacobs]

On Thu, 22 Feb 2007 00:25:27 +0100
Aurelien Jacobs <aurel at gnuage.org> wrote:

> On Wed, 21 Feb 2007 02:26:49 +0100
> Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> > 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
> 
> In the precise case of roqvideo.c, the buffer management is so simple that
> it can't really be buggy. But in a general way, I understand your point.
> So I now modified roqvideo.c to avoid copying AVFrame. I use pointers to
> AVFrame that I swap instead. This way, there's no need to trick get_buffer().
> The patch is quite big because I replaced every use of current_frame and
> last_frame by a pointer. The only effective change is using FFSWAP instead
> of copying AVFrame.
> I think it's ok, so I will commit soon if no one disagree.

Done.

Aurel