[FFmpeg-devel] [PATCH 1/4] hwcontext_drm: Add AVDRMFrameDescriptor.format field

[Mark Thompson]

On 09/05/2019 20:40, Jonas Karlman wrote:
> A AVDRMFrameDescriptor for a NV12 frame may be described in
> a single layer descriptor with multiple planes,
> 
> (AVDRMFrameDescriptor) {
>     .nb_layers = 1,
>     .layers[0] = {
>         .format           = DRM_FORMAT_NV12,
>         .nb_planes        = 2,
>         .planes[0] = {
>             .object_index = 0,
>         },
>         .planes[1] = {
>             .object_index = 0,
>         },
>     },
> }
> 
> or a multi-layer descriptor with one plane in each layer.
> 
> (AVDRMFrameDescriptor) {
>     .nb_layers = 2,
>     .layers[0] = {
>         .format           = DRM_FORMAT_R8,
>         .nb_planes        = 1,
>         .planes[0] = {
>             .object_index = 0,
>         },
>     },
>     .layers[1] = {
>         .format           = DRM_FORMAT_RG88,
>         .nb_planes        = 1,
>         .planes[0] = {
>             .object_index = 1,
>         },
>     },
> }
> 
> With a multi-layer descriptor, the frame format is missing.
> 
> Add a AVDRMFrameDescriptor.format field to remove any ambiguity of
> what frame format a multi-layer descriptor may have.
> 
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> ---
>  doc/APIchanges            | 3 +++
>  libavutil/hwcontext_drm.h | 4 ++++
>  libavutil/version.h       | 4 ++--
>  3 files changed, 9 insertions(+), 2 deletions(-)

Can you explain carefully the rationale for why the ABI change is ok, and add it to the commit message?

It looks like it might be ok (the most obvious failure mode doesn't happen because you're avoiding adding any reads of the field in FFmpeg), but I'm not entirely sure so an explicit comment would be helpful.

> diff --git a/doc/APIchanges b/doc/APIchanges
> index e75c4044ce..d9c21ec030 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2019-05-08 - xxxxxxxxxx - lavu 56.27.100 - hwcontext_drm.h
> +  Add AVDRMFrameDescriptor.format.
> +
>  2019-04-20 - 3153a6502a - lavc 58.52.100 - avcodec.h
>    Add AV_CODEC_FLAG_DROPCHANGED to allow avcodec_receive_frame to drop
>    frames whose parameters differ from first decoded frame in stream.
> diff --git a/libavutil/hwcontext_drm.h b/libavutil/hwcontext_drm.h
> index 42709f215e..0ccbd19acc 100644
> --- a/libavutil/hwcontext_drm.h
> +++ b/libavutil/hwcontext_drm.h
> @@ -147,6 +147,10 @@ typedef struct AVDRMFrameDescriptor {
>       * Array of layers in the frame.
>       */
>      AVDRMLayerDescriptor layers[AV_DRM_MAX_PLANES];
> +    /**
> +     * Format of the frame (DRM_FORMAT_*).
> +     */
> +    uint32_t format;

Probably wants a note that it need not be set, since there may not be a specific format for the whole frame.  I guess it should be DRM_FORMAT_INVALID in that case?

>  } AVDRMFrameDescriptor;
>  
>  /**
> diff --git a/libavutil/version.h b/libavutil/version.h
> index c0968de621..12b4f9fc3a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,8 +79,8 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  56
> -#define LIBAVUTIL_VERSION_MINOR  26
> -#define LIBAVUTIL_VERSION_MICRO 101
> +#define LIBAVUTIL_VERSION_MINOR  27
> +#define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>                                                 LIBAVUTIL_VERSION_MINOR, \
> 

Thanks,

- Mark