diff mbox

[FFmpeg-devel,03/13] lavc: Use hardware config information in ff_get_format()

Message ID 20171118184713.11490-3-sw@jkqxz.net
State Superseded
Headers show

Commit Message

Mark Thompson Nov. 18, 2017, 6:47 p.m. UTC
This removes the dependency that hardware pixel formats previously had on
AVHWAccel instances, meaning only those which actually do something need
exist after this patch.

Also updates avcodec_default_get_format() to be able to choose hardware
formats if either a matching device has been supplied or no additional
external configuration is required, and avcodec_get_hw_frames_parameters()
to use the hardware config rather than searching the old hwaccel list.

The FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS mechanism is deleted because it
no longer does anything (the codec already contains the pointers to the
matching hwaccels).
---
 libavcodec/avcodec.h  |   7 --
 libavcodec/cuviddec.c |   2 -
 libavcodec/decode.c   | 285 +++++++++++++++++++++++++++++++++++---------------
 libavcodec/internal.h |  11 +-
 4 files changed, 208 insertions(+), 97 deletions(-)

Comments

Mark Thompson Nov. 18, 2017, 7:15 p.m. UTC | #1
On 18/11/17 18:47, Mark Thompson wrote:
> This removes the dependency that hardware pixel formats previously had on
> AVHWAccel instances, meaning only those which actually do something need
> exist after this patch.
> 
> Also updates avcodec_default_get_format() to be able to choose hardware
> formats if either a matching device has been supplied or no additional
> external configuration is required, and avcodec_get_hw_frames_parameters()
> to use the hardware config rather than searching the old hwaccel list.
> 
> The FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS mechanism is deleted because it
> no longer does anything (the codec already contains the pointers to the
> matching hwaccels).
> ---
>  libavcodec/avcodec.h  |   7 --
>  libavcodec/cuviddec.c |   2 -
>  libavcodec/decode.c   | 285 +++++++++++++++++++++++++++++++++++---------------
>  libavcodec/internal.h |  11 +-
>  4 files changed, 208 insertions(+), 97 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index a7f1e23fc2..8b2bec1ce9 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> ...
> +        for (i = 0;; i++) {
> +            hw_config = avctx->codec->hw_configs[i];
> +            if (!hw_config)
> +                break;
> +            if (hw_config->public.pix_fmt == user_choice)
>                  break;
> -            }
>          }

This needs to check for avctx->codec->hw_configs being non-NULL, and immediately fall into the below case if it isn't.  (Stupidly I was only testing with codecs where it's set...)

>  
> -        if (!setup_hwaccel(avctx, ret, desc->name))
> +        if (!hw_config) {
> +            // No config available, so no extra setup required.
> +            ret = user_choice;
>              break;
> +        }

Fixed locally.

- Mark
Philip Langdale Nov. 21, 2017, 3:56 a.m. UTC | #2
On Sat, 18 Nov 2017 19:15:32 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> On 18/11/17 18:47, Mark Thompson wrote:
> > This removes the dependency that hardware pixel formats previously
> > had on AVHWAccel instances, meaning only those which actually do
> > something need exist after this patch.
> > 
> > Also updates avcodec_default_get_format() to be able to choose
> > hardware formats if either a matching device has been supplied or
> > no additional external configuration is required, and
> > avcodec_get_hw_frames_parameters() to use the hardware config
> > rather than searching the old hwaccel list.
> > 
> > The FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS mechanism is deleted because
> > it no longer does anything (the codec already contains the pointers
> > to the matching hwaccels).
> > ---
> >  libavcodec/avcodec.h  |   7 --
> >  libavcodec/cuviddec.c |   2 -
> >  libavcodec/decode.c   | 285
> > +++++++++++++++++++++++++++++++++++---------------
> > libavcodec/internal.h |  11 +- 4 files changed, 208 insertions(+),
> > 97 deletions(-)
> > 
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index a7f1e23fc2..8b2bec1ce9 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > ...
> > +        for (i = 0;; i++) {
> > +            hw_config = avctx->codec->hw_configs[i];
> > +            if (!hw_config)
> > +                break;
> > +            if (hw_config->public.pix_fmt == user_choice)
> >                  break;
> > -            }
> >          }  
> 
> This needs to check for avctx->codec->hw_configs being non-NULL, and
> immediately fall into the below case if it isn't.  (Stupidly I was
> only testing with codecs where it's set...)
>
> >  
> > -        if (!setup_hwaccel(avctx, ret, desc->name))
> > +        if (!hw_config) {
> > +            // No config available, so no extra setup required.
> > +            ret = user_choice;
> >              break;
> > +        }  
> 
> Fixed locally.

