[FFmpeg-devel] [PATCH] Added QSV based VPP filter - second try

[Sven Dueking]

> -----Ursprüngliche Nachricht-----
> Von: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] Im Auftrag
> von Carl Eugen Hoyos
> Gesendet: Donnerstag, 29. Oktober 2015 18:39
> An: ffmpeg-devel at ffmpeg.org
> Betreff: Re: [FFmpeg-devel] [PATCH] Added QSV based VPP filter - second
> try
> 
> Sven Dueking <sven <at> nablet.com> writes:
> 
> > Please find attached my second attempt to add Intel´s VPP to FFMpeg.
> > As requested, the patch includes a documentation file
> 
> I don't know much about how the documentation works but why isn't this
> part of the filter documentation and how are users supposed to find it?

Hi Carl,

I was unsure how to add the documentation, that´s why I asked this before.
Will merge my changes into the filter documentation.

> 
> > +        AV_PIX_FMT_RGB32,
> 
> The Intel documentation for RGB4 reads:
> "ARGB in that order, A channel is 8 MSBs"
> Making this AV_PIX_FMT_ARGB imo
> But I'm probably wrong again...

https://software.intel.com/sites/default/files/mediasdk-man.pdf

RGB4
Thirty-two-bit RGB color format. Also known as RGB32

MFX_FOURCC_RGB4 : RGB4 (RGB32) color planes

This matches the define in the latest SDK

    MFX_FOURCC_RGB4         = MFX_MAKEFOURCC('R','G','B','4'),    /* RGB32 */

As far as I know older versions of mfxstructures.h have such comment as you mentioned.

Anyway, according to the sample

   case MFX_FOURCC_RGB4:
        ptr->G = ptr->B + 1;
        ptr->R = ptr->B + 2;
        ptr->A = ptr->B + 3;

it´s BGRA (and the output is fine using BRGA and "bad" using ARGB).

> 
> > +    if (inlink->format == AV_PIX_FMT_YUV420P)
> > +        vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_YV12;
> > +    else if (inlink->format == AV_PIX_FMT_YUV422P)
> > +        vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_YUY2;
> > +    else if (inlink->format == AV_PIX_FMT_NV12)
> > +        vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_NV12;
> > +    else
> > +        vpp->pVppParam->vpp.In.FourCC = MFX_FOURCC_RGB4;
> 
> Did you consider to add BGR4 and ARGB16 in the future?

No, VPP doesn´t support ARGB16.

> 
> > +    unsigned int bits_per_pixel =
> > + get_bpp(vpp->pVppParam->vpp.In.FourCC);
> 
> See below.
> 
> > +    width = (unsigned int) FFALIGN(vpp->pVppParam->vpp.In.Width,
> 32);
> > +    height = (unsigned int) FFALIGN(vpp->pVppParam->vpp.In.Height,
> > + 32);
> 
> Are the casts necessary or useful?

Nope ;)

> 
> > +    vpp->in_surface = av_mallocz(sizeof(mfxFrameSurface1) *
> > vpp->num_surfaces_in);
> 
> This looks wrong to me, I believe you want to allocate
> num_surfaces_in pointers, not structs. (Sorry if I miss
> somthing.)

You are correct, thanks !

> 
> > +        return -1;
> 
> ENOMEM.
> 
> > +    for (i = 0; i < vpp->num_surfaces_in; i++)
> > +        vpp->in_surface[i] = NULL;
> 
> This is unnecessary if you use mallocz() as you do.
> 
> > +    for (i = 0; i < vpp->num_surfaces_in; i++) {
> > +        vpp->in_surface[i] = av_mallocz(sizeof(mfxFrameSurface1));
> > +
> > +        if (!vpp->in_surface[i])
> > +            return -1;
> 
> ENOMEM but this leaks memory, you have to free
> everything else on failure.
> (same below for out_surface)
> 
> > +    bits_per_pixel = 12;
> 
> Imo, remove the variable and use get_bpp() once
> and "12" on the second usage.
> 

Also ok.

Many thanks for your feedback !

> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel