[RFC,7/7] media: uapi: make H264 stateless codec interface public
diff mbox series

Message ID 20200623182809.1375-8-ezequiel@collabora.com
State New
Headers show
Series
  • media: Clean and make H264 stateless uAPI public
Related show

Commit Message

Ezequiel Garcia June 23, 2020, 6:28 p.m. UTC
The H264 interface is now ready to be part of the official
public API.

In addition, sanitize header includes.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
 include/media/v4l2-ctrls.h                                | 3 ++-
 include/media/v4l2-h264.h                                 | 2 +-
 .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
 4 files changed, 7 insertions(+), 11 deletions(-)
 rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)

Comments

Hans Verkuil June 25, 2020, 7:52 a.m. UTC | #1
On 23/06/2020 20:28, Ezequiel Garcia wrote:
> The H264 interface is now ready to be part of the official
> public API.
> 
> In addition, sanitize header includes.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
>  include/media/v4l2-ctrls.h                                | 3 ++-
>  include/media/v4l2-h264.h                                 | 2 +-
>  .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
>  4 files changed, 7 insertions(+), 11 deletions(-)
>  rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)

This isn't quite how this should be done.

The V4L2_PIX_FMT_H264_SLICE define and the V4L2_CTRL_TYPE_H264_* defines should
move to videodev2.h.

The remaining CID defines and the data structures should be moved to v4l2-controls.h.

Yes, I know, v4l2-controls.h is getting large. At some point (could actually be
done in a follow-up patch) the codec controls in v4l2-controls.h should be split off
into their own header (v4l2-codec-controls.h).

One more thing that I was wondering about:

#define V4L2_CID_MPEG_VIDEO_H264_SPS            (V4L2_CID_MPEG_BASE+1000)

These controls are at V4L2_CID_MPEG_BASE+1000. But I was wondering if:

1) wouldn't it be a good thing to use new CID values since this is the actual
   uAPI version? This series changes the layout of several structs, so creating
   new CID values to prevent confusion in applications might be a good idea.

2) related to 1): should we make a new control class for stateless codecs?
   Currently it is mixed in with the stateful codec controls, but I am not so
   sure that that is such a good idea. Creating a separate stateless codec
   control class would be a clean separation of stateful and stateless, and it
   would probably improve the documentation as well.

   The only 'standard' codec control that is used by stateless codecs is
   V4L2_CID_MPEG_VIDEO_H264_PROFILE in hantro, although it is not clear to me
   how it is used. It looks like it is just to report the supported profiles?
   But it isn't present in the cedrus driver, so it's a bit odd.

Thank you for working on finalizing the H264 API.

Regards,

	Hans

> 
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index 4053d8710e04..48d5be144319 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -11,9 +11,8 @@
>  
>  #include <linux/interrupt.h>
>  #include <linux/v4l2-controls.h>
> -#include <media/h264-ctrls.h>
> -#include <media/mpeg2-ctrls.h>
> -#include <media/vp8-ctrls.h>
> +
> +#include <media/v4l2-ctrls.h>
>  #include <media/videobuf2-core.h>
>  
>  #define DEC_8190_ALIGN_MASK	0x07U
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f40e2cbb21d3..fc725ba2ebd8 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -13,13 +13,14 @@
>  #include <linux/videodev2.h>
>  #include <media/media-request.h>
>  
> +#include <linux/v4l2-h264-ctrls.h>
> +
>  /*
>   * Include the stateless codec compound control definitions.
>   * This will move to the public headers once this API is fully stable.
>   */
>  #include <media/mpeg2-ctrls.h>
>  #include <media/fwht-ctrls.h>
> -#include <media/h264-ctrls.h>
>  #include <media/vp8-ctrls.h>
>  #include <media/hevc-ctrls.h>
>  
> diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> index f08ba181263d..d2314f4d4490 100644
> --- a/include/media/v4l2-h264.h
> +++ b/include/media/v4l2-h264.h
> @@ -10,7 +10,7 @@
>  #ifndef _MEDIA_V4L2_H264_H
>  #define _MEDIA_V4L2_H264_H
>  
> -#include <media/h264-ctrls.h>
> +#include <media/v4l2-ctrls.h>
>  
>  /**
>   * struct v4l2_h264_reflist_builder - Reference list builder object
> diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
> similarity index 96%
> rename from include/media/h264-ctrls.h
> rename to include/uapi/linux/v4l2-h264-ctrls.h
> index 6446ec9f283d..a06f60670d68 100644
> --- a/include/media/h264-ctrls.h
> +++ b/include/uapi/linux/v4l2-h264-ctrls.h
> @@ -2,14 +2,10 @@
>  /*
>   * These are the H.264 state controls for use with stateless H.264
>   * codec drivers.
> - *
> - * It turns out that these structs are not stable yet and will undergo
> - * more changes. So keep them private until they are stable and ready to
> - * become part of the official public API.
>   */
>  
> -#ifndef _H264_CTRLS_H_
> -#define _H264_CTRLS_H_
> +#ifndef __LINUX_V4L2_H264_CONTROLS_H
> +#define __LINUX_V4L2_H264_CONTROLS_H
>  
>  #include <linux/videodev2.h>
>  
>
Ezequiel Garcia June 25, 2020, 5:51 p.m. UTC | #2
On Thu, 2020-06-25 at 09:52 +0200, Hans Verkuil wrote:
> On 23/06/2020 20:28, Ezequiel Garcia wrote:
> > The H264 interface is now ready to be part of the official
> > public API.
> > 
> > In addition, sanitize header includes.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/staging/media/hantro/hantro_hw.h                  | 5 ++---
> >  include/media/v4l2-ctrls.h                                | 3 ++-
> >  include/media/v4l2-h264.h                                 | 2 +-
> >  .../{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h}  | 8 ++------
> >  4 files changed, 7 insertions(+), 11 deletions(-)
> >  rename include/{media/h264-ctrls.h => uapi/linux/v4l2-h264-ctrls.h} (96%)
> 
> This isn't quite how this should be done.
> 
> The V4L2_PIX_FMT_H264_SLICE define and the V4L2_CTRL_TYPE_H264_* defines should
> move to videodev2.h.
> 

