Message ID | d0202e47-cd2e-b393-f042-aacbfda2890a@jkqxz.net |
---|---|
State | Accepted |
Commit | 706ed34ce7aca7be4adab69a55dab02f4573591a |
Headers | show |
Series | [FFmpeg-devel] ffmpeg: Don't require a known device to pass a frames context to an encoder | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
> 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
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. - return 0; } + return 0; } static int hwaccel_retrieve_data(AVCodecContext *avctx, AVFrame *input)