> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Friday, December 21, 2018 7:39 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavfi/vf_hwmap: make hwunmap from
> software frame work.
>
> On 18/12/2018 01:28, Song, Ruiling wrote:
> >> -----Original Message-----
> >> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@ffmpeg.org] On Behalf
> Of
> >> Mark Thompson
> >> Sent: Tuesday, December 18, 2018 6:33 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavfi/vf_hwmap: make hwunmap
> from
> >> software frame work.
> >>
> >> 13/12/2018 01:50, Ruiling Song wrote:
> >>> This patch was used to fix the second hwmap filter issue:
> >>> [vaapi_frame] hwmap [software filters] hwmap [vaapi_frame]
> >>> For such case, we also need to allocate the hardware frame
> >>> and map it back to software.
> >>>
> >>> Signed-off-by: Ruiling Song <ruiling.s...@intel.com>
> >>> ---
> >>> libavfilter/vf_hwmap.c | 125 +++++++++++++++++++++++++++++-------------
> ---
> >> ----
> >>> 1 file changed, 75 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/libavfilter/vf_hwmap.c b/libavfilter/vf_hwmap.c
> >>> index 290559a..03cb325 100644
> >>> --- a/libavfilter/vf_hwmap.c
> >>> +++ b/libavfilter/vf_hwmap.c
> >>> @@ -50,6 +50,36 @@ static int hwmap_query_formats(AVFilterContext
> >> *avctx)
> >>> return 0;
> >>> }
> >>>
> >>> +static int create_hwframe_context(HWMapContext *ctx, AVFilterContext
> >> *avctx,
> >>> + AVBufferRef *device, int format,
> >>> + int sw_format, int width, int height)
> >>> +{
> >>> + int err;
> >>> + AVHWFramesContext *frames;
> >>> +
> >>> + ctx->hwframes_ref = av_hwframe_ctx_alloc(device);
> >>> + if (!ctx->hwframes_ref) {
> >>> + return AVERROR(ENOMEM);
> >>> + }
> >>> + frames = (AVHWFramesContext*)ctx->hwframes_ref->data;
> >>> +
> >>> + frames->format = format;
> >>> + frames->sw_format = sw_format;
> >>> + frames->width = width;
> >>> + frames->height = height;
> >>> +
> >>> + if (avctx->extra_hw_frames >= 0)
> >>> + frames->initial_pool_size = 2 + avctx->extra_hw_frames;
> >>> +
> >>> + err = av_hwframe_ctx_init(ctx->hwframes_ref);
> >>> + if (err < 0) {
> >>> + av_log(avctx, AV_LOG_ERROR, "Failed to initialise "
> >>> + "target frames context: %d.\n", err);
> >>> + return err;
> >>> + }
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int hwmap_config_output(AVFilterLink *outlink)
> >>> {
> >>> AVFilterContext *avctx = outlink->src;
> >>> @@ -130,29 +160,11 @@ static int hwmap_config_output(AVFilterLink
> >> *outlink)
> >>> // overwrite the input hwframe context with a derived context
> >>> // mapped from that back to the source type.
> >>> AVBufferRef *source;
> >>> - AVHWFramesContext *frames;
> >>> -
> >>> - ctx->hwframes_ref = av_hwframe_ctx_alloc(device);
> >>> - if (!ctx->hwframes_ref) {
> >>> - err = AVERROR(ENOMEM);
> >>> + err = create_hwframe_context(ctx, avctx, device,
> >>> outlink->format,
> >>> + hwfc->sw_format, hwfc->width,
> >>> + hwfc->height);
> >>> + if (err < 0)
> >>> goto fail;
> >>> - }
> >>> - frames = (AVHWFramesContext*)ctx->hwframes_ref->data;
> >>> -
> >>> - frames->format = outlink->format;
> >>> - frames->sw_format = hwfc->sw_format;
> >>> - frames->width = hwfc->width;
> >>> - frames->height = hwfc->height;
> >>> -
> >>> - if (avctx->extra_hw_frames >= 0)
> >>> - frames->initial_pool_size = 2 + avctx->extra_hw_frames;
> >>> -
> >>> - err = av_hwframe_ctx_init(ctx->hwframes_ref);
> >>> - if (err < 0) {
> >>> - av_log(avctx, AV_LOG_ERROR, "Failed to initialise "
> >>> - "target frames context: %d.\n", err);
> >>> - goto fail;
> >>> - }
> >>>
> >>> err = av_hwframe_ctx_create_derived(&source,
> >>> inlink->format,
> >>> @@ -175,10 +187,20 @@ static int hwmap_config_output(AVFilterLink
> >> *outlink)
> >>> inlink->hw_frames_ctx = source;
> >>>
> >>> } else if ((outlink->format == hwfc->format &&
> >>> - inlink->format == hwfc->sw_format) ||
> >>> - inlink->format == hwfc->format) {
> >>> - // Map from a hardware format to a software format, or
> >>> - // undo an existing such mapping.
> >>> + inlink->format == hwfc->sw_format)) {
> >>> + // unmap a software frame back to hardware
> >>> + ctx->reverse = 1;
> >>> + // incase user does not provide filter device, use the
> >>> device_ref
> >>> + // from inlink
> >>> + if (!device)
> >>> + device = hwfc->device_ref;
> >>> +
> >>> + err = create_hwframe_context(ctx, avctx, device,
> >>> outlink->format,
> >>> + inlink->format, inlink->w,
> >>> inlink->h);
> >>> + if (err < 0)
> >>> + goto fail;
> >>
> >> I don't think the unmap case here wants to make a new hardware frames
> >> context? You have a software frame which is actually a mapping of a
> hardware
> >> frame and want to retrieve the original hardware frame, so the frames
> context
> >> is that of the original frame.
> >
> > The drawtext filter does directly write to that mapped frame, thank you for
> showing me that.
> > But I think still there are many other filters that do not directly write
> > to the
> input frame.
> > I am creating the frames context for the latter case that ask for a new
> > frame
> from output link. Consider a simple transpose.
> >
> > ./ffmpeg_g -y -loglevel error -init_hw_device vaapi=va:/dev/dri/renderD128 -
> hwaccel vaapi -hwaccel_device va -hwaccel_output_format vaapi -i input.mp4 -
> an -filter_complex 'hwmap,transpose=dir=1,format=nv12,hwmap' -c:v
> h264_vaapi out.mp4
>
> I think the reason this particular case fails is that avfilter setup
> propagates the
> hw_frames_ctx of a link through non-hwframe-aware filters. The second
> hwmap then sees it on input and takes the first branch at line 72 rather than
> the
> second branch at line 200 which you actually want.
>
> I'm not sure what the best way to fix that is. The propagation is desirable
> because it lets you use plumbing filters easily (like format and split), but
> you
> don't want it to happen if a software filter is going to create new frames on
> the
> output. Maybe we could detect or mark those cases and not propagate when
> that happens?
Based on git history seems that the propagation was needed for the unmap to
work.
Could you explain a little bit about " lets you use plumbing filters easily
(like format and split)" ?
I currently don't have any good idea on detecting situation like "creating new
frames on the output", which seems more like dynamic behavior.
>
> >>
> >> This happens when making writeable mappings to make changes to a frame.
> >> Consider, for example:
> >>
> >> ./ffmpeg_g -y -hwaccel vaapi -hwaccel_device /dev/dri/renderD128 -
> >> hwaccel_output_format vaapi -i in.mp4 -an -vf
> >>
> 'scale_vaapi=format=nv12,hwmap=mode=read+write,format=nv12,drawtext=fo
> >> ntfile=font.ttf:text=Hello World!,format=nv12,hwmap' -c:v h264_vaapi
> out.mp4
> >>
> >>> + } else if (inlink->format == hwfc->format) {
> >>> + // Map from a hardware format to a software format
> >>>
> >>> ctx->hwframes_ref = av_buffer_ref(inlink->hw_frames_ctx);
> >>> if (!ctx->hwframes_ref) {
> >>> @@ -212,29 +234,10 @@ static int hwmap_config_output(AVFilterLink
> >> *outlink)
> >>> }
> >>>
> >>> ctx->reverse = 1;
> >>> -
> >>> - ctx->hwframes_ref = av_hwframe_ctx_alloc(device);
> >>> - if (!ctx->hwframes_ref) {
> >>> - err = AVERROR(ENOMEM);
> >>> - goto fail;
> >>> - }
> >>> - hwfc = (AVHWFramesContext*)ctx->hwframes_ref->data;
> >>> -
> >>> - hwfc->format = outlink->format;
> >>> - hwfc->sw_format = inlink->format;
> >>> - hwfc->width = inlink->w;
> >>> - hwfc->height = inlink->h;
> >>> -
> >>> - if (avctx->extra_hw_frames >= 0)
> >>> - hwfc->initial_pool_size = 2 + avctx->extra_hw_frames;
> >>> -
> >>> - err = av_hwframe_ctx_init(ctx->hwframes_ref);
> >>> - if (err < 0) {
> >>> - av_log(avctx, AV_LOG_ERROR, "Failed to create frame "
> >>> - "context for reverse mapping: %d.\n", err);
> >>> + err = create_hwframe_context(ctx, avctx, device, outlink->format,
> >>> + inlink->format, inlink->w,
> >>> inlink->h);
> >>> + if (err < 0)
> >>> goto fail;
> >>> - }
> >>> -
> >>> } else {
> >>> av_log(avctx, AV_LOG_ERROR, "Mapping requires a hardware "
> >>> "context (a device, or frames on input).\n");
> >>
> >> Merging the new context creation in the other two paths looks right to me.
> >>
> >>> @@ -266,8 +269,17 @@ static AVFrame *hwmap_get_buffer(AVFilterLink
> >> *inlink, int w, int h)
> >>> AVFilterContext *avctx = inlink->dst;
> >>> AVFilterLink *outlink = avctx->outputs[0];
> >>> HWMapContext *ctx = avctx->priv;
> >>> + const AVPixFmtDescriptor *desc;
> >>> +
> >>> + desc = av_pix_fmt_desc_get(inlink->format);
> >>> + if (!desc) {
> >>> + av_log(avctx, AV_LOG_ERROR, "Unknown pix format %d\n", inlink-
> >>> format);
> >>> + return NULL;
> >>> + }
> >>>
> >>> - if (ctx->reverse && !inlink->hw_frames_ctx) {
> >>> + // if we are asking for software formats, we currently always
> >>> + // allocate a hardware frame and map it reversely to software format.
> >>> + if (!(desc->flags & AV_PIX_FMT_FLAG_HWACCEL)) {
> >>> AVFrame *src, *dst;
> >>> int err;
> >>>
> >>> @@ -306,12 +318,20 @@ static int hwmap_filter_frame(AVFilterLink *link,
> >> AVFrame *input)
> >>> AVFilterLink *outlink = avctx->outputs[0];
> >>> HWMapContext *ctx = avctx->priv;
> >>> AVFrame *map = NULL;
> >>> + const AVPixFmtDescriptor *desc;
> >>> int err;
> >>>
> >>> av_log(ctx, AV_LOG_DEBUG, "Filter input: %s, %ux%u (%"PRId64").\n",
> >>> av_get_pix_fmt_name(input->format),
> >>> input->width, input->height, input->pts);
> >>>
> >>> + desc = av_pix_fmt_desc_get(input->format);
> >>> + if (!desc) {
> >>> + av_log(avctx, AV_LOG_ERROR, "Unknown pix format %d\n", input-
> >>> format);
> >>> + err = AVERROR(EINVAL);
> >>> + goto fail;
> >>> + }
> >>> +
> >>> map = av_frame_alloc();
> >>> if (!map) {
> >>> err = AVERROR(ENOMEM);
> >>> @@ -319,7 +339,12 @@ static int hwmap_filter_frame(AVFilterLink *link,
> >> AVFrame *input)
> >>> }
> >>>
> >>> map->format = outlink->format;
> >>> - map->hw_frames_ctx = av_buffer_ref(ctx->hwframes_ref);
> >>> + // The software frame may be mapped in another frame context,
> >>> + // so we also do the unmap in that frame context.
> >>> + if (!(desc->flags & AV_PIX_FMT_FLAG_HWACCEL) && input-
> >>> hw_frames_ctx)
> >>> + map->hw_frames_ctx = av_buffer_ref(input->hw_frames_ctx);
> >>> + else
> >>> + map->hw_frames_ctx = av_buffer_ref(ctx->hwframes_ref);
> >>
> >> I'm not sure when this is hit. Maybe it is exactly the case where you
> >> made the
> >> unnecessary frames context above?
> > This is hit when a new frames context was created for this hwmap filter but
> the input frame was mapped in a previous hwmap, consider the passthrough
> mode in transpose filter. Or the drawtext filter you mentioned, now that I am
> creating a new frames context, this ctx->hwframes_ref is not the same as the
> input->hw_frames_ctx.
>
> In that case, I think the new frames context created as ctx->hwframes_ref is
> unused? I would prefer to avoid this case ever happening - outlink-
> >hw_frames_ctx will be wrong if it does, which will confuse following filters.
Yes, the created frames is unused, because it seems hard to detect whether the
created frames_ctx
will be used or not at the configuration stage. so there will be mismatch
between link->frames_ctx and
AVFrame's frames_ctx. I am not sure is there any side-effect or specific
concern about this?
I agree if something will be wrong or not work, we should try to avoid this.
Thanks!
Ruiling
>
> > I am writing the patch so that user could freely use software filters with
> > vappi
> codec pipeline.
> >
> >>> if (!map->hw_frames_ctx) {
> >>> err = AVERROR(ENOMEM);
> >>> goto fail;
> >>>
>
> Thanks,
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