OK.

> The remaining CID defines and the data structures should be moved to v4l2-controls.h.
> 

OK.

> Yes, I know, v4l2-controls.h is getting large. At some point (could actually be
> done in a follow-up patch) the codec controls in v4l2-controls.h should be split off
> into their own header (v4l2-codec-controls.h).
> 

OK, that makes sense.

> One more thing that I was wondering about:
> 
> #define V4L2_CID_MPEG_VIDEO_H264_SPS            (V4L2_CID_MPEG_BASE+1000)
> 
> These controls are at V4L2_CID_MPEG_BASE+1000. But I was wondering if:
> 
> 1) wouldn't it be a good thing to use new CID values since this is the actual
>    uAPI version? This series changes the layout of several structs, so creating
>    new CID values to prevent confusion in applications might be a good idea.
> 
> 2) related to 1): should we make a new control class for stateless codecs?
>    Currently it is mixed in with the stateful codec controls, but I am not so
>    sure that that is such a good idea. Creating a separate stateless codec
>    control class would be a clean separation of stateful and stateless, and it
>    would probably improve the documentation as well.
> 

Good idea.

>    The only 'standard' codec control that is used by stateless codecs is
>    V4L2_CID_MPEG_VIDEO_H264_PROFILE in hantro, although it is not clear to me
>    how it is used. It looks like it is just to report the supported profiles?
>    But it isn't present in the cedrus driver, so it's a bit odd.
> 

I can take a look and add profiles to cedrus as well.

> Thank you for working on finalizing the H264 API.
> 

Thanks for the guidelines and feedback.

I will drop this patch for now, since I think we
now have a clear guideline on how to go public
(which was the goal of this RFC!).

I will move forward the H264 uAPI changes,
and then we can try to push H264, MPEG-2 and VP8
public, as these interfaces are the ones
that seem stable.

Thanks!
Ezequiel  

