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@xxxxxxxxxxxxx>
> > ---
> > 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>
> >
> >
