[Ffmpeg-devel] r7794 broke roqvideo

[Aurelien Jacobs]

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.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: roq-fix2.diff
Type: text/x-diff
Size: 5643 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070222/8375afe5/attachment.diff>