Looks fine with the fix.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel




--phil
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 5bbeb67a0d..fb18f46ea2 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -3652,13 +3652,6 @@  typedef struct AVHWAccel {
      * that avctx->hwaccel_priv_data is invalid.
      */
     int (*frame_params)(AVCodecContext *avctx, AVBufferRef *hw_frames_ctx);
-
-    /**
-     * Some hwaccels are ambiguous if only the id and pix_fmt fields are used.
-     * If non-NULL, the associated AVCodec must have
-     * FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS set.
-     */
-    const AVClass *decoder_class;
 } AVHWAccel;
 
 /**
diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
index 0e5123304a..346e54e7c6 100644
--- a/libavcodec/cuviddec.c
+++ b/libavcodec/cuviddec.c
@@ -1120,7 +1120,6 @@  static const AVCodecHWConfigInternal *cuvid_hw_configs[] = {
         .type           = AVMEDIA_TYPE_VIDEO, \
         .id             = AV_CODEC_ID_##X, \
         .pix_fmt        = AV_PIX_FMT_CUDA, \
-        .decoder_class  = &x##_cuvid_class, \
     }; \
     AVCodec ff_##x##_cuvid_decoder = { \
         .name           = #x "_cuvid", \
@@ -1135,7 +1134,6 @@  static const AVCodecHWConfigInternal *cuvid_hw_configs[] = {
         .receive_frame  = cuvid_output_frame, \
         .flush          = cuvid_flush, \
         .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AVOID_PROBING, \
-        .caps_internal  = FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS, \
         .pix_fmts       = (const enum AVPixelFormat[]){ AV_PIX_FMT_CUDA, \
                                                         AV_PIX_FMT_NV12, \
                                                         AV_PIX_FMT_P010, \
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index a7f1e23fc2..8b2bec1ce9 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -40,6 +40,7 @@ 
 #include "avcodec.h"
 #include "bytestream.h"
 #include "decode.h"
+#include "hwaccel.h"
 #include "internal.h"
 #include "thread.h"
 
@@ -1077,33 +1078,67 @@  int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
     return ret;
 }
 
-static int is_hwaccel_pix_fmt(enum AVPixelFormat pix_fmt)
+enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *avctx,
+                                              const enum AVPixelFormat *fmt)
 {
-    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
-    return desc->flags & AV_PIX_FMT_FLAG_HWACCEL;
-}
-
-enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s, const enum AVPixelFormat *fmt)
-{
-    while (*fmt != AV_PIX_FMT_NONE && is_hwaccel_pix_fmt(*fmt))
-        ++fmt;
-    return fmt[0];
-}
-
-static AVHWAccel *find_hwaccel(AVCodecContext *avctx,
-                               enum AVPixelFormat pix_fmt)
-{
-    AVHWAccel *hwaccel = NULL;
-    const AVClass *av_class =
-        (avctx->codec->caps_internal & FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS)
-        ? avctx->codec->priv_class : NULL;
-
-    while ((hwaccel = av_hwaccel_next(hwaccel))) {
-        if (hwaccel->decoder_class == av_class && hwaccel->id == avctx->codec_id
-            && hwaccel->pix_fmt == pix_fmt)
-            return hwaccel;
+    const AVPixFmtDescriptor *desc;
+    const AVCodecHWConfig *config;
+    int i, n;
+
+    // If a device was supplied when the codec was opened, assume that the
+    // user wants to use it.
+    if (avctx->hw_device_ctx && avctx->codec->hw_configs) {
+        AVHWDeviceContext *device_ctx =
+            (AVHWDeviceContext*)avctx->hw_device_ctx->data;
+        for (i = 0;; i++) {
+            config = &avctx->codec->hw_configs[i]->public;
+            if (!config)
+                break;
+            if (!(config->methods &
+                  AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX))
+                continue;
+            if (device_ctx->type != config->device_type)
+                continue;
+            for (n = 0; fmt[n] != AV_PIX_FMT_NONE; n++) {
+                if (config->pix_fmt == fmt[n])
+                    return fmt[n];
+            }
+        }
+    }
+    // No device or other setup, so we have to choose from things which
+    // don't any other external information.
+
+    // If the last element of the list is a software format, choose it
+    // (this should be best software format if any exist).
+    for (n = 0; fmt[n] != AV_PIX_FMT_NONE; n++);
+    desc = av_pix_fmt_desc_get(fmt[n - 1]);
+    if (!(desc->flags & AV_PIX_FMT_FLAG_HWACCEL))
+        return fmt[n - 1];
+
+    // Finally, traverse the list in order and choose the first entry
+    // with no external dependencies (if there is no hardware configuration
+    // information available then this just picks the first entry).
+    for (n = 0; fmt[n] != AV_PIX_FMT_NONE; n++) {
+        for (i = 0;; i++) {
+            config = avcodec_get_hw_config(avctx->codec, i);
+            if (!config)
+                break;
+            if (config->pix_fmt == fmt[n])
+                break;
+        }
+        if (!config) {
+            // No specific config available, so the decoder must be able
+            // to handle this format without any additional setup.
+            return fmt[n];
+        }
+        if (config->methods & AV_CODEC_HW_CONFIG_METHOD_INTERNAL) {
+            // Usable with only internal setup.
+            return fmt[n];
+        }
     }
-    return NULL;
+
+    // Nothing is usable, give up.
+    return AV_PIX_FMT_NONE;
 }
 
 int ff_decode_get_hw_frames_ctx(AVCodecContext *avctx,
@@ -1167,9 +1202,19 @@  int avcodec_get_hw_frames_parameters(AVCodecContext *avctx,
                                      AVBufferRef **out_frames_ref)
 {
     AVBufferRef *frames_ref = NULL;
-    AVHWAccel *hwa = find_hwaccel(avctx, hw_pix_fmt);
-    int ret;
+    const AVCodecHWConfigInternal *hw_config;
+    const AVHWAccel *hwa;
+    int i, ret;
 
+    for (i = 0;; i++) {
+        hw_config = avctx->codec->hw_configs[i];
+        if (!hw_config)
+            return AVERROR(ENOENT);
+        if (hw_config->public.pix_fmt == hw_pix_fmt)
+            break;
+    }
+
+    hwa = hw_config->hwaccel;
     if (!hwa || !hwa->frame_params)
         return AVERROR(ENOENT);
 
@@ -1186,59 +1231,73 @@  int avcodec_get_hw_frames_parameters(AVCodecContext *avctx,
     return ret;
 }
 
-static int setup_hwaccel(AVCodecContext *avctx,
-                         const enum AVPixelFormat fmt,
-                         const char *name)
+static int hwaccel_init(AVCodecContext *avctx,
+                        const AVCodecHWConfigInternal *hw_config)
 {
-    AVHWAccel *hwa = find_hwaccel(avctx, fmt);
-    int ret        = 0;
-
-    if (!hwa) {
-        av_log(avctx, AV_LOG_ERROR,
-               "Could not find an AVHWAccel for the pixel format: %s\n",
-               name);
-        return AVERROR(ENOENT);
-    }
+    const AVHWAccel *hwaccel;
+    int err;
 
-    if (hwa->capabilities & AV_HWACCEL_CODEC_CAP_EXPERIMENTAL &&
+    hwaccel = hw_config->hwaccel;
+    if (hwaccel->capabilities & AV_HWACCEL_CODEC_CAP_EXPERIMENTAL &&
         avctx->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
         av_log(avctx, AV_LOG_WARNING, "Ignoring experimental hwaccel: %s\n",
-               hwa->name);
+               hwaccel->name);
         return AVERROR_PATCHWELCOME;
     }
 
-    if (hwa->priv_data_size) {
-        avctx->internal->hwaccel_priv_data = av_mallocz(hwa->priv_data_size);
+    if (hwaccel->priv_data_size) {
+        avctx->internal->hwaccel_priv_data =
+            av_mallocz(hwaccel->priv_data_size);
         if (!avctx->internal->hwaccel_priv_data)
             return AVERROR(ENOMEM);
     }
 
-    avctx->hwaccel = hwa;
-    if (hwa->init) {
-        ret = hwa->init(avctx);
-        if (ret < 0) {
-            av_freep(&avctx->internal->hwaccel_priv_data);
-            avctx->hwaccel = NULL;
-            return ret;
-        }
+    avctx->hwaccel = (AVHWAccel*)hwaccel;
+    err = hwaccel->init(avctx);
+    if (err < 0) {
+        av_log(avctx, AV_LOG_ERROR, "Failed setup for format %s: "
+               "hwaccel initialisation returned error.\n",
+               av_get_pix_fmt_name(hw_config->public.pix_fmt));
+        av_freep(&avctx->internal->hwaccel_priv_data);
+        avctx->hwaccel = NULL;
+        return err;
     }
 
     return 0;
 }
 
+static void hwaccel_uninit(AVCodecContext *avctx)
+{
+    if (avctx->hwaccel && avctx->hwaccel->uninit)
+        avctx->hwaccel->uninit(avctx);
+
+    av_freep(&avctx->internal->hwaccel_priv_data);
+
+    avctx->hwaccel = NULL;
+
+    av_buffer_unref(&avctx->hw_frames_ctx);
+}
+
 int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
 {
     const AVPixFmtDescriptor *desc;
     enum AVPixelFormat *choices;
-    enum AVPixelFormat ret;
-    unsigned n = 0;
-
-    while (fmt[n] != AV_PIX_FMT_NONE)
-        ++n;
-
+    enum AVPixelFormat ret, user_choice;
+    const AVCodecHWConfigInternal *hw_config;
+    const AVCodecHWConfig *config;
+    int i, n, err;
+
+    // Find end of list.
+    for (n = 0; fmt[n] != AV_PIX_FMT_NONE; n++);
+    // Must contain at least one entry.
     av_assert0(n >= 1);
-    avctx->sw_pix_fmt = fmt[n - 1];
-    av_assert2(!is_hwaccel_pix_fmt(avctx->sw_pix_fmt));
+    // If a software format is available, it must be the last entry.
+    desc = av_pix_fmt_desc_get(fmt[n - 1]);
+    if (desc->flags & AV_PIX_FMT_FLAG_HWACCEL) {
+        // No software format is available.
+    } else {
+        avctx->sw_pix_fmt = fmt[n - 1];
+    }
 
     choices = av_malloc_array(n + 1, sizeof(*choices));
     if (!choices)
@@ -1247,44 +1306,104 @@  int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
     memcpy(choices, fmt, (n + 1) * sizeof(*choices));
 
     for (;;) {
-        if (avctx->hwaccel && avctx->hwaccel->uninit)
-            avctx->hwaccel->uninit(avctx);
-        av_freep(&avctx->internal->hwaccel_priv_data);
-        avctx->hwaccel = NULL;
+        // Remove the previous hwaccel, if there was one.
+        hwaccel_uninit(avctx);
 
-        av_buffer_unref(&avctx->hw_frames_ctx);
-
-        ret = avctx->get_format(avctx, choices);
+        user_choice = avctx->get_format(avctx, choices);
+        if (user_choice == AV_PIX_FMT_NONE) {
+            // Explicitly chose nothing, give up.
+            ret = AV_PIX_FMT_NONE;
+            break;
+        }
 
-        desc = av_pix_fmt_desc_get(ret);
+        desc = av_pix_fmt_desc_get(user_choice);
         if (!desc) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid format returned by "
+                   "get_format() callback.\n");
             ret = AV_PIX_FMT_NONE;
             break;
         }
+        av_log(avctx, AV_LOG_DEBUG, "Format %s chosen by get_format().\n",
+               desc->name);
 
-        if (!(desc->flags & AV_PIX_FMT_FLAG_HWACCEL))
+        for (i = 0; i < n; i++) {
+            if (choices[i] == user_choice)
+                break;
+        }
+        if (i == n) {
+            av_log(avctx, AV_LOG_ERROR, "Invalid return from get_format(): "
+                   "%s not in possible list.\n", desc->name);
             break;
+        }
 
-        if (avctx->hw_frames_ctx) {
-            AVHWFramesContext *hw_frames_ctx = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
-            if (hw_frames_ctx->format != ret) {
-                av_log(avctx, AV_LOG_ERROR, "Format returned from get_buffer() "
-                       "does not match the format of provided AVHWFramesContext\n");
-                ret = AV_PIX_FMT_NONE;
+        for (i = 0;; i++) {
+            hw_config = avctx->codec->hw_configs[i];
+            if (!hw_config)
+                break;
+            if (hw_config->public.pix_fmt == user_choice)
                 break;
-            }
         }
 
-        if (!setup_hwaccel(avctx, ret, desc->name))
+        if (!hw_config) {
+            // No config available, so no extra setup required.
+            ret = user_choice;
             break;
+        }
+        config = &hw_config->public;
+
+        if (config->methods &
+            AV_CODEC_HW_CONFIG_METHOD_HW_FRAMES_CTX &&
+            avctx->hw_frames_ctx) {
+            const AVHWFramesContext *frames_ctx =
+                (AVHWFramesContext*)avctx->hw_frames_ctx->data;
+            if (frames_ctx->format != user_choice) {
+                av_log(avctx, AV_LOG_ERROR, "Invalid setup for format %s: "
+                       "does not match the format of the provided frames "
+                       "context.\n", desc->name);
+                goto try_again;
+            }
+        } else if (config->methods &
+                   AV_CODEC_HW_CONFIG_METHOD_HW_DEVICE_CTX &&
+                   avctx->hw_device_ctx) {
+            const AVHWDeviceContext *device_ctx =
+                (AVHWDeviceContext*)avctx->hw_device_ctx->data;
+            if (device_ctx->type != config->device_type) {
+                av_log(avctx, AV_LOG_ERROR, "Invalid setup for format %s: "
+                       "does not match the type of the provided device "
+                       "context.\n", desc->name);
+                goto try_again;
+            }
+        } else if (config->methods &
+                   AV_CODEC_HW_CONFIG_METHOD_INTERNAL) {
+            // Internal-only setup, no additional configuration.
+        } else if (config->methods &
+                   AV_CODEC_HW_CONFIG_METHOD_AD_HOC) {
+            // Some ad-hoc configuration we can't see and can't check.
+        } else {
+            av_log(avctx, AV_LOG_ERROR, "Invalid setup for format %s: "
+                   "missing configuration.\n", desc->name);
+            goto try_again;
+        }
+        if (hw_config->hwaccel) {
+            av_log(avctx, AV_LOG_DEBUG, "Format %s requires hwaccel "
+                   "initialisation.\n", desc->name);
+            err = hwaccel_init(avctx, hw_config);
+            if (err < 0)
+                goto try_again;
+        }
+        ret = user_choice;
+        break;
 
-        /* Remove failed hwaccel from choices */
-        for (n = 0; choices[n] != ret; n++)
-            av_assert0(choices[n] != AV_PIX_FMT_NONE);
-
-        do
-            choices[n] = choices[n + 1];
-        while (choices[n++] != AV_PIX_FMT_NONE);
+    try_again:
+        av_log(avctx, AV_LOG_DEBUG, "Format %s not usable, retrying "
+               "get_format() without it.\n", desc->name);
+        for (i = 0; i < n; i++) {
+            if (choices[i] == user_choice)
+                break;
+        }
+        for (; i + 1 < n; i++)
+            choices[i] = choices[i + 1];
+        --n;
     }
 
     av_freep(&choices);
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index d47ce0e93d..a3d046199b 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -69,11 +69,6 @@ 
  */
 #define FF_CODEC_CAP_SLICE_THREAD_HAS_MF    (1 << 5)
 
-/**
- * Allow only AVHWAccels which have a matching decoder_class field.
- */
-#define FF_CODEC_CAP_HWACCEL_REQUIRE_CLASS  (1 << 6)
-
 #ifdef TRACE
 #   define ff_tlog(ctx, ...) av_log(ctx, AV_LOG_TRACE, __VA_ARGS__)
 #else
@@ -378,6 +373,12 @@  int ff_side_data_update_matrix_encoding(AVFrame *frame,
  * Select the (possibly hardware accelerated) pixel format.
  * This is a wrapper around AVCodecContext.get_format() and should be used
  * instead of calling get_format() directly.
+ *
+ * The list of pixel formats must contain at least one valid entry, and is
+ * terminated with AV_PIX_FMT_NONE.  If it is possible to decode to software,
+ * the last entry in the list must be the most accurate software format.
+ * If it is not possible to decode to software, AVCodecContext.sw_pix_fmt
+ * must be set before calling this function.
  */
 int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt);