On Fri, Aug 11, 2017 at 09:46:17AM +0800, Huang, Zhengxu wrote:
> From c4e8d8c22f2100bd9bffaccca628c5bd3bfd7281 Mon Sep 17 00:00:00 2001
> From: "Huang, Zhengxu" <zhengxu.maxw...@gmail.com>
> Date: Tue, 25 Jul 2017 21:55:50 +0800
> Subject: [PATCH] libavfilter/overlay_qsv: Add QSV overlay vpp filter
No need for the module prefix as you are adding it and overlay_qsv does
not exist yet, so you cannot be modifying it.
> Add intel QSV overlay filter. Now it supports two input. And it also supports
> the second input scale(implicit) during composition compared to the sw
> overlay.
There is no need to repeat that you added a filter because you just said
that you added a filter in the title and hence repeating that you added a
filter is as redundant as me repeating here that you should not repeat
what you just said because you already said it. ;)
Instead:
The filter supports two inputs and (implicitly) scaling the second input
during composition, unlike the software overlay.
> Code has been separated into common interface part and qsv overlay implement
> part.
The code has been separated into common interface and qsv overlay
implementation.
> The common part mainly creates the qsv session and manages the surface which
> are
Either "surface which is" or "surfaces which are".
> nearly the same for all qsv filters. So the qsvvpp.c/qsvvpp.h API can be used
> by
> other QSV vpp filters to reduce code redundancy.
>
> Usage:
> -hwaccel qsv -c:v mpeg2_qsv -r 25 -i in.m2v -hwaccel qsv -c:v h264_qsv -i
> in.h264 -filter_complex
> "overlay_qsv=eof_action=repeat:x=(W-w)/2:y=(H-h)/2" -b 2M -maxrate 3M -c:v
> h264_qsv -y out.h264
>
> two input should have different size otherwise one will be completely covered
> or you need scale the
> second input as follow:
Two inputs should have different sizes otherwise one will be completely
covered or you need to scale the second input as follows:
> --- /dev/null
> +++ b/libavfilter/qsvvpp.c
> @@ -0,0 +1,737 @@
> + * 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
> + */
> +/**
> + * @file
> + * Intel Quick Sync Video VPP base function
> + */
nit: Add an empty line between the two comment blocks..
> +#include "libavutil/common.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/hwcontext.h"
> +#include "libavutil/hwcontext_qsv.h"
> +#include "libavutil/time.h"
> +#include "libavutil/pixdesc.h"
> +#include "internal.h"
> +#include "qsvvpp.h"
> +#include "video.h"
.. and between the libavutil header block and the other headers.
> +#define IS_VIDEO_MEMORY(mode) (mode &
> (MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET | \
> + MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET))
> +#define IS_OPAQUE_MEMORY(mode) (mode & MFX_MEMTYPE_OPAQUE_FRAME)
> +#define IS_SYSTEM_MEMORY(mode) (mode & MFX_MEMTYPE_SYSTEM_MEMORY)
This would be more readable like this:
#define IS_VIDEO_MEMORY(mode) (mode &
(MFX_MEMTYPE_VIDEO_MEMORY_DECODER_TARGET | \
MFX_MEMTYPE_VIDEO_MEMORY_PROCESSOR_TARGET))
#define IS_OPAQUE_MEMORY(mode) (mode & MFX_MEMTYPE_OPAQUE_FRAME)
#define IS_SYSTEM_MEMORY(mode) (mode & MFX_MEMTYPE_SYSTEM_MEMORY)
> +typedef struct QSVFrame {
> + AVFrame *frame;
> + mfxFrameSurface1 *surface;
> + mfxFrameSurface1 surface_internal; ///< for system memory
Why is this a Doxygen comment? More below ..
> +/* abstract struct for all QSV filters */
> +struct QSVVPPContext {
> + mfxSession session;
> + int (*filter_frame) (AVFilterLink *outlink, AVFrame *frame);///<
> callback function
> + enum AVPixelFormat out_sw_format; ///< Real output format
> + mfxVideoParam vpp_param;
> + mfxFrameInfo *frame_infos; ///< frame info for each input
> +
> + /* members releated to the input/output surface */
rel_ated
> + /* MFXVPP extern parameters*/
space before *
> +static const AVRational default_tb = {1, 90000};
and inside {}
> + if (req->Type & MFX_MEMTYPE_FROM_VPPIN) {
> + resp->mids = av_mallocz(s->nb_surface_ptrs_in * sizeof(*resp->mids));
> + if (!resp->mids)
> + return AVERROR(ENOMEM);
> + resp->NumFrameActual = s->nb_surface_ptrs_in;
> + } else {
> + resp->mids = av_mallocz(s->nb_surface_ptrs_in * sizeof(*resp->mids));
> + if (!resp->mids)
> + return AVERROR(ENOMEM);
Either there is an in/out typo or the code is duplicated and could be
moved before the if.
> +static int pix_fmt_to_mfx_fourcc(int format)
> +{
> + switch(format) {
switch (
> +static void clear_frame_list(QSVFrame **list)
> +{
> + QSVFrame *frame;
> +
> + while (*list) {
> + frame = *list;
> + *list = (*list)->next;
> + av_frame_free(&frame->frame);
> + av_freep(&frame);
> + }
> +}
You could move the variable declaration inside the while block.
> +/* get the input surface */
> +static QSVFrame *submit_frame(QSVVPPContext *s, AVFilterLink *inlink,
> AVFrame *picref)
> +{
> + /**
> + * Turn AVFrame into mfxFrameSurface1.
> + * For video/opaque memory mode, pix_fmt is AV_PIX_FMT_QSV, and
> + * mfxFrameSurface1 is stored in AVFrame->data[3];
> + * for system memory mode, raw video data is stored in
> + * AVFrame, we should map it into mfxFrameSurface1.
> + */
> + if (!IS_SYSTEM_MEMORY(s->in_mem_mode)) {
Again, why is this a Doxygen comment? As a rule of thumb, we only use
Doxygen for actual API documentation that is extracted from the source
files and read independently of the source files. This comment elaborates
on some implementation detail and will not make any sense outside of this
file.
> + qsv_frame->frame = picref;
> + qsv_frame->surface = (mfxFrameSurface1*)qsv_frame->frame->data[3];
nit: align = and add a space before *
> + qsv_frame->surface->Info = s->frame_infos[FF_INLINK_IDX(inlink)];
> + qsv_frame->surface->Data.TimeStamp = av_rescale_q(qsv_frame->frame->pts,
> + inlink->time_base,
> default_tb);
same
> + out_frame = get_free_frame(&s->out_frame_list);
> + if (!out_frame) {
> + av_log(ctx, AV_LOG_ERROR, "Can't alloc new output frame.\n");
> + return NULL;
> + }
This av_log output is getting repeated, maybe it should be done inside
get_free_frame() to avoid the duplication.
> + /**
> + * For video memory, get a hw frame;
> + * for system memory, get a sw frame and map it into a mfx_surface.
> + */
see above; also, maybe you could write this compactly in 2 lines instead of 4.
> + ret = av_hwframe_get_buffer(outlink->hw_frames_ctx,
> out_frame->frame, 0);
> + if (ret < 0) {
> + av_log(ctx, AV_LOG_ERROR, "Can't allocate a surface.\n");
> + return NULL;
> + }
same?
> + out_frame->surface = (mfxFrameSurface1*)out_frame->frame->data[3];
space before *
> + } else {
> + /**
> + * Get a frame with aligned dimensions.
> + * Libmfx need system memory being 128x64 aligned
> + */
see above
> + /**
> + * @mem-mode
> + * 1.Input has a HW context, Video or Opaque mode
> + * 2.Otherwise, System mode.
> + */
@mem-mode is .. what?
> + out_frames_ctx = (AVHWFramesContext*)out_frames_ref->data;
space before *
> + s->surface_ptrs_out = av_mallocz_array(out_frames_hwctx->nb_surfaces,
> + sizeof(*s->surface_ptrs_out));
> + if (!s->surface_ptrs_out) {
> + av_buffer_unref(&out_frames_ref);
> + return AVERROR(ENOMEM);
> + }
> +
> + for (i = 0; i < out_frames_hwctx->nb_surfaces; i++)
> + s->surface_ptrs_out[i] = out_frames_hwctx->surfaces + i;
> + s->nb_surface_ptrs_out = out_frames_hwctx->nb_surfaces;
> +
> + av_buffer_unref(&outlink->hw_frames_ctx);
> + outlink->hw_frames_ctx = out_frames_ref;
> + } else
> + s->out_mem_mode = MFX_MEMTYPE_SYSTEM_MEMORY;
> +
> + /* extract the properties of the "master" session given to us */
> + ret = MFXQueryIMPL(device_hwctx->session, &impl);
> + if (ret == MFX_ERR_NONE)
> + ret = MFXQueryVersion(device_hwctx->session, &ver);
> + if (ret != MFX_ERR_NONE) {
> + av_log(avctx, AV_LOG_ERROR, "Error querying the session
> attributes\n");
> + return AVERROR_UNKNOWN;
> + }
Using plain "else" instead of another if would be clearer.
> + /* create a "slave" session with those same properties, to be used for
> vpp*/
space before *
> +int ff_qsvvpp_create(AVFilterContext *avctx, QSVVPPContext **vpp,
> QSVVPPParam *param)
> +{
> + int ret = 0, i;
The init is pointless.
> + QSVVPPContext *s;
> +
> + s = av_mallocz(sizeof(*s));
> + if (!s) {
> + ret = AVERROR(ENOMEM);
> + goto failed;
> + }
No need for the goto here, you can just return directly.
> + /* Init each input's information*/
space before *
> +failed:
> + if (s)
> + ff_qsvvpp_free(&s);
> +
> + return ret;
> +}
The if is unnecessary if you dropped the goto above.
> +int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame
> *picref)
> +{
> + in_frame = submit_frame(s, inlink, picref);
> + if (!in_frame) {
> + av_log(ctx, AV_LOG_ERROR, "Fail to submit frame on input[%d]\n",
> + FF_INLINK_IDX(inlink));
"Failed", indentation is off.
> + return AVERROR(EINVAL);
Is this an error due to user-supplied data or user-supplied configuration?
Because that's what EINVAL is for.
> + do {
> + out_frame = query_frame(s, outlink);
> + if (!out_frame) {
> + av_log(ctx, AV_LOG_ERROR, "Fail to query an output frame.\n");
Failed
> + if (ret < 0 && ret != MFX_ERR_MORE_SURFACE) {
> + /*Ignore more_data error*/
space after * and before *
> --- /dev/null
> +++ b/libavfilter/qsvvpp.h
> @@ -0,0 +1,64 @@
> + * 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
> + */
> +/**
> + * @file
> + * Intel Quick Sync Video VPP base function
> + */
nit: Add an empty line between the two comment blocks..
> +#ifndef AVFILTER_QSVVPP_H
> +#define AVFILTER_QSVVPP_H
> +
> +#include "avfilter.h"
> +#include <mfx/mfxvideo.h>
Move system headers before local headers (add an empty line between) to
avoid unwanted sideeffects.
> +typedef struct QSVVPPParam {
> + /* default is ff_filter_frame */
> + int (*filter_frame) (AVFilterLink *outlink, AVFrame *frame);
Drop the space between ) and (.
> +/* create and initialize the QSV session*/
space before *
> +/* release the resources eg.surfaces */
release the resources (e.g. surfaces)
> +#endif /* QSVVPP_H */
The comment does not match the name of the multiple inclusion guard above.
> --- /dev/null
> +++ b/libavfilter/vf_overlay_qsv.c
> @@ -0,0 +1,493 @@
> +
> +/**
> + * @vf_overlay_qsv
What is @vf_overlay_qsv?
> +#include "internal.h"
> +#include "avfilter.h"
> +#include "formats.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/common.h"
> +#include "libavutil/pixdesc.h"
> +#include "libavutil/eval.h"
> +#include "libavutil/hwcontext.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/imgutils.h"
> +#include "libavutil/mathematics.h"
> +#include "video.h"
> +
> +#include "qsvvpp.h"
Move the libavutil headers before the other headers and add an empty line.
> +typedef struct QSVOverlayContext {
> + const AVClass *class;
This struct member is unused
> +/**
> + * qsv composite support overlay implicitly scale so the params much
> + * more complicated than sw overlay
> + */
This sentence no verb.
> +static const char *const var_names[] = {
> + "main_w", "W", ///< input width of the main layer
> + "main_h", "H", ///< input height of the main layer
> + "overlay_iw", ///< input width of the overlay layer
> + "overlay_ih", ///< input height of the overlay layer
> + "overlay_x", "x", ///< x position of the overlay layer inside of main
> + "overlay_y", "y", ///< y position of the overlay layer inside of main
> + "overlay_w", "w", ///< output width of overlay layer
> + "overlay_h", "h", ///< output height of overlay layer
> + NULL
> +};
Please review all your Doxygen comments and decide whether or not they
should be normal C comments instead.
> +static const AVOption options[] = {
> + { "x", "Overlay x position", OFFSET(overlay_ox), AV_OPT_TYPE_STRING,
> {.str="0"}, 0, 255, .flags = FLAGS},
> + { "y", "Overlay y position", OFFSET(overlay_oy), AV_OPT_TYPE_STRING,
> {.str="0"}, 0, 255, .flags = FLAGS},
> + { "w", "Overlay width", OFFSET(overlay_ow), AV_OPT_TYPE_STRING,
> {.str="overlay_iw"}, 0, 255, .flags = FLAGS},
> + { "h", "Overlay height", OFFSET(overlay_oh), AV_OPT_TYPE_STRING,
> {.str="overlay_ih*w/overlay_iw"}, 0, 255, .flags = FLAGS},
> + { "alpha", "Overlay global alpha", OFFSET(overlay_alpha),
> AV_OPT_TYPE_INT, {.i64 = 255}, 0, 255, .flags = FLAGS},
spaces inside {}
> +static int config_output(AVFilterLink *outlink)
> +{
> + av_log(ctx, AV_LOG_DEBUG, "Output is of %s.\n",
> av_get_pix_fmt_name(outlink->format));
> + if ((in0->format == AV_PIX_FMT_QSV && in1->format != AV_PIX_FMT_QSV) ||
> + (in0->format != AV_PIX_FMT_QSV && in1->format == AV_PIX_FMT_QSV)) {
> + av_log(ctx, AV_LOG_ERROR, "One of the inputs is of AV_PIX_FMT_QSV,"
> + "but the other is of soft pixel format.\n");
> + av_log(ctx, AV_LOG_ERROR, "HW/SW mixed format is not supported
> now.\n");
I would just shorten this to one line:
Mixing hardware and software pixel formats is not supported.
> + } else if (in0->format == AV_PIX_FMT_QSV) {
> + AVHWFramesContext *hw_frame0 =
> (AVHWFramesContext*)in0->hw_frames_ctx->data;
> + AVHWFramesContext *hw_frame1 =
> (AVHWFramesContext*)in1->hw_frames_ctx->data;
space before *
> + if (hw_frame0->device_ctx != hw_frame1->device_ctx) {
> + av_log(ctx, AV_LOG_ERROR, "Inputs underlying different QSV
> devices is forbidden.\n");
Inputs with different underlying QSV devices are forbidden.
> +static int blend_frame(AVFilterContext *ctx, AVFrame *mpic, AVFrame *opic)
> +{
> + /**
> + * Reference the overlay frame. Because:
> + * 1. ff_qsvvpp_filter_frame will take control of the given frame
> + * 2. We need to repeat the overlay frame when 2nd input goes into
> EOF
> + */
> + opic_copy = av_frame_clone(opic);
see above about Doxygen
> +static int request_frame(AVFilterLink *outlink)
> +{
> + if (s->main->pts == AV_NOPTS_VALUE ||
> + s->over_next->pts == AV_NOPTS_VALUE ||
> + !av_compare_ts(s->over_next->pts, tb_over, s->main->pts, tb_main)) {
> + ret = blend_frame(ctx, s->main, s->over_next);
> + av_frame_free(&s->over_prev);
> + FFSWAP(AVFrame*, s->over_prev, s->over_next);
> + } else if (s->over_prev) {
> + ret = blend_frame(ctx, s->main, s->over_prev);
> + } else {
> + av_frame_free(&s->main);
> + ret = AVERROR(EAGAIN);
> + }
> +
> + s->main = NULL;
> +
> + return ret ;
stray space before ;
> +static int overlay_qsv_init(AVFilterContext *ctx)
> +{
> + vpp->comp_conf.InputStream =
> + av_mallocz_array(sizeof(*vpp->comp_conf.InputStream),
> ctx->nb_inputs);
Indentation is off.
> + /* initialize QSVVPP params */
> + vpp->qsv_param.filter_frame = NULL;
> + vpp->qsv_param.ext_buf =
> av_mallocz(sizeof(*vpp->qsv_param.ext_buf));
> + if (!vpp->qsv_param.ext_buf)
> + return AVERROR(ENOMEM);
Don't you leak vpp->comp_conf.InputStream here?
> +AVFilter ff_vf_overlay_qsv = {
> + .name = "overlay_qsv",
> + .description = NULL_IF_CONFIG_SMALL("Quick Sync Video overlay."),
> + .priv_size = sizeof(QSVOverlayContext),
> + .query_formats = overlay_qsv_query_formats,
> + .init = overlay_qsv_init,
> + .uninit = overlay_qsv_uninit,
> + .inputs = overlay_qsv_inputs,
> + .outputs = overlay_qsv_outputs,
> + .priv_class = &overlay_qsv_class,
> + .flags_internal = FF_FILTER_FLAG_HWFRAME_AWARE,
> +};
Align the =.
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
