diff mbox

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

Message ID 1544665804-13895-1-git-send-email-ruiling.song@intel.com
State New
Headers show

Commit Message

Ruiling Song Dec. 13, 2018, 1:50 a.m. UTC
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@intel.com>
---
 libavfilter/vf_hwmap.c | 125 +++++++++++++++++++++++++++++--------------------
 1 file changed, 75 insertions(+), 50 deletions(-)

Comments

Mark Thompson Dec. 17, 2018, 10:33 p.m. UTC | #1
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@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.

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=fontfile=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?

>      if (!map->hw_frames_ctx) {
>          err = AVERROR(ENOMEM);
>          goto fail;
> 

Thanks,

- Mark
Ruiling Song Dec. 18, 2018, 1:28 a.m. UTC | #2
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.song@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

> 

> 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.
I am writing the patch so that user could freely use software filters with vappi codec pipeline.

Thanks!
Ruiling
> 

> >      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
Mark Thompson Dec. 20, 2018, 11:38 p.m. UTC | #3
On 18/12/2018 01:28, Song, Ruiling wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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.song@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
Ruiling Song Dec. 25, 2018, 11:35 a.m. UTC | #4
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@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-bounces@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.song@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
diff mbox

Patch

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;
+        } 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");
@@ -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);
     if (!map->hw_frames_ctx) {
         err = AVERROR(ENOMEM);
         goto fail;