> Regards,
> 
> 	Hans
> 
> > diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> > index 4053d8710e04..48d5be144319 100644
> > --- a/drivers/staging/media/hantro/hantro_hw.h
> > +++ b/drivers/staging/media/hantro/hantro_hw.h
> > @@ -11,9 +11,8 @@
> >  
> >  #include <linux/interrupt.h>
> >  #include <linux/v4l2-controls.h>
> > -#include <media/h264-ctrls.h>
> > -#include <media/mpeg2-ctrls.h>
> > -#include <media/vp8-ctrls.h>
> > +
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/videobuf2-core.h>
> >  
> >  #define DEC_8190_ALIGN_MASK	0x07U
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index f40e2cbb21d3..fc725ba2ebd8 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -13,13 +13,14 @@
> >  #include <linux/videodev2.h>
> >  #include <media/media-request.h>
> >  
> > +#include <linux/v4l2-h264-ctrls.h>
> > +
> >  /*
> >   * Include the stateless codec compound control definitions.
> >   * This will move to the public headers once this API is fully stable.
> >   */
> >  #include <media/mpeg2-ctrls.h>
> >  #include <media/fwht-ctrls.h>
> > -#include <media/h264-ctrls.h>
> >  #include <media/vp8-ctrls.h>
> >  #include <media/hevc-ctrls.h>
> >  
> > diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
> > index f08ba181263d..d2314f4d4490 100644
> > --- a/include/media/v4l2-h264.h
> > +++ b/include/media/v4l2-h264.h
> > @@ -10,7 +10,7 @@
> >  #ifndef _MEDIA_V4L2_H264_H
> >  #define _MEDIA_V4L2_H264_H
> >  
> > -#include <media/h264-ctrls.h>
> > +#include <media/v4l2-ctrls.h>
> >  
> >  /**
> >   * struct v4l2_h264_reflist_builder - Reference list builder object
> > diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
> > similarity index 96%
> > rename from include/media/h264-ctrls.h
> > rename to include/uapi/linux/v4l2-h264-ctrls.h
> > index 6446ec9f283d..a06f60670d68 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/uapi/linux/v4l2-h264-ctrls.h
> > @@ -2,14 +2,10 @@
> >  /*
> >   * These are the H.264 state controls for use with stateless H.264
> >   * codec drivers.
> > - *
> > - * It turns out that these structs are not stable yet and will undergo
> > - * more changes. So keep them private until they are stable and ready to
> > - * become part of the official public API.
> >   */
> >  
> > -#ifndef _H264_CTRLS_H_
> > -#define _H264_CTRLS_H_
> > +#ifndef __LINUX_V4L2_H264_CONTROLS_H
> > +#define __LINUX_V4L2_H264_CONTROLS_H
> >  
> >  #include <linux/videodev2.h>
> >  
> >

Patch
diff mbox series

diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index 4053d8710e04..48d5be144319 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -11,9 +11,8 @@ 
 
 #include <linux/interrupt.h>
 #include <linux/v4l2-controls.h>
-#include <media/h264-ctrls.h>
-#include <media/mpeg2-ctrls.h>
-#include <media/vp8-ctrls.h>
+
+#include <media/v4l2-ctrls.h>
 #include <media/videobuf2-core.h>
 
 #define DEC_8190_ALIGN_MASK	0x07U
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f40e2cbb21d3..fc725ba2ebd8 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -13,13 +13,14 @@ 
 #include <linux/videodev2.h>
 #include <media/media-request.h>
 
+#include <linux/v4l2-h264-ctrls.h>
+
 /*
  * Include the stateless codec compound control definitions.
  * This will move to the public headers once this API is fully stable.
  */
 #include <media/mpeg2-ctrls.h>
 #include <media/fwht-ctrls.h>
-#include <media/h264-ctrls.h>
 #include <media/vp8-ctrls.h>
 #include <media/hevc-ctrls.h>
 
diff --git a/include/media/v4l2-h264.h b/include/media/v4l2-h264.h
index f08ba181263d..d2314f4d4490 100644
--- a/include/media/v4l2-h264.h
+++ b/include/media/v4l2-h264.h
@@ -10,7 +10,7 @@ 
 #ifndef _MEDIA_V4L2_H264_H
 #define _MEDIA_V4L2_H264_H
 
-#include <media/h264-ctrls.h>
+#include <media/v4l2-ctrls.h>
 
 /**
  * struct v4l2_h264_reflist_builder - Reference list builder object
diff --git a/include/media/h264-ctrls.h b/include/uapi/linux/v4l2-h264-ctrls.h
similarity index 96%
rename from include/media/h264-ctrls.h
rename to include/uapi/linux/v4l2-h264-ctrls.h
index 6446ec9f283d..a06f60670d68 100644
--- a/include/media/h264-ctrls.h
+++ b/include/uapi/linux/v4l2-h264-ctrls.h
@@ -2,14 +2,10 @@ 
 /*
  * These are the H.264 state controls for use with stateless H.264
  * codec drivers.
- *
- * It turns out that these structs are not stable yet and will undergo
- * more changes. So keep them private until they are stable and ready to
- * become part of the official public API.
  */
 
-#ifndef _H264_CTRLS_H_
-#define _H264_CTRLS_H_
+#ifndef __LINUX_V4L2_H264_CONTROLS_H
+#define __LINUX_V4L2_H264_CONTROLS_H
 
 #include <linux/videodev2.h>