On Wed, 11 Oct 2017 01:48:51 +0200
Anton Khirnov <an...@khirnov.net> wrote:
> Quoting wm4 (2017-09-26 17:09:56)
> > From: wm4 <nfx...@googlemail.com>
> >
> > This adds a new API, which allows the API user to query the required
> > AVHWFramesContext parameters. This also reduces code duplication across
> > the hwaccels by introducing ff_decode_get_hw_frames_ctx(), which uses
> > the new API function. It takes care of initializing the hw_frames_ctx
> > if needed, and does additional error handling and API usage checking.
> >
> > Support for VDA and Cuvid missing.
> > ---
> > Added the rest of the codecs, fixed the other TODO items.
> >
> > Cuvid should be able to support it, but I still didn't manage to
> > install the Cuda SDK. I don't care about VDA, it should just be
> > removed.
> >
> > Ready to be applied, if you ask me.
> > ---
> > doc/APIchanges | 3 +
> > libavcodec/avcodec.h | 82 +++++++++++++++++
> > libavcodec/decode.c | 67 ++++++++++++++
> > libavcodec/decode.h | 9 ++
> > libavcodec/dxva2.c | 55 +++++------
> > libavcodec/dxva2_h264.c | 3 +
> > libavcodec/dxva2_hevc.c | 3 +
> > libavcodec/dxva2_internal.h | 3 +
> > libavcodec/dxva2_mpeg2.c | 3 +
> > libavcodec/dxva2_vc1.c | 5 +
> > libavcodec/vaapi_decode.c | 216
> > +++++++++++++++++++++-----------------------
> > libavcodec/vaapi_decode.h | 5 +-
> > libavcodec/vaapi_h264.c | 1 +
> > libavcodec/vaapi_hevc.c | 1 +
> > libavcodec/vaapi_mpeg2.c | 1 +
> > libavcodec/vaapi_mpeg4.c | 2 +
> > libavcodec/vaapi_vc1.c | 2 +
> > libavcodec/vaapi_vp8.c | 1 +
> > libavcodec/vdpau.c | 58 ++++++------
> > libavcodec/vdpau_h264.c | 1 +
> > libavcodec/vdpau_hevc.c | 1 +
> > libavcodec/vdpau_internal.h | 2 +
> > libavcodec/vdpau_mpeg12.c | 1 +
> > libavcodec/vdpau_mpeg4.c | 1 +
> > libavcodec/vdpau_vc1.c | 2 +
> > libavcodec/version.h | 2 +-
> > 26 files changed, 349 insertions(+), 181 deletions(-)
> >
>
> Okay, this is less icky than I expected.
>
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index fa27007f44..aa419dc0e9 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -13,6 +13,9 @@ libavutil: 2017-03-23
> >
> > API changes, most recent first:
> >
> > +2017-xx-xx - xxxxxxx - lavc 58.5.0 - avcodec.h
> > + Add avcodec_fill_hw_frames_parameters().
> > +
> > 2017-xx-xx - xxxxxxx - lavu 56.6.0 - pixdesc.h
> > Add av_color_range_from_name(), av_color_primaries_from_name(),
> > av_color_transfer_from_name(), av_color_space_from_name(), and
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 162f1abe4b..0d1e1e5254 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -2990,6 +2990,16 @@ typedef struct AVHWAccel {
> > * Internal hwaccel capabilities.
> > */
> > int caps_internal;
> > +
> > + /**
> > + * Fill the given hw_frames context with current codec parameters.
> > Called
> > + * from get_format. Refer to avcodec_fill_hw_frames_parameters() for
> > + * details.
> > + *
> > + * This CAN be called before AVHWAccel.init is called, and you must
> > assume
> > + * that avctx->hwaccel_priv_data is invalid.
> > + */
> > + int (*frame_params)(AVCodecContext *avctx, AVBufferRef *hw_frames_ctx);
> > } AVHWAccel;
> >
> > /**
> > @@ -3436,6 +3446,78 @@ int avcodec_open2(AVCodecContext *avctx, const
> > AVCodec *codec, AVDictionary **op
> > */
> > int avcodec_close(AVCodecContext *avctx);
> >
> > +/**
> > + * Fill the given hw_frames_ctx (or actually, the AVHWFramesContext
> > pointed to
> > + * by it) with values adequate for hardware decoding. This is meant to get
> > + * called from the get_format callback, and is a helper for preparing a
> > + * AVHWFramesContext for AVCodecContext.hw_frames_ctx. This API is for
> > decoding
> > + * with certain hardware acceleration modes/APIs only. Calling this
> > function is
> > + * not a requirement, but strongly encouraged, unless you know what you are
> > + * doing.
>
> I mildly dislike the last sentence. This should be a convenience
> function intended to simplify user's lives in common cases, with manual
> creation still fully supported and allowed. Statements like these in the
> docs might make people think manual creation is something like a second
> class citizen, which it certainly should not be.
Well, it's the truth. How to use the hw_frames_ctx API is mostly
undocumented, and requires deeper knowledge about how each libavcodec
hwaccel implementation does things. It probably requires reading the
implementation code. How is a user supposed to know you need to uses
coded_width/height instead of width/height for the dimensions? (At
least the required surface alignment is documented by the underlying
OS API.) How does the user know which sw_format to set? The D3D hwaccels
have additional requirements of having to set fields like surface_type.
So I'd say this sentence is justified. Should I remove it?
> > + *
> > + * Alternatively to this, an API user can set AVCodecContext.hw_device_ctx,
> > + * which sets up AVCodecContext.hw_frames_ctx fully automatically, and
> > makes
> > + * it unnecessary to call this function or having to care about
> > + * AVHWFramesContext initialization at all.
> > + *
> > + * There are a number of requirements for calling this function:
> > + *
> > + * - It must be called from get_format with the same avctx parameter you
> > are
> > + * going to use for decoding. Calling it outside of get_format is not
> > allowed,
> > + * and can trigger undefined behavior.
> > + * - If the decoder does not support this functionality, AVERROR(ENOENT)
> > will
> > + * be returned. This happens only if the format is a software format, or
> > if
> > + * the decoder does not support custom allocated AVHWFramesContext
> > properly.
> > + * Be aware that even if this returns successfully, hwaccel
> > initialization
> > + * could fail later.
> > + * - The hw_pix_fmt must be one of the choices suggested by get_format. If
> > the
> > + * user decides to use a hw_frames_ctx prepared with this API function,
> > the
> > + * user must return the same hw_pix_fmt from get_format.
> > + * - The hw_frames_ctx must be allocated from a device type that supports
> > the
> > + * given hw_pix_fmt.
> > + * - The passed hw_frames_ctx must not have been initialized yet.
> > + * - The API function may overwrite any fields in the hw_frames_ctx. It
> > will not
> > + * actually initialize the context. A user calls this API function to get
> > + * basic parameters, and can afterwards modify the parameters as needed.
> > + * - After calling this API function, it is the user's responsibility to
> > + * initialize the hw_frames_ctx, and to set AVCodecContext.hw_frames_ctx
> > + * to it. If done, this must be done before returning from get_format
> > (this
> > + * is implied by the normal AVCodecContext.hw_frames_ctx API rules).
> > + * - The hw_frames_ctx parameters may change every time time get_format is
> > + * called. Also, AVCodecContext.hw_frames_ctx is reset before
> > get_format. So
> > + * you are inherently required to go through this process again on every
> > + * get_format call.
> > + * - It is perfectly possible to call this function without actually using
> > + * the resulting hw_frames_ctx. One use-case might be trying to reuse a
> > + * previously initialized hw_frames_ctx, and calling this API function
> > only
> > + * to test whether the required frame parameters have changed.
> > + *
> > + * The function will set at least the following fields on hw_frames_ctx
> > + * (potentially more, depending on hwaccel API):
> > + *
> > + * - Set the format field to hw_pix_fmt.
> > + * - Set the sw_format field to the most suited and most versatile format.
> > (An
> > + * implication is that this will prefer generic formats over opaque
> > formats
> > + * with arbitrary restrictions, if possible.)
> > + * - Set the width/height fields to the coded frame size, rounded up to the
> > + * API-specific minimum alignment.
> > + * - Only _if_ the hwaccel requires a pre-allocated pool: set the
> > initial_pool_size
> > + * field to the number of maximum reference surfaces possible with the
> > codec,
> > + * plus 1 surface for the user to work (meaning the user can safely
> > reference
> > + * at most 1 decoded surface at a time), plus additional buffering
> > introduced
> > + * by frame threading. If the hwaccel does not require pre-allocation,
> > the
> > + * field is left to 0, and the decoder will allocate new surfaces on
> > demand
> > + * during decoding.
> > + *
> > + * @param avctx The context which is currently calling get_format, and
> > which
> > + * implicitly contains all state needed for filling
> > hw_frames_ctx
> > + * properly.
> > + * @param hw_pix_fmt The hwaccel format you are going to return from
> > get_format.
> > + * @param hw_frames_ctx A reference to an _uninitialized_
> > AVHWFramesContext.
> > + * Fields will be set to values required for
> > decoding.
>
> The frames context can potentially contain user-allocated content which
> is then freed in the user-set free() callback. The semantics of that
> memory management is then unclear.
>
> Is there any reason the av_hwframe_ctx_alloc() call must be done by the
> user? Doing it inside this function (and so changing the last parameter
> to AVBufferRef**) seems to avoid plenty of weird corner cases with no
> disadvantages.
>
> Then we have to say whether this function is allowed to set the
> opaque/free fields and any possibly-allocated fields in the
> type-specific contexts (e.g. attributes for vaapi). Forbidding it makes
> it all simpler but may bite us later when it turns out they are
> necessary because some hwaccel is particularly insane.
I don't think there's any reason not to do it, and it does sound
cleaner. It would look like this:
int avcodec_get_hw_frames_parameters(AVCodecContext *avctx,
AVBufferRef *device_ref,
enum AVPixelFormat hw_pix_fmt,
AVBufferRef **out_frames_ref);
Maybe?
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
