On Tue, 31 Jan 2017 00:05:21 +0000
Mark Thompson <[email protected]> wrote:
> For use by codec implementations which can allocate frames internally.
> ---
> On 30/01/17 12:55, wm4 wrote:
> > ... Maybe the exclusiveness of both fields could be emphasized
> > in a more explicit way:
> >
> > The user must never set both hw_frames_ctx and hw_device_ctx. The
> > API supports setting hw_frames_ctx for manual frame allocation, and
> > hw_device_ctx for automatic management. Both modes are exclusive to
> > each other.
>
> Now I'm wondering whether it would be simpler to just allow hw_device_ctx to
> be set when using hw_frames_ctx. In that case, hw_frames_ctx always wins if
> set by the user in the get_format() call and the device need not be cleared.
>
> That is, the hwaccel just sets hw_frames_ctx automatically from hw_device_ctx
> if it is not set (then readable by the user in get_buffer2()), and otherwise
> doesn't distinguish the cases. Maybe that is nicer?
>
> (Not reflected below yet.)
Essentially that would allow the user to set hw_frames_ctx in
get_format, even if hw_device_ctx is set.
Pro:
- possibly less confusing - API user can start out with setting only
hw_device_ctx, and later change to overriding frame allocation it by
setting a custom hw_frames_ctx
- could help with decoders which need a hw_device_ctx on opening, but
also allow customizing frame allocation (are there any candidates?)
Contra:
- possibly more confusing - right now we clearly distinguish between
"automagic" mode, that does everything for the user by setting
hw_device_ctx, and "manual" mode, where you set hw_frames_ctx.
- there's no technical advantage to allowing this
- a hwaccel can't know whether we're in "automagic" or "manual" mode at
reconfiguration time - for example, if it keeps the pool allocation
across get_format calls for efficiency reasons, and the user sets a
custom pool, you'd have two pools allocated at the same time, which
could be bad.
(I think I missed that aspect in my review for patch 2/4)
- we could always allow this later by extending the API, but adding it
now and disallowing it later would be an API break
So... I don't know. I'm hoping for a 3rd opinion by elenril.
On a side note, how will setting hw_device_ctx work for hwaccels again?
Does the user still need to override get_format? Should the user need
to do that? It seems pretty redundant.
>
>
> doc/APIchanges | 3 +++
> libavcodec/avcodec.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
> libavcodec/utils.c | 1 +
> libavcodec/version.h | 2 +-
> 4 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index c8c2a219f..4de20a09e 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -13,6 +13,9 @@ libavutil: 2015-08-28
>
> API changes, most recent first:
>
> +2017-xx-xx - xxxxxxx - lavc 57.34.0 - avcodec.h
> + Add AVCodecContext.hw_device_ctx.
> +
> 2016-xx-xx - xxxxxxx - lavc 57.31.0 - avcodec.h
> Add AVCodecContext.apply_cropping to control whether cropping
> is handled by libavcodec or the caller.
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 18721561d..f3d07be5c 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3092,8 +3092,14 @@ typedef struct AVCodecContext {
>
> /**
> * A reference to the AVHWFramesContext describing the input (for
> encoding)
> - * or output (decoding) frames. The reference is set by the caller and
> - * afterwards owned (and freed) by libavcodec.
> + * or output (for decoding) frames which will be used by a hardware
> codec.
> + *
> + * It can be set by the user if they wish to supply the frames used, or
> it
> + * can be set automatically by libavcodec for decoding when get_format()
> + * indicates that it should output hardware frames. If supplied by the
> + * user, the reference is thereafter owned (and freed) by libavcodec.
> + *
> + * If set by the user:
> *
> * - decoding: This field should be set by the caller from the
> get_format()
> * callback. The previous reference (if any) will always be
> @@ -3110,6 +3116,20 @@ typedef struct AVCodecContext {
> * AVCodecContext.pix_fmt.
> *
> * This field should be set before avcodec_open2() is called.
> + *
> + * If set by libavcodec:
> + *
> + * - decoding: The hw_device_ctx field should be set by the user before
> + * calling avcodec_open2(). If at some later point a
> hardware
> + * pixel format is selected by get_format(), then a new
> + * AVHWFramesContext will be created by the decoder and put
> + * in this field.
> + *
> + * It can be read only inside the get_buffer2() callback
> + * (which must return a buffer from the given context).
> + *
> + * - encoding: Unused. In this case the user must not supply hardware
> + * frames to the encoder.
> */
> AVBufferRef *hw_frames_ctx;
>
> @@ -3139,6 +3159,32 @@ typedef struct AVCodecContext {
> * (with the display dimensions being determined by the crop_* fields).
> */
> int apply_cropping;
> +
> + /**
> + * A reference to the AVHWDeviceContext describing the device which will
> + * be used by a hardware encoder/decoder. The reference is set by the
> + * caller and afterwards owned (and freed) by libavcodec.
> + *
> + * This should only be used if either the codec device does not require
> + * hardware frames or any that are used are allocated internally by
> + * libavcodec. If the user ever intends to supply any of the frames used
> + * for decoding then hw_frames_ctx must be used instead.
> + *
> + * If the same device is to be used throughout the lifetime of a decoder,
> + * this field should be set before avcodec_open2() is called. Some
> + * decoders can only use one device and therefore require this behaviour.
> + *
> + * For other decoders, the hw_device_ctx field can be set inside the
> + * get_format() callback to indicate the device to use for decoding a
> + * particular section of the stream. Any previous value in the field
> + * must be unreferenced before being overwritten. This also supports
> + * changing between use of hw_device_ctx and hw_frames_ctx - when doing
> + * so, the other field must always be set to NULL.
> + *
> + * For encoders, this field must be set before avcodec_open2() is called.
> + * (Encoders can only use a single device.)
> + */
> + AVBufferRef *hw_device_ctx;
(I wonder, should we have flags to indicate which decoders support
which of those methods?)
> } AVCodecContext;
>
> /**
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 2978109a2..1d316bd03 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -801,6 +801,7 @@ av_cold int avcodec_close(AVCodecContext *avctx)
> avctx->nb_coded_side_data = 0;
>
> av_buffer_unref(&avctx->hw_frames_ctx);
> + av_buffer_unref(&avctx->hw_device_ctx);
>
> if (avctx->priv_data && avctx->codec && avctx->codec->priv_class)
> av_opt_free(avctx->priv_data);
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 2ade539c6..a6eda6a69 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
> #include "libavutil/version.h"
>
> #define LIBAVCODEC_VERSION_MAJOR 57
> -#define LIBAVCODEC_VERSION_MINOR 33
> +#define LIBAVCODEC_VERSION_MINOR 34
> #define LIBAVCODEC_VERSION_MICRO 0
>
> #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
Generally LGTM.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
