On 15/02/18 09:27, Diego Biurrun wrote:
> On Wed, Feb 14, 2018 at 10:54:26PM +0000, Mark Thompson wrote:
>> --- /dev/null
>> +++ b/libavfilter/vaapi_vpp.c
>> @@ -0,0 +1,370 @@
>> +
>> +#include <string.h>
>> +
>> +#include "config.h"
>
> This does not appear to use config.h.
...
+ if (HAVE_VAAPI_1 || ctx->hwctx->driver_quirks &
...
>> +#include "libavutil/avassert.h"
>> +#include "libavutil/mem.h"
>> +#include "libavutil/pixdesc.h"
>> +#include "formats.h"
>> +#include "internal.h"
>> +#include "vaapi_vpp.h"
>
> nit: empty line between header groups
>
>> +int ff_vaapi_vpp_config_input(AVFilterLink *inlink)
>> +{
>> + ctx->input_frames_ref = av_buffer_ref(inlink->hw_frames_ctx);
>> + if (!ctx->input_frames_ref) {
>> + av_log(avctx, AV_LOG_ERROR, "A input frames reference create "
>> + "failed.\n");
>
> That sentence sounds ungrammatical to me.
It's ENOMEM, I've just removed the message.
>> + ctx->input_frames = (AVHWFramesContext*)ctx->input_frames_ref->data;
>
> nit: space before *
>
>> + ctx->device_ref = av_buffer_ref(ctx->input_frames->device_ref);
>> + if (!ctx->device_ref) {
>> + av_log(avctx, AV_LOG_ERROR, "A device reference create "
>> + "failed.\n");
>
> see above
Likewise.
>> + ctx->hwctx = ((AVHWDeviceContext*)ctx->device_ref->data)->hwctx;
>
> see above
>
>> +int ff_vaapi_vpp_colour_standard(enum AVColorSpace av_cs)
>> +{
>> + switch(av_cs) {
>
> switch (
>
>> +#define CS(av, va) case AVCOL_SPC_ ## av: return VAProcColorStandard ## va;
>> + CS(BT709, BT709);
>> + CS(BT470BG, BT601);
>> + CS(SMPTE170M, SMPTE170M);
>> + CS(SMPTE240M, SMPTE240M);
>> +#undef CS
>> + default:
>> + return VAProcColorStandardNone;
>> + }
>> +}
>
> I don't think the #define eases readability, on the contrary.
Disagree? In any case, this function isn't really correct because it doesn't
make use of all of the colour information, and there are also many more cases
in libva 2.0. I'll send a fix for that at some point, but for now it's just
factorising out the code which was previously in individual filters.
>> +int ff_vaapi_vpp_render_picture(AVFilterContext *avctx,
>> + VAProcPipelineParameterBuffer *params,
>> + VASurfaceID output_surface)
>> +{
>> + int err = 0;
>> + VAAPIVPPContext *ctx = avctx->priv;
>
> odd extra space
>
>> +void ff_vaapi_vpp_ctx_init(AVFilterContext *avctx)
>> +{
>> + int i;
>> + VAAPIVPPContext *ctx = avctx->priv;
>
> same
>
>> +void ff_vaapi_vpp_ctx_uninit(AVFilterContext *avctx)
>> +{
>> + VAAPIVPPContext *ctx = avctx->priv;
>
> same
Sure, all removed.
>> --- /dev/null
>> +++ b/libavfilter/vaapi_vpp.h
>> @@ -0,0 +1,79 @@
>> +#ifndef AVFILTER_VAAPI_VPP_H
>> +#define AVFILTER_VAAPI_VPP_H
>> +
>> +#include <va/va.h>
>> +#include <va/va_vpp.h>
>
> I think this needs to go in SKIPHEADERS.
Yes.
Thanks,
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
