On 27/02/16 09:11, Anton Khirnov wrote:
> Quoting Mark Thompson (2016-02-26 00:48:11)
>> ---
>> libavfilter/Makefile | 2 +
>> libavfilter/allfilters.c | 2 +
>> libavfilter/avfilter.c | 1 +
>> libavfilter/avfilter.h | 9 ++
>> libavfilter/vf_hwdownload.c | 178 ++++++++++++++++++++++++++++++++++
>> libavfilter/vf_hwupload.c | 227
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 419 insertions(+)
>> create mode 100644 libavfilter/vf_hwdownload.c
>> create mode 100644 libavfilter/vf_hwupload.c
>>
>> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
>> index f5dd2e9..fa6647b 100644
>> --- a/libavfilter/Makefile
>> +++ b/libavfilter/Makefile
>> @@ -56,6 +56,8 @@ OBJS-$(CONFIG_FREI0R_FILTER) += vf_frei0r.o
>> OBJS-$(CONFIG_GRADFUN_FILTER) += vf_gradfun.o
>> OBJS-$(CONFIG_HFLIP_FILTER) += vf_hflip.o
>> OBJS-$(CONFIG_HQDN3D_FILTER) += vf_hqdn3d.o
>> +OBJS-$(CONFIG_HWDOWNLOAD_FILTER) += vf_hwdownload.o
>> +OBJS-$(CONFIG_HWUPLOAD_FILTER) += vf_hwupload.o
>> OBJS-$(CONFIG_HWUPLOAD_CUDA_FILTER) += vf_hwupload_cuda.o
>> OBJS-$(CONFIG_INTERLACE_FILTER) += vf_interlace.o
>> OBJS-$(CONFIG_LUT_FILTER) += vf_lut.o
>> diff --git a/libavfilter/allfilters.c b/libavfilter/allfilters.c
>> index 4bdfaea..9461145 100644
>> --- a/libavfilter/allfilters.c
>> +++ b/libavfilter/allfilters.c
>> @@ -82,6 +82,8 @@ void avfilter_register_all(void)
>> REGISTER_FILTER(GRADFUN, gradfun, vf);
>> REGISTER_FILTER(HFLIP, hflip, vf);
>> REGISTER_FILTER(HQDN3D, hqdn3d, vf);
>> + REGISTER_FILTER(HWDOWNLOAD, hwdownload, vf);
>> + REGISTER_FILTER(HWUPLOAD, hwupload, vf);
>> REGISTER_FILTER(HWUPLOAD_CUDA, hwupload_cuda, vf);
>> REGISTER_FILTER(INTERLACE, interlace, vf);
>> REGISTER_FILTER(LUT, lut, vf);
>> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
>> index 8eefc51..bc7c343 100644
>> --- a/libavfilter/avfilter.c
>> +++ b/libavfilter/avfilter.c
>> @@ -532,6 +532,7 @@ void avfilter_free(AVFilterContext *filter)
>> av_freep(&filter->outputs);
>> av_freep(&filter->priv);
>> av_freep(&filter->internal);
>> + av_freep(&filter->hw_device_ctx);
>
> You mean av_buffer_unref().
Oops; fixed.
>> av_free(filter);
>> }
>>
>> diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
>> index 0a0c415..65918fc 100644
>> --- a/libavfilter/avfilter.h
>> +++ b/libavfilter/avfilter.h
>> @@ -300,6 +300,15 @@ struct AVFilterContext {
>> * An opaque struct for libavfilter internal use.
>> */
>> AVFilterInternal *internal;
>> +
>> + /**
>> + * For filters which will create hardware frames, sets the device the
>> + * filter should create them in. All other filters will ignore this
>> field:
>> + * in particular, a filter which consumes or processes hardware frames
>> will
>> + * instead use the hw_frames_ctx field in AVFilterLink to carry the
>> + * hardware context information.
>> + */
>> + AVBufferRef *hw_device_ctx;
>> };
>>
>> /**
>> diff --git a/libavfilter/vf_hwdownload.c b/libavfilter/vf_hwdownload.c
>> new file mode 100644
>> index 0000000..e9bd453
>> --- /dev/null
>> +++ b/libavfilter/vf_hwdownload.c
>> @@ -0,0 +1,178 @@
>> +/*
>> + * 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 "libavutil/avassert.h"
>> +#include "libavutil/buffer.h"
>> +#include "libavutil/hwcontext.h"
>> +#include "libavutil/log.h"
>> +#include "libavutil/opt.h"
>> +
>> +#include "avfilter.h"
>> +#include "formats.h"
>> +#include "internal.h"
>> +#include "video.h"
>> +
>> +typedef struct HWDownloadContext {
>> + const AVClass *class;
>> +
>> + AVBufferRef *hwframes_ref;
>> + AVHWFramesContext *hwframes;
>> +} HWDownloadContext;
>> +
>> +static int hwdownload_query_formats(AVFilterContext *avctx)
>> +{
>> + if (avctx->inputs[0]) {
>> + ff_formats_ref(ff_all_formats(AVMEDIA_TYPE_VIDEO),
>> + &avctx->inputs[0]->out_formats);
>> + }
>> + if (avctx->outputs[0]) {
>> + ff_formats_ref(ff_all_formats(AVMEDIA_TYPE_VIDEO),
>> + &avctx->outputs[0]->in_formats);
>> + }
>
> The checks are redundant, the inputs and output are guaranteed to be
> there. Also, this makes lavfi think this filter can convert from any
> format to any other format. Granted, we don't have and are not adding
> proper hwaccel format negotiation yet, but perhaps you could at least
> limit the input formats to hwaccel ones.
The checks came from copying vf_scale - I assumed there was some case where it
might query only inputs or outputs. I'll remove them from all the other
filters too.
Should the sets be made by running through all formats with
av_pix_fmt_desc_next() and splitting them into two lists depending on
AV_PIX_FMT_FLAG_HWACCEL, then?
>> +
>> + return 0;
>> +}
>> +
>> +static int hwdownload_config_input(AVFilterLink *inlink)
>> +{
>> + AVFilterContext *avctx = inlink->dst;
>> + HWDownloadContext *ctx = avctx->priv;
>> +
>> + av_buffer_unref(&ctx->hwframes_ref);
>> +
>> + if (!inlink->hw_frames_ctx) {
>> + // We allow success here in order to allow dynamic reconfiguration
>> + // to do the right thing after one step. filter_frame must not
>> + // be called in this state, however.
>> + return 0;
>> + }
>> +
>> + ctx->hwframes_ref = av_buffer_ref(inlink->hw_frames_ctx);
>> + if (!ctx->hwframes_ref)
>> + return AVERROR(ENOMEM);
>> +
>> + ctx->hwframes = (AVHWFramesContext*)ctx->hwframes_ref->data;
>> +
>> + return 0;
>> +}
>
> It's not clear to me why this is needed. There is no dynamic
> reconfiguration in lavfi currently. So unless I'm missing something, you
> can just have config_output which reads the hw_frames_ctx from the input
> link.
>
> You also need to ensure that hw_frames_ctx is unset on the output link.
> For that you will probably need to modify the generic bit that copies it
> in avfilter.c, either by moving it before the config_props() call and
> then unsetting it here, or by only setting it for hwaccel formats.
It was to handle the nasty reinit case you get with a hwaccel decoder which
sets everything up with it's software output format (probably YUV420P), then
only sets up the hwaccel format once actually running.
Maybe it's not directly needed here, but the typically-late setting of
hw_frames_ctx made it seem like a sensible thing to allow.
>> +
>> +static int hwdownload_config_output(AVFilterLink *outlink)
>> +{
>> + AVFilterContext *avctx = outlink->src;
>> + AVFilterLink *inlink = avctx->inputs[0];
>> + HWDownloadContext *ctx = avctx->priv;
>> +
>> + if (ctx->hwframes_ref) {
>> + outlink->format = ctx->hwframes->sw_format;
>
> You're not allowed to set the format here, as it's already set by
> autonegotiation. If you override it to something the destination filter
> doesn't expect, it might explode. So I think the best thing you can do
> here currently is check that the autonegotiated format is supported and
> error out if it's not.
Sure. (av_hwframes_transfer_get_formats(), error if not in the list.)
>> + outlink->w = ctx->hwframes->width;
>> + outlink->h = ctx->hwframes->height;
>
> The hwframe dimensions are the allocated ones, not display. You should
> always use the input link dimensions (which is what the generic code
> does by default, so you can just drop those lines).
Ok.
>> + } else {
>> + outlink->format = inlink->format;
>> + outlink->w = inlink->w;
>> + outlink->h = inlink->h;
>> + }
This bit is trying to allow the config_input/config_output combination to not
fail in the absence of an hw_frames_ctx (as long as you never actually give it
any frames). I'll remove it if the section above goes.
>> +
>> + return 0;
>> +}
>> +
>> +static int hwdownload_filter_frame(AVFilterLink *link, AVFrame *input)
>> +{
>> + AVFilterContext *avctx = link->dst;
>> + HWDownloadContext *ctx = avctx->priv;
>> + AVFrame *output = NULL;
>> + int err;
>> +
>> + if (!ctx->hwframes_ref || !input->hw_frames_ctx) {
>> + av_log(ctx, AV_LOG_ERROR, "Input frames must have hardware
>> context.\n");
>> + return AVERROR(EINVAL);
>
> input is leaked here.
Ack.
>> + }
>> + av_assert0((void*)ctx->hwframes == input->hw_frames_ctx->data);
>
> You shouldn't assert on something outside of your control, return an
> error instead.
Ok.
>> +
>> + output = av_frame_alloc();
>> + if (!output) {
>> + err = AVERROR(ENOMEM);
>> + goto fail;
>> + }
>> +
>> + output->format = ctx->hwframes->sw_format;
>> + output->width = input->width;
>> + output->height = input->height;
>
> You should use ff_get_video_buffer() to allocate the output frame.
Right (I didn't know about that at all, apologies). Fixed.
>
> Comments on hwupload later.
>
Thanks,
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
