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

Mark Thompson sw at jkqxz.net
Fri Dec 21 01:38:32 EET 2018


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?

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

> 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


More information about the ffmpeg-devel mailing list