On 28/03/17 11:18, wm4 wrote:
> On Mon, 27 Mar 2017 21:38:12 +0100
> Mark Thompson <s...@jkqxz.net> wrote:
>
>> This also adds a new field to AVHWAccel to set supported hardware device
>> types, so that we can query the right hwaccel.
>> ---
>> Not entirely sure that the AVCodecContext should be the first argument of
>> probe_hw() - AVCodecParameters might be nicer, but we are likely to end up
>> needing an AVCodecContext anyway. (More generally, please bikeshed freely
>> over naming and structures used.)
>>
>> The probe_hw() function on the D*VA hwaccel would be where the extra
>> alignment gets set (when AVCodecContext.hw_device_ctx is not used).
>>
>>
>> doc/APIchanges | 3 +++
>> libavcodec/avcodec.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/decode.c | 53
>> ++++++++++++++++++++++++++++++++++++++++++++-------
>> libavcodec/encode.c | 13 +++++++++++++
>> libavcodec/internal.h | 10 ++++++++++
>> libavcodec/utils.c | 15 +++++++++++++++
>> 6 files changed, 138 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 5da821c90..649d35a08 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -13,6 +13,9 @@ libavutil: 2017-03-23
>>
>> API changes, most recent first:
>>
>> +2017-04-xx - xxxxxxx - lavc 58.x+1.0 - avcodec.h
>> + Add AVHWAccel.device_type and avcodec_probe_hw().
>> +
>> 2017-04-xx - xxxxxxx - lavc 58.x.y+1 - avcodec.h
>> Add AV_HWACCEL_FLAG_ALLOW_PROFILE_MISMATCH.
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 2aa70ca4f..8f7293d02 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -35,6 +35,7 @@
>> #include "libavutil/cpu.h"
>> #include "libavutil/dict.h"
>> #include "libavutil/frame.h"
>> +#include "libavutil/hwcontext.h"
>> #include "libavutil/log.h"
>> #include "libavutil/pixfmt.h"
>> #include "libavutil/rational.h"
>> @@ -2857,6 +2858,15 @@ typedef struct AVCodec {
>> * packets before decoding.
>> */
>> const char *bsfs;
>> +
>> + /**
>> + * Test support for the given encode/decode parameters, and if possible
>> + * set the fields in the frames context to a set of usable values.
>> + *
>> + * If a hwaccel is used with a decoder and it also has a probe_hw()
>> + * function, that will be called instead of this.
>> + */
>> + int (*probe_hw)(AVCodecContext *avctx, AVHWFramesContext *hw_frames);
>> } AVCodec;
>>
>> /**
>> @@ -2898,6 +2908,11 @@ typedef struct AVHWAccel {
>> */
>> int capabilities;
>>
>> + /**
>> + * Hardware device type which this hwaccel can use.
>> + */
>> + enum AVHWDeviceType device_type;
>> +
>> /*****************************************************************
>> * No fields below this line are part of the public API. They
>> * may not be used outside of libavcodec and can be changed and
>> @@ -2988,6 +3003,11 @@ typedef struct AVHWAccel {
>> * Internal hwaccel capabilities.
>> */
>> int caps_internal;
>> +
>> + /**
>> + * Probe hardware for support and parameters.
>> + */
>> + int (*probe_hw)(AVCodecContext *avctx, AVHWFramesContext *hw_frames);
>> } AVHWAccel;
>>
>> /**
>> @@ -4927,6 +4947,37 @@ const AVCodecDescriptor
>> *avcodec_descriptor_get_by_name(const char *name);
>> AVCPBProperties *av_cpb_properties_alloc(size_t *size);
>>
>> /**
>> + * Probe hardware support for the given codec parameters.
>> + *
>> + * For decoders, it tests whether a stream with the given parameters can be
>> + * decoded on the given device. For encoders, it tests whether encoding
>> with
>> + * the given parameters is possible on the device.
>> + *
>> + * On successful return, the fields of hw_frames_ref are set such that, once
>> + * initialised, it will be usable as AVCodecContext.hw_frames_ctx. If the
>> + * user has additional requirements to apply to the frames context (for
>> + * example, for a larger pool size or for frames created with some system-
>> + * specific attribute) then they should be applied after this call but
>> before
>> + * calling av_hwframe_ctx_init().
>> + *
>> + * The device to use is provided as the allocating context of hw_frames_ref.
>> + * (Even if set, the hw_frames_ctx and hw_device_ctx fields of avctx are
>> + * ignored.)
>> + *
>> + * @param avctx Codec context with relevant fields set. It need not have
>> been
>> + * opened.
>> + * @param hw_frames_ref A reference to a newly-allocated AVHWFramesContext,
>> + * which will be used to return the result.
>> + * @return 0 on success, otherwise negative error code:
>> + * AVERROR(ENOSYS): This function is not implemented (hardware support
>> + * may or may not be available).
>> + * AVERROR(ENODEV): The device is not usable.
>> + * AVERROR(EINVAL): The codec parameters are not supported.
>> + * Other errors: Probing failed for some other reason.
>> + */
>> +int avcodec_probe_hw(AVCodecContext *avctx, AVBufferRef *hw_frames_ref);
>
> This API still makes me a bit uneasy. Regular get_format hwaccel init
> relies on the fact that the decoder has set some fields from parsing
> the bit stream, like the current sw_pixfmt or frame size. These aren't
> necessarily available on a "fresh" AVCodecContext (not even if
> extradata is available).
>
> So normally, the only time this will work properly is in the get_format
> callback. Which is ok, but which I think isn't the entire intention? I
> think I'd be fine with allowing this call only during get_format or so.
It's certainly the intent that this is usable outside get format. In the most
general case, probing with a codec, a profile, and maybe some dimensions can
test the hardware support without any stream at all.
> Or maybe it's supposed to return "approximate" results outside of
> get_format in the first place.
>
> (For encoding on the other hand it's probably fine - though I'm not
> sure if this is supposed to be called before or after opening the
> context.)
Before. I didn't provide a VAAPI implementation for encode because with the
current design it requires some nontrivial rearrangement of code to work (stuff
done by the current per-codec init functions needs to move to a callback so
that it's usable at other times - I'll do this if once we have some agreement
on the API).
> As a minor nit, this can't distinguish between required initial pool
> size and upper bound on pool size. Users who want to preallocate frames
> with e.g. vdpau would want to know the latter. (Well, I don't want
> that, and see no reason why anyone would want it, so I officially don't
> care about this argument.)
My intent is that for now the decoder would always allocate the returned
initial_pool_size + extra_hw_frames, so a user would do the same. (They
control extra_hw_frames so how many more they have is completely up to them,
but it works the same whether they are doing the allocation or lavc is.)
> I'll make full use of my permission to bikeshed, so here is a different
> API that impractically changes everything:
>
> Proposal 1:
>
> Add new init_hw_device and init_hw_frames callbacks:
>
> /*
> * Create a hw device and set ctx->hw_device_ctx. This might be used
> * for hardware decoding. If ENOSYS is returned, the next hwaccel
> * method is tried, and if all fail, software decoding is used.
> *
> * If hw_device_ctx is already set, this callback is not used, and
> * does not need to be set.
> *
> * If internal reinit is done (like on flush or if the codec
> * parameters change), hw_device_ctx is cleared, and this callback
> * can be called again.
> */
> int (*init_hw_device)(AVCodecContext *ctx, AVHWDeviceType type);
>
> /*
> * When this is called, ctx->hw_frames_ctx is already setup in a basic
> * way (such as frame dimensions and surface format). This callback
> * can be used to refine the frame parameters further.
> */
> int (*init_hw_frames)(AVCodecContext *ctx);
Hmm, that only covers the inline decode case, but it does do it rather well.
> Proposal 2:
>
> Add a mechanism to let the caller setup parameters in advance, and
> letting the hwaccel code use these parameters without needing callbacks
> back to user code.
>
> Proposal 2a:
>
> Add these fields to AVCodecContext:
>
> AVDictionary *hw_frames_params;
> void *hw_frames_opaque_params; // AVDictionary can't have pointers
> AVHWDeviceType hw_frames_opaque_params_type;
>
> Proposal 2b:
>
> Add this field to AVCodecContext:
>
> /*
> * When allocating a new hw_frames_ctx, use the parameters found in
> * this context as defaults (if the device type matches). The pixel
> * format and size will be changed, and the number of initial pool
> * frames can be extended.
> */
> AHWFramesContext *hw_frames_template;
I think this proposal is horrible.
> Proposal 3:
>
> Use your API, and restrict when/where it's allowed to be called.
Proposal 13:
Do both 1 and 3. 1 as above for inline decode, then:
int avcodec_probe_hw(AVCodecParameters *par, AVSomething *returned_stuff);
for the non-inline cases (both encode and decode).
Unclear what type the something should be - AVHWFramesContext is less useful
here, though it would be neat for decode when you already have the dimensions.
>> +
>> +/**
>> * @}
>> */
>>
>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>> index e4f6a0d72..4029683e6 100644
>> --- a/libavcodec/decode.c
>> +++ b/libavcodec/decode.c
>> @@ -701,13 +701,6 @@ static int is_hwaccel_pix_fmt(enum AVPixelFormat
>> 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(enum AVCodecID codec_id,
>> enum AVPixelFormat pix_fmt)
>> {
>> @@ -753,6 +746,28 @@ static int setup_hwaccel(AVCodecContext *avctx,
>> return 0;
>> }
>>
>> +enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext *s,
>> const enum AVPixelFormat *fmt)
>> +{
>> + if (s->hw_device_ctx) {
>> + const AVHWDeviceContext *device = (const
>> AVHWDeviceContext*)s->hw_device_ctx->data;
>> + const AVHWAccel *hwaccel;
>> + int i;
>> + for (i = 0; fmt[i] != AV_PIX_FMT_NONE; i++) {
>> + if (!is_hwaccel_pix_fmt(fmt[i]))
>> + continue;
>> + hwaccel = find_hwaccel(s->codec_id, fmt[i]);
>> + if (!hwaccel)
>> + continue;
>> + if (hwaccel->device_type == device->type)
>> + return fmt[i];
>> + }
>> + }
>> +
>> + while (*fmt != AV_PIX_FMT_NONE && is_hwaccel_pix_fmt(*fmt))
>> + ++fmt;
>> + return fmt[0];
>> +}
>
> Does this change normal decode behavior to using hwaccels if
> hw_device_ctx is used and get_format isn't set by the user? That's
> great, but could use a mention in the commit message. (And I suppose
> some would say this should be a separate commit.)
I was actually trying to change it to match by device type to make sure it gets
the same answer as probe_hw() does, but yes it does do somewhat more than that.
I'll split it into another patch and make that clear.
>> +
>> int ff_get_format(AVCodecContext *avctx, const enum AVPixelFormat *fmt)
>> {
>> const AVPixFmtDescriptor *desc;
>> @@ -1210,3 +1225,27 @@ void ff_decode_bsfs_uninit(AVCodecContext *avctx)
>> av_freep(&s->bsfs);
>> s->nb_bsfs = 0;
>> }
>> +
>> +int ff_decode_probe_hw(AVCodecContext *avctx, AVHWFramesContext *hw_frames)
>> +{
>> + const AVCodec *codec = avctx->codec;
>> + const AVHWAccel *hwaccel = NULL;
>> +
>> + if (!codec)
>> + return AVERROR(EINVAL);
>> +
>> + if (!hw_frames->device_ctx)
>> + return AVERROR(ENODEV);
>> +
>> + while ((hwaccel = av_hwaccel_next(hwaccel))) {
>> + if (hwaccel->id == avctx->codec_id &&
>> + hwaccel->device_type == hw_frames->device_ctx->type)
>> + break;
>> + }
>> + if (hwaccel && hwaccel->probe_hw)
>> + return hwaccel->probe_hw(avctx, hw_frames);
>> + else if (codec->probe_hw)
>> + return codec->probe_hw(avctx, hw_frames);
>> + else
>> + return AVERROR(ENOSYS);
>> +}
>> diff --git a/libavcodec/encode.c b/libavcodec/encode.c
>> index 9bb7ae5bd..d4914d7da 100644
>> --- a/libavcodec/encode.c
>> +++ b/libavcodec/encode.c
>> @@ -354,3 +354,16 @@ int attribute_align_arg
>> avcodec_receive_packet(AVCodecContext *avctx, AVPacket *
>> avctx->internal->buffer_pkt_valid = 0;
>> return 0;
>> }
>> +
>> +int ff_encode_probe_hw(AVCodecContext *avctx, AVHWFramesContext *hw_frames)
>> +{
>> + const AVCodec *codec = avctx->codec;
>> +
>> + if (!codec)
>> + return AVERROR(EINVAL);
>> +
>> + if (codec->probe_hw)
>> + return codec->probe_hw(avctx, hw_frames);
>> + else
>> + return AVERROR(ENOSYS);
>> +}
>> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>> index 403fb4a09..930b32fd6 100644
>> --- a/libavcodec/internal.h
>> +++ b/libavcodec/internal.h
>> @@ -285,4 +285,14 @@ int ff_decode_frame_props(AVCodecContext *avctx,
>> AVFrame *frame);
>> */
>> AVCPBProperties *ff_add_cpb_side_data(AVCodecContext *avctx);
>>
>> +/**
>> + * Probe decoder/hwaccel hardware support.
>> + */
>> +int ff_decode_probe_hw(AVCodecContext *avctx, AVHWFramesContext *hw_frames);
>> +
>> +/**
>> + * Probe encoder hardware support.
>> + */
>> +int ff_encode_probe_hw(AVCodecContext *avctx, AVHWFramesContext *hw_frames);
>> +
>> #endif /* AVCODEC_INTERNAL_H */
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index bc421f67f..2a73098ec 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1685,3 +1685,18 @@ int avcodec_parameters_to_context(AVCodecContext
>> *codec,
>>
>> return 0;
>> }
>> +
>> +int avcodec_probe_hw(AVCodecContext *avctx, AVBufferRef *hw_frames_ref)
>> +{
>> + AVHWFramesContext *hw_frames = (AVHWFramesContext*)hw_frames_ref->data;
>> +
>> + switch (avctx->codec_type) {
>> + case AVMEDIA_TYPE_VIDEO:
>> + if (av_codec_is_encoder(avctx->codec))
>> + return ff_encode_probe_hw(avctx, hw_frames);
>> + else
>> + return ff_decode_probe_hw(avctx, hw_frames);
>> + default:
>> + return AVERROR(ENOSYS);
>> + }
>> +}
>
> Otherwise all seems fine.
Thanks,
- Mark
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
