[FFmpeg-devel] [PATCH]NVIDIA VDPAU patch for h264

[Uoti Urpala]

On Mon, 2008-12-01 at 11:17 -0800, Stephen Warren wrote:
> 2) The data formats really are different; there are syntax elements or features allowed in H.264 BASELINE profile that are not allowed in MAIN or HIGH profiles (for example, FMO, ASO, RS). A conformant decoder for MAIN is allowed to solely implement the features required for MAIN, and omit those required for BASELINE.
> 
> 3) Due to (2) above, the VDPAU API exposes the ability to specifically create a decoder object that is capable of decoding a specific profile. NVIDIA's implementation of VDPAU only supports MAIN and HIGH for example.

Does the decoder really need to choose an explicit profile? You can't
feed it data (with whatever profile) and see if the decoder can handle
the features used? Or is it less efficient to do so?


> > > +    assert(render != NULL);
> > > +    assert(render->magic == MP_VDPAU_RENDER_MAGIC);
> > > +    if ((render == NULL) || (render->magic != MP_VDPAU_RENDER_MAGIC))
> > > +        return -1; // make sure that this is render packet
> > 
> > assert(!A)
> > if(A)
> > 
> > also this code is duplicated all over the place ...
> 
> Again, we followed the XvMC model. We'll certainly fix up the syntax change you specify.

> > > +extern int ff_VDPAU_h264_picture_complete(H264Context *h);
> > > +
> > 
> > extern is useless should not be used here IMO.
> 
> We modeled this on mpeg12.c's code for declaring the XvMC functions.

There's some old low-quality code left in FFmpeg, and especially in
MPlayer. Unfortunately much of the XvMC code falls in that category. So
it's not the best example of existing practice.

> Anyway, since the function are implemented in a different file, it seems they should be declared extern..

An "extern" declaration like that has no effect in C. You can't have a
function definition without a body, so "extern" is not needed to
distinguish definitions and declarations.


> > > @@ -2142,10 +2155,8 @@
> > >      s->quarter_sample = 1;
> > >      s->low_delay= 1;
> > >
> > > -    if(avctx->codec_id == CODEC_ID_SVQ3)
> > > -        avctx->pix_fmt= PIX_FMT_YUVJ420P;
> > > -    else
> > > -        avctx->pix_fmt= PIX_FMT_YUV420P;
> > > +    // Set in decode_postinit() once initial parsing is complete
> > > +    avctx->pix_fmt = PIX_FMT_NONE;
> > 
> > What is the point of moving the SVQ3 case, I doubt the hardware
> > acceleration supports that?
> > Also without the profile stuff the pix_fmt could still be set here...
> 
> I'm not actually familiar with what SVQ3 is. Any explanation would be helpful.

http://en.wikipedia.org/wiki/SVQ3


> > > +    if(avctx->vdpau_acceleration) {
> > > +        return;
> > > +    } else
> > 
> > Useless {} and else
> 
> Well, the {} is a matter of coding style. Is there a document for ffmpeg that we can follow?

Basically K & R coding style with space indentation only (no tabs). How
strictly each file follows that varies.


> > > +    render->info.h264.field_pic_flag                         = (s-
> > >picture_structure != PICT_FRAME) ? 1 : 0;
> > > +    render->info.h264.bottom_field_flag                      = (s-
> > >picture_structure == PICT_BOTTOM_FIELD) ? 1 : 0;
> > 
> > Uh, like useless code much? You could add ten more "? 1 : 0" and the
> > result still would not change a bit...
> 
> Technically, the compiler is free to generate any non-zero value for comparison results,

It is not. The result is always either 0 or 1.