[FFmpeg-devel] ffmpeg: Don't require a known device to pass a frames context to an encoder

The previous version here did not handle passing a frames context when
ffmpeg itself did not know about the device it came from (for example,
because it was created by device derivation inside a filter graph), which
would break encoders requiring that input.  Fix that by checking for HW
frames and device context methods independently, and prefer to use a
frames context method if possible.  At the same time, revert the encoding
additions to the device matching function because the additional
complexity was not relevant to decoding.
---
 fftools/ffmpeg_hw.c | 75 +++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Mark Thompson
> Sent: Wednesday, April 29, 2020 06:57
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: [FFmpeg-devel] [PATCH] ffmpeg: Don't require a known device to
> pass a frames context to an encoder
> 
> The previous version here did not handle passing a frames context when
> ffmpeg itself did not know about the device it came from (for example,
> because it was created by device derivation inside a filter graph), which
> would break encoders requiring that input.  Fix that by checking for HW
> frames and device context methods independently, and prefer to use a
> frames context method if possible.  At the same time, revert the encoding
> additions to the device matching function because the additional
> complexity was not relevant to decoding.
> ---
>  fftools/ffmpeg_hw.c | 75 +++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 33 deletions(-)
> diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c
> index c5c8aa97ef..fc4a5d31d6 100644
> --- a/fftools/ffmpeg_hw.c
> +++ b/fftools/ffmpeg_hw.c
> @@ -19,6 +19,7 @@
>  #include <string.h>
> 
>  #include "libavutil/avstring.h"
> +#include "libavutil/pixdesc.h"
>  #include "libavfilter/buffersink.h"
> 
>  #include "ffmpeg.h"
> @@ -282,10 +283,7 @@ void hw_device_free_all(void)
>      nb_hw_devices = 0;
>  }
> 
> -static HWDevice *hw_device_match_by_codec(const AVCodec *codec,
> -                                          enum AVPixelFormat format,
> -                                          int possible_methods,
> -                                          int *matched_methods)
> +static HWDevice *hw_device_match_by_codec(const AVCodec *codec)
>  {
>      const AVCodecHWConfig *config;
>      HWDevice *dev;
> @@ -294,18 +292,11 @@ static HWDevice
> *hw_device_match_by_codec(const AVCodec *codec,
>          config = avcodec_get_hw_config(codec, i);
>          if (!config)
>              return NULL;
> -        if (format != AV_PIX_FMT_NONE &&
> -            config->pix_fmt != AV_PIX_FMT_NONE &&
> -            config->pix_fmt != format)
> -            continue;
> -        if (!(config->methods & possible_methods))
> +        if (!(config->methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX))
>              continue;
>          dev = hw_device_get_by_type(config->device_type);
> -        if (dev) {
> -            if (matched_methods)
> -                *matched_methods = config->methods & possible_methods;
> +        if (dev)
>              return dev;
> -        }
>      }
>  }
> 
> @@ -351,9 +342,7 @@ int hw_device_setup_for_decode(InputStream *ist)
>              if (!dev)
>                  err = hw_device_init_from_type(type, NULL, &dev);
>          } else {
> -            dev = hw_device_match_by_codec(ist->dec, AV_PIX_FMT_NONE,
> -                                           AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX,
> -                                           NULL);
> +            dev = hw_device_match_by_codec(ist->dec);
>              if (!dev) {
>                  // No device for this codec, but not using generic hwaccel
>                  // and therefore may well not need one - ignore.
> @@ -429,37 +418,57 @@ int hw_device_setup_for_decode(InputStream
> *ist)
> 
>  int hw_device_setup_for_encode(OutputStream *ost)
>  {
> -    HWDevice *dev;
> -    AVBufferRef *frames_ref;
> -    int methods = AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX;
> -    int matched_methods;
> +    const AVCodecHWConfig *config;
> +    HWDevice *dev = NULL;
> +    AVBufferRef *frames_ref = NULL;
> +    int i;
> 
>      if (ost->filter) {
>          frames_ref = av_buffersink_get_hw_frames_ctx(ost->filter->filter);
>          if (frames_ref &&
>              ((AVHWFramesContext*)frames_ref->data)->format ==
> -            ost->enc_ctx->pix_fmt)
> -            methods |= AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX;
> +            ost->enc_ctx->pix_fmt) {
> +            // Matching format, will try to use hw_frames_ctx.
> +        } else {
> +            frames_ref = NULL;
> +        }
>      }
> 
> -    dev = hw_device_match_by_codec(ost->enc, ost->enc_ctx->pix_fmt,
> -                                   methods, &matched_methods);
> -    if (dev) {
> -        if (matched_methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX) {
> -            ost->enc_ctx->hw_device_ctx = av_buffer_ref(dev->device_ref);
> -            if (!ost->enc_ctx->hw_device_ctx)
> -                return AVERROR(ENOMEM);
> -        }
> -        if (matched_methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX) {
> +    for (i = 0;; i++) {
> +        config = avcodec_get_hw_config(ost->enc, i);
> +        if (!config)
> +            break;
> +
> +        if (frames_ref &&
> +            config->methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX &&
> +            (config->pix_fmt == AV_PIX_FMT_NONE ||
> +             config->pix_fmt == ost->enc_ctx->pix_fmt)) {
> +            av_log(ost->enc_ctx, AV_LOG_VERBOSE, "Using input "
> +                   "frames context (format %s) with %s encoder.\n",
> +                   av_get_pix_fmt_name(ost->enc_ctx->pix_fmt),
> +                   ost->enc->name);
>              ost->enc_ctx->hw_frames_ctx = av_buffer_ref(frames_ref);
>              if (!ost->enc_ctx->hw_frames_ctx)
>                  return AVERROR(ENOMEM);
> +            return 0;
>          }
> -        return 0;
> +
> +        if (!dev &&
> +            config->methods &
> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX)
> +            dev = hw_device_get_by_type(config->device_type);
> +    }
> +
> +    if (dev) {
> +        av_log(ost->enc_ctx, AV_LOG_VERBOSE, "Using device %s "
> +               "(type %s) with %s encoder.\n", dev->name,
> +               av_hwdevice_get_type_name(dev->type), ost->enc->name);
> +        ost->enc_ctx->hw_device_ctx = av_buffer_ref(dev->device_ref);
> +        if (!ost->enc_ctx->hw_device_ctx)
> +            return AVERROR(ENOMEM);
>      } else {
>          // No device required, or no device available.

I've got a question on these comments for long time which is not related to the patch
itself, isn't it a good idea to treat them as logs with a verbosity level of warning or DEBUG,
then user/developer may catch/seek this easier if something unexpected happens?

Another instance is:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190815043242.20478-1-linjie.fu@intel.com/

> -        return 0;
>      }
> +    return 0;
>  }
> 
>  static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)
> --

Works for me for #8637, and also tested in device context methods with
-init_hw_device qsv=hw:/dev/dri/renderD128 -filter_hw_device hw

Hence would it be better to mention the ticket number as well?

- Linjie

On 29/04/2020 06:34, Fu, Linjie wrote:
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Mark Thompson
>> Sent: Wednesday, April 29, 2020 06:57
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: [FFmpeg-devel] [PATCH] ffmpeg: Don't require a known device to
>> pass a frames context to an encoder
>>
>> The previous version here did not handle passing a frames context when
>> ffmpeg itself did not know about the device it came from (for example,
>> because it was created by device derivation inside a filter graph), which
>> would break encoders requiring that input.  Fix that by checking for HW
>> frames and device context methods independently, and prefer to use a
>> frames context method if possible.  At the same time, revert the encoding
>> additions to the device matching function because the additional
>> complexity was not relevant to decoding.
>> ---
>>  fftools/ffmpeg_hw.c | 75 +++++++++++++++++++++++++--------------------
>>  1 file changed, 42 insertions(+), 33 deletions(-)
>> diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c
>> index c5c8aa97ef..fc4a5d31d6 100644
>> --- a/fftools/ffmpeg_hw.c
>> +++ b/fftools/ffmpeg_hw.c
>> @@ -19,6 +19,7 @@
>>  #include <string.h>
>>
>>  #include "libavutil/avstring.h"
>> +#include "libavutil/pixdesc.h"
>>  #include "libavfilter/buffersink.h"
>>
>>  #include "ffmpeg.h"
>> @@ -282,10 +283,7 @@ void hw_device_free_all(void)
>>      nb_hw_devices = 0;
>>  }
>>
>> -static HWDevice *hw_device_match_by_codec(const AVCodec *codec,
>> -                                          enum AVPixelFormat format,
>> -                                          int possible_methods,
>> -                                          int *matched_methods)
>> +static HWDevice *hw_device_match_by_codec(const AVCodec *codec)
>>  {
>>      const AVCodecHWConfig *config;
>>      HWDevice *dev;
>> @@ -294,18 +292,11 @@ static HWDevice
>> *hw_device_match_by_codec(const AVCodec *codec,
>>          config = avcodec_get_hw_config(codec, i);
>>          if (!config)
>>              return NULL;
>> -        if (format != AV_PIX_FMT_NONE &&
>> -            config->pix_fmt != AV_PIX_FMT_NONE &&
>> -            config->pix_fmt != format)
>> -            continue;
>> -        if (!(config->methods & possible_methods))
>> +        if (!(config->methods &
>> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX))
>>              continue;
>>          dev = hw_device_get_by_type(config->device_type);
>> -        if (dev) {
>> -            if (matched_methods)
>> -                *matched_methods = config->methods & possible_methods;
>> +        if (dev)
>>              return dev;
>> -        }
>>      }
>>  }
>>
>> @@ -351,9 +342,7 @@ int hw_device_setup_for_decode(InputStream *ist)
>>              if (!dev)
>>                  err = hw_device_init_from_type(type, NULL, &dev);
>>          } else {
>> -            dev = hw_device_match_by_codec(ist->dec, AV_PIX_FMT_NONE,
>> -                                           AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX,
>> -                                           NULL);
>> +            dev = hw_device_match_by_codec(ist->dec);
>>              if (!dev) {
>>                  // No device for this codec, but not using generic hwaccel
>>                  // and therefore may well not need one - ignore.
>> @@ -429,37 +418,57 @@ int hw_device_setup_for_decode(InputStream
>> *ist)
>>
>>  int hw_device_setup_for_encode(OutputStream *ost)
>>  {
>> -    HWDevice *dev;
>> -    AVBufferRef *frames_ref;
>> -    int methods = AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX;
>> -    int matched_methods;
>> +    const AVCodecHWConfig *config;
>> +    HWDevice *dev = NULL;
>> +    AVBufferRef *frames_ref = NULL;
>> +    int i;
>>
>>      if (ost->filter) {
>>          frames_ref = av_buffersink_get_hw_frames_ctx(ost->filter->filter);
>>          if (frames_ref &&
>>              ((AVHWFramesContext*)frames_ref->data)->format ==
>> -            ost->enc_ctx->pix_fmt)
>> -            methods |= AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX;
>> +            ost->enc_ctx->pix_fmt) {
>> +            // Matching format, will try to use hw_frames_ctx.
>> +        } else {
>> +            frames_ref = NULL;
>> +        }
>>      }
>>
>> -    dev = hw_device_match_by_codec(ost->enc, ost->enc_ctx->pix_fmt,
>> -                                   methods, &matched_methods);
>> -    if (dev) {
>> -        if (matched_methods &
>> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX) {
>> -            ost->enc_ctx->hw_device_ctx = av_buffer_ref(dev->device_ref);
>> -            if (!ost->enc_ctx->hw_device_ctx)
>> -                return AVERROR(ENOMEM);
>> -        }
>> -        if (matched_methods &
>> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX) {
>> +    for (i = 0;; i++) {
>> +        config = avcodec_get_hw_config(ost->enc, i);
>> +        if (!config)
>> +            break;
>> +
>> +        if (frames_ref &&
>> +            config->methods &
>> AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX &&
>> +            (config->pix_fmt == AV_PIX_FMT_NONE ||
>> +             config->pix_fmt == ost->enc_ctx->pix_fmt)) {
>> +            av_log(ost->enc_ctx, AV_LOG_VERBOSE, "Using input "
>> +                   "frames context (format %s) with %s encoder.\n",
>> +                   av_get_pix_fmt_name(ost->enc_ctx->pix_fmt),
>> +                   ost->enc->name);
>>              ost->enc_ctx->hw_frames_ctx = av_buffer_ref(frames_ref);
>>              if (!ost->enc_ctx->hw_frames_ctx)
>>                  return AVERROR(ENOMEM);
>> +            return 0;
>>          }
>> -        return 0;
>> +
>> +        if (!dev &&
>> +            config->methods &
>> AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX)
>> +            dev = hw_device_get_by_type(config->device_type);
>> +    }
>> +
>> +    if (dev) {
>> +        av_log(ost->enc_ctx, AV_LOG_VERBOSE, "Using device %s "
>> +               "(type %s) with %s encoder.\n", dev->name,
>> +               av_hwdevice_get_type_name(dev->type), ost->enc->name);
>> +        ost->enc_ctx->hw_device_ctx = av_buffer_ref(dev->device_ref);
>> +        if (!ost->enc_ctx->hw_device_ctx)
>> +            return AVERROR(ENOMEM);
>>      } else {
>>          // No device required, or no device available.
> 
> I've got a question on these comments for long time which is not related to the patch
> itself, isn't it a good idea to treat them as logs with a verbosity level of warning or DEBUG,
> then user/developer may catch/seek this easier if something unexpected happens?

I didn't include a log line here because it would be printed by /every/ encoder created which didn't touch one of the other cases.

> Another instance is:
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190815043242.20478-1-linjie.fu@intel.com/
> 
>> -        return 0;
>>      }
>> +    return 0;
>>  }
>>
>>  static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)
>> --
> 
> Works for me for #8637, and also tested in device context methods with
> -init_hw_device qsv=hw:/dev/dri/renderD128 -filter_hw_device hw
> 
> Hence would it be better to mention the ticket number as well?

Huh, yeah, that's exactly the same problem but with the device creation hidden in the ad-hoc libmfx setup code rather than in a filter graph.

Added a note to that effect and applied.

Thanks,

- Mark