Quoting Mark Thompson (2017-01-31 01:05:21)
> 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?
Does it need to be readable by the caller? I have a feeling it might be
preferable to hide both those fields in AVCodecInternal on return from
the callbacks(), so that they are not visible by the caller.
There is no reason I can see the caller needs to read them back, and due
to delay (reordering/frame mt), its value is not even very well defined.
>
> (Not reflected below yet.)
>
>
> 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;
As per above, it seems better to me to just explicitly state the user
must not read from this field ever and its contents are not defined.
>
> @@ -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 is somewhat vague. Unreferenced by whom? The wording implies by the
caller, which contradicts the above statement that the reference is
owned by the decoder. Again, I think it'd be best to move those refs to
private hidden places and then just optionally update them with the
caller-provided values.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
