[FFmpeg-devel] Implemented non key frame support in NUV decoder

[Laurent Aimar]

Hi,

On Tue, Sep 02, 2008, Reimar D?ffinger wrote:
> On Tue, Sep 02, 2008 at 07:52:10PM +0200, Laurent Aimar wrote:
> > The attached patch provides non key frame support in NUV decoder.
> 
> How to reproduce the difference?
I have uploaded a sample to ftp mplayer incoming.
(ftp://upload.mplayerhq.hu:/MPlayer/incoming/nuv-rtjpeg/nuv_rtjpeg_non_key_frame.nuv)

> > @@ -159,6 +160,7 @@
> >          return -1;
> >      }
> >      comptype = buf[1];
> > +    key_frame = buf[2] == 0;
> 
> Are you sure this is always correctly set?
It cannot assure you that but it seems so.

At worst it will be true when it should be false and you will have the
same behavior than before.

> > @@ -184,18 +186,26 @@
> >          buf_size -= 12;
> >      }
> >  
> > -    if (c->pic.data[0])
> > -        avctx->release_buffer(avctx, &c->pic);
> >      c->pic.reference = 1;
> >      c->pic.buffer_hints = FF_BUFFER_HINTS_VALID | FF_BUFFER_HINTS_READABLE |
> >                            FF_BUFFER_HINTS_PRESERVE | FF_BUFFER_HINTS_REUSABLE;
> > -    if (avctx->get_buffer(avctx, &c->pic) < 0) {
> > -        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > -        return -1;
> > +    if (key_frame) {
> > +        if (c->pic.data[0])
> > +            avctx->release_buffer(avctx, &c->pic);
> > +
> > +        if (avctx->get_buffer(avctx, &c->pic) < 0) {
> > +            av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > +            return -1;
> > +        }
> > +    } else {
> > +        if (avctx->reget_buffer(avctx, &c->pic) < 0) {
> > +            av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> > +            return -1;
> > +        }
> 
> I just misunderstood the API. IMO the best solution would be to remove
> the release_buffer call and replace get_buffer with reget_buffer - that
> was the effect that I think I originally intended.
 It could be done like that but then a useless picture copy may be done
on intra picture (at least with the default reget_buffer).

> 
> >      }
> >  
> > -    c->pic.pict_type = FF_I_TYPE;
> > -    c->pic.key_frame = 1;
> > +    c->pic.pict_type = key_frame ? FF_I_TYPE : FF_P_TYPE;
> > +    c->pic.key_frame = key_frame;
> 
> That might a good idea, but possibly in an extra step.
I do not think it is a good idea to separate it from the previous step, as you would
advertize an inter frame as an intra one (which seems buggy to me).

Regards,

-- 
fenrir