Re: [libav-devel] [PATCH 1/6] lavfi: VAAPI VPP common infrastructure.

2018-02-15 Thread Mark Thompson 
On 15/02/18 09:27, Diego Biurrun wrote:
> On Wed, Feb 14, 2018 at 10:54:26PM +, Mark Thompson wrote:
>> --- /dev/null
>> +++ b/libavfilter/vaapi_vpp.c
>> @@ -0,0 +1,370 @@
>> +
>> +#include 
>> +
>> +#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 
>> +#include 
> 
> I think this needs to go in SKIPHEADERS.

Yes.

Thanks,

- Mark
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/6] lavfi: VAAPI VPP common infrastructure.

2018-02-15 Thread Diego Biurrun 
On Thu, Feb 15, 2018 at 10:27:15AM +0100, Diego Biurrun wrote:
> On Wed, Feb 14, 2018 at 10:54:26PM +, Mark Thompson wrote:
> > --- /dev/null
> > +++ b/libavfilter/vaapi_vpp.c
> > @@ -0,0 +1,370 @@
> > +int ff_vaapi_vpp_colour_standard(enum AVColorSpace av_cs)
> > +{
> > +switch(av_cs) {
> > +#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.

I see this is copy+pasted from other files, oh well ..

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/6] lavfi: VAAPI VPP common infrastructure.

2018-02-15 Thread Diego Biurrun 
On Wed, Feb 14, 2018 at 10:54:26PM +, Mark Thompson wrote:
> --- /dev/null
> +++ b/libavfilter/vaapi_vpp.c
> @@ -0,0 +1,370 @@
> +
> +#include 
> +
> +#include "config.h"

This does not appear to use config.h.

> +#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.

> +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

> +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.

> +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

> --- /dev/null
> +++ b/libavfilter/vaapi_vpp.h
> @@ -0,0 +1,79 @@
> +#ifndef AVFILTER_VAAPI_VPP_H
> +#define AVFILTER_VAAPI_VPP_H
> +
> +#include 
> +#include 

I think this needs to go in SKIPHEADERS.

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH 1/6] lavfi: VAAPI VPP common infrastructure.

2018-02-14 Thread Mark Thompson 
From: Jun Zhao 

Re-work the VAAPI common infrastructure to avoid code duplication
between filters.

From ffmpeg commit dfdeed5a2c8f432d6c5eda1a3a6a1f333f3d4604.

Signed-off-by: Mark Thompson 
---
 libavfilter/vaapi_vpp.c | 370 
 libavfilter/vaapi_vpp.h |  79 +++
 2 files changed, 449 insertions(+)
 create mode 100644 libavfilter/vaapi_vpp.c
 create mode 100644 libavfilter/vaapi_vpp.h

diff --git a/libavfilter/vaapi_vpp.c b/libavfilter/vaapi_vpp.c
new file mode 100644
index 0..5522e1242
--- /dev/null
+++ b/libavfilter/vaapi_vpp.c
@@ -0,0 +1,370 @@
+/*
+ * This file is part of Libav.
+ *
+ * Libav is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * Libav is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with Libav; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include 
+
+#include "config.h"
+
+#include "libavutil/avassert.h"
+#include "libavutil/mem.h"
+#include "libavutil/pixdesc.h"
+#include "formats.h"
+#include "internal.h"
+#include "vaapi_vpp.h"
+
+int ff_vaapi_vpp_query_formats(AVFilterContext *avctx)
+{
+enum AVPixelFormat pix_fmts[] = {
+AV_PIX_FMT_VAAPI, AV_PIX_FMT_NONE,
+};
+
+ff_set_common_formats(avctx, ff_make_format_list(pix_fmts));
+
+return 0;
+}
+
+void ff_vaapi_vpp_pipeline_uninit(AVFilterContext *avctx)
+{
+VAAPIVPPContext *ctx   = avctx->priv;
+int i;
+for (i = 0; i < ctx->nb_filter_buffers; i++) {
+if (ctx->filter_buffers[i] != VA_INVALID_ID) {
+vaDestroyBuffer(ctx->hwctx->display, ctx->filter_buffers[i]);
+ctx->filter_buffers[i] = VA_INVALID_ID;
+}
+}
+ctx->nb_filter_buffers = 0;
+
+if (ctx->va_context != VA_INVALID_ID) {
+vaDestroyContext(ctx->hwctx->display, ctx->va_context);
+ctx->va_context = VA_INVALID_ID;
+}
+
+if (ctx->va_config != VA_INVALID_ID) {
+vaDestroyConfig(ctx->hwctx->display, ctx->va_config);
+ctx->va_config = VA_INVALID_ID;
+}
+
+av_buffer_unref(>device_ref);
+ctx->hwctx = NULL;
+}
+
+int ff_vaapi_vpp_config_input(AVFilterLink *inlink)
+{
+AVFilterContext *avctx = inlink->dst;
+VAAPIVPPContext *ctx   = avctx->priv;
+
+if (ctx->pipeline_uninit)
+ctx->pipeline_uninit(avctx);
+
+if (!inlink->hw_frames_ctx) {
+av_log(avctx, AV_LOG_ERROR, "A hardware frames reference is "
+   "required to associate the processing device.\n");
+return AVERROR(EINVAL);
+}
+
+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");
+return AVERROR(ENOMEM);
+}
+ctx->input_frames = (AVHWFramesContext*)ctx->input_frames_ref->data;
+
+return 0;
+}
+
+int ff_vaapi_vpp_config_output(AVFilterLink *outlink)
+{
+AVFilterContext *avctx = outlink->src;
+VAAPIVPPContext *ctx   = avctx->priv;
+AVVAAPIHWConfig *hwconfig = NULL;
+AVHWFramesConstraints *constraints = NULL;
+AVHWFramesContext *output_frames;
+AVVAAPIFramesContext *va_frames;
+VAStatus vas;
+int err, i;
+
+if (ctx->pipeline_uninit)
+ctx->pipeline_uninit(avctx);
+
+if (!ctx->output_width)
+ctx->output_width  = avctx->inputs[0]->w;
+if (!ctx->output_height)
+ctx->output_height = avctx->inputs[0]->h;
+
+av_assert0(ctx->input_frames);
+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");
+return AVERROR(ENOMEM);
+}
+ctx->hwctx = ((AVHWDeviceContext*)ctx->device_ref->data)->hwctx;
+
+av_assert0(ctx->va_config == VA_INVALID_ID);
+vas = vaCreateConfig(ctx->hwctx->display, VAProfileNone,
+ VAEntrypointVideoProc, NULL, 0, >va_config);
+if (vas != VA_STATUS_SUCCESS) {
+av_log(avctx, AV_LOG_ERROR, "Failed to create processing pipeline "
+   "config: %d (%s).\n", vas, vaErrorStr(vas));
+err = AVERROR(EIO);
+goto fail;
+}
+
+hwconfig = av_hwdevice_hwconfig_alloc(ctx->device_ref);
+if (!hwconfig) {
+err = AVERROR(ENOMEM);
+goto fail;
+}
+hwconfig->config_id = ctx->va_config;
+
+constraints =