[FFmpeg-devel] [PATCH 1/2] lavfi/vf_hwmap: make hwunmap from software frame work.

Song, Ruiling ruiling.song at intel.com
Tue Dec 25 13:35:51 EET 2018



> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of
> Mark Thompson
> Sent: Friday, December 21, 2018 7:39 AM
> To: ffmpeg-devel at 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-bounces at ffmpeg.org] On Behalf
> Of
> >> Mark Thompson
> >> Sent: Tuesday, December 18, 2018 6:33 AM
> >> To: ffmpeg-devel at 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.song at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list