mbox series

[v2,0/9] Stateless H.264 de-staging

Message ID 20201113215121.505173-1-ezequiel@collabora.com (mailing list archive)
Headers show
Series Stateless H.264 de-staging | expand

Message

Ezequiel Garcia Nov. 13, 2020, 9:51 p.m. UTC
Now that H.264 stateless controls are solid, we can get it
out of staging.

Following some guidelines from Hans, this series creates a
new stateless control class for the stable codec controls to land.

While here, I'm including a patch from Jonas adding profiles
and levels to Rkvdec, and also made a similar fix for Cedrus.

This series was tested on a i.MX8MQ EVK board, using GStreamer:

https://gitlab.freedesktop.org/ezequielgarcia/gst-plugins-bad/-/commits/h264_stable_uapi

In case someone wants to give this a test.

Note that v4l2-compliance isn't passing, as the Hantro driver
doesn't set legal default values for the H264 SPS, PPS, etc
controls.

That's something we should fix, although it's since it's just
meant to please v4l2-compliance, we could also argue that these
controls shouldn't be expected to have any default value.

Thanks,
Ezequiel

v2:
  * Split destage changes in several patches so it's easier to review.
  * Added missing changes to drivers/media/v4l2-core/v4l2-ctrls.c.
  * Renamed V4L2_CID_CODEC_CX2341X_ and V4L2_CID_MPEG_MFC51_
  * Moved the compatibility macros for MPEG to the end of the header.

Ezequiel Garcia (8):
  media: cedrus: h264: Support profile and level controls
  media: Rename stateful codec control macros
  media: Clean stateless control includes
  media: controls: Add the stateless codec control class
  media: uapi: Move parsed H264 pixel format out of staging
  media: uapi: Move the H264 stateless control types out of staging
  media: uapi: move H264 stateless controls out of staging
  media: docs: Move the H264 stateless codec uAPI

Jonas Karlman (1):
  media: rkvdec: h264: Support profile and level controls

 .../userspace-api/media/v4l/common.rst        |   1 +
 .../userspace-api/media/v4l/dev-mem2mem.rst   |   2 +-
 .../media/v4l/ext-ctrls-codec-stateless.rst   | 674 +++++++++++++++
 .../media/v4l/ext-ctrls-codec.rst             | 696 +---------------
 .../media/v4l/extended-controls.rst           |   8 +-
 .../media/v4l/pixfmt-compressed.rst           |  14 +-
 .../media/v4l/vidioc-g-ext-ctrls.rst          |   6 +-
 drivers/media/common/cx2341x.c                |   4 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c  |   2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |   2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  45 +-
 drivers/staging/media/hantro/hantro_drv.c     |  26 +-
 drivers/staging/media/hantro/hantro_h264.c    |   8 +-
 drivers/staging/media/hantro/hantro_hw.h      |   4 +-
 drivers/staging/media/rkvdec/rkvdec-h264.c    |   8 +-
 drivers/staging/media/rkvdec/rkvdec.c         |  39 +-
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  45 +-
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  12 +-
 include/media/fwht-ctrls.h                    |   2 +-
 include/media/h264-ctrls.h                    |  27 +-
 include/media/hevc-ctrls.h                    |  10 +-
 include/media/mpeg2-ctrls.h                   |   4 +-
 include/media/v4l2-ctrls.h                    |   1 -
 include/media/v4l2-h264.h                     |   2 +-
 include/media/vp8-ctrls.h                     |   2 +-
 include/uapi/linux/v4l2-controls.h            | 788 +++++++++++++-----
 include/uapi/linux/videodev2.h                |   8 +
 27 files changed, 1422 insertions(+), 1018 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst

Comments

Hans Verkuil Nov. 14, 2020, 12:58 p.m. UTC | #1
On 13/11/2020 22:51, Ezequiel Garcia wrote:
> Now that H.264 stateless controls are solid, we can get it
> out of staging.
> 
> Following some guidelines from Hans, this series creates a
> new stateless control class for the stable codec controls to land.
> 
> While here, I'm including a patch from Jonas adding profiles
> and levels to Rkvdec, and also made a similar fix for Cedrus.
> 
> This series was tested on a i.MX8MQ EVK board, using GStreamer:
> 
> https://gitlab.freedesktop.org/ezequielgarcia/gst-plugins-bad/-/commits/h264_stable_uapi
> 
> In case someone wants to give this a test.
> 
> Note that v4l2-compliance isn't passing, as the Hantro driver
> doesn't set legal default values for the H264 SPS, PPS, etc
> controls.
> 
> That's something we should fix, although it's since it's just
> meant to please v4l2-compliance, we could also argue that these
> controls shouldn't be expected to have any default value.

This really needs to be fixed. I've ignored this issue since the API
was in staging and still changing, but when we move it out of staging,
then this issue should be tackled.

It should likely be done in std_init_compound() in v4l2-ctrls.c.

It is probably a good idea to also verify std_validate_compound(), making
sure that everything there is still valid and up to date.

And std_log() should be taught about these new controls. For compound
controls I would just log the type name, e.g. "H264_SPS" and not the
values of these compound controls.

Regards,

	Hans

> 
> Thanks,
> Ezequiel
> 
> v2:
>   * Split destage changes in several patches so it's easier to review.
>   * Added missing changes to drivers/media/v4l2-core/v4l2-ctrls.c.
>   * Renamed V4L2_CID_CODEC_CX2341X_ and V4L2_CID_MPEG_MFC51_
>   * Moved the compatibility macros for MPEG to the end of the header.
> 
> Ezequiel Garcia (8):
>   media: cedrus: h264: Support profile and level controls
>   media: Rename stateful codec control macros
>   media: Clean stateless control includes
>   media: controls: Add the stateless codec control class
>   media: uapi: Move parsed H264 pixel format out of staging
>   media: uapi: Move the H264 stateless control types out of staging
>   media: uapi: move H264 stateless controls out of staging
>   media: docs: Move the H264 stateless codec uAPI
> 
> Jonas Karlman (1):
>   media: rkvdec: h264: Support profile and level controls
> 
>  .../userspace-api/media/v4l/common.rst        |   1 +
>  .../userspace-api/media/v4l/dev-mem2mem.rst   |   2 +-
>  .../media/v4l/ext-ctrls-codec-stateless.rst   | 674 +++++++++++++++
>  .../media/v4l/ext-ctrls-codec.rst             | 696 +---------------
>  .../media/v4l/extended-controls.rst           |   8 +-
>  .../media/v4l/pixfmt-compressed.rst           |  14 +-
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |   6 +-
>  drivers/media/common/cx2341x.c                |   4 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c  |   2 +-
>  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |   2 +-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  45 +-
>  drivers/staging/media/hantro/hantro_drv.c     |  26 +-
>  drivers/staging/media/hantro/hantro_h264.c    |   8 +-
>  drivers/staging/media/hantro/hantro_hw.h      |   4 +-
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |   8 +-
>  drivers/staging/media/rkvdec/rkvdec.c         |  39 +-
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  45 +-
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  12 +-
>  include/media/fwht-ctrls.h                    |   2 +-
>  include/media/h264-ctrls.h                    |  27 +-
>  include/media/hevc-ctrls.h                    |  10 +-
>  include/media/mpeg2-ctrls.h                   |   4 +-
>  include/media/v4l2-ctrls.h                    |   1 -
>  include/media/v4l2-h264.h                     |   2 +-
>  include/media/vp8-ctrls.h                     |   2 +-
>  include/uapi/linux/v4l2-controls.h            | 788 +++++++++++++-----
>  include/uapi/linux/videodev2.h                |   8 +
>  27 files changed, 1422 insertions(+), 1018 deletions(-)
>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
>
Ezequiel Garcia Nov. 15, 2020, 12:02 a.m. UTC | #2
Hi Hans,

Thanks for the quick review.

On Sat, 2020-11-14 at 13:58 +0100, Hans Verkuil wrote:
> On 13/11/2020 22:51, Ezequiel Garcia wrote:
> > Now that H.264 stateless controls are solid, we can get it
> > out of staging.
> > 
> > Following some guidelines from Hans, this series creates a
> > new stateless control class for the stable codec controls to land.
> > 
> > While here, I'm including a patch from Jonas adding profiles
> > and levels to Rkvdec, and also made a similar fix for Cedrus.
> > 
> > This series was tested on a i.MX8MQ EVK board, using GStreamer:
> > 
> > https://gitlab.freedesktop.org/ezequielgarcia/gst-plugins-bad/-/commits/h264_stable_uapi
> > 
> > In case someone wants to give this a test.
> > 
> > Note that v4l2-compliance isn't passing, as the Hantro driver
> > doesn't set legal default values for the H264 SPS, PPS, etc
> > controls.
> > 
> > That's something we should fix, although it's since it's just
> > meant to please v4l2-compliance, we could also argue that these
> > controls shouldn't be expected to have any default value.
> 

Looking into more detail, v4l2-compliance was failing due to
checks in std_validate_compound for VP8 controls.

H264 controls are not currently validated (but sounds like
we'll want to, from below discussion).

> This really needs to be fixed. I've ignored this issue since the API
> was in staging and still changing, but when we move it out of staging,
> then this issue should be tackled.
> 

Sure.

> It should likely be done in std_init_compound() in v4l2-ctrls.c.
> 

Yes, MPEG2 controls are already initialized over there.

>   
> It is probably a good idea to also verify std_validate_compound(), making
> sure that everything there is still valid and up to date.
> 

Indeed. I'm actually more concerned about std_validate_compound,
which should do some parameter sanitization. Thanks for catching this.
 
> And std_log() should be taught about these new controls. For compound
> controls I would just log the type name, e.g. "H264_SPS" and not the
> values of these compound controls.
> 

I guess this makes some sense, if only for consistency.

Is there any other reason to have this? The codec drivers are not
implementing VIDIOC_LOG_STATUS. If we were to use v4l2_ctrl_log_status,
we'd only get something like this:

   v4l2-ctrls: hantro-vpu 38300000.video-codec: H264 Sequence Parameter Set: H264_SPS
   v4l2-ctrls: hantro-vpu 38300000.video-codec: H264 Picture Parameter Set: H264_PPS
   ...
   v4l2-ctrls: hantro-vpu 38300000.video-codec: H264 Decode Mode: Frame-Based
   v4l2-ctrls: hantro-vpu 38300000.video-codec: H264 Start Code: Annex B Start Code
   v4l2-ctrls: hantro-vpu 38300000.video-codec: H264 Profile: Main

Also...

Another thing I missed is here is driver destaging. I would really want to start
getting the drivers out of staging as well. Can we have some
VIDEO_V4L2_UNSTABLE_STATELESS option in media/staging, so we can
use that in drivers?

This way, we would not offer the staging controls and pixel formats
unless the user explicitly enables that option.

The core could also print a warning when the option is enabled,
and controls are first created.

So we'd move cedrus, hantro and rkvdec out of staging on this series
as well, supporting H264 only, by default.
 
Thanks!
Ezequiel 

> Regards,
> 
> 	Hans
> 
> > Thanks,
> > Ezequiel
> > 
> > v2:
> >   * Split destage changes in several patches so it's easier to review.
> >   * Added missing changes to drivers/media/v4l2-core/v4l2-ctrls.c.
> >   * Renamed V4L2_CID_CODEC_CX2341X_ and V4L2_CID_MPEG_MFC51_
> >   * Moved the compatibility macros for MPEG to the end of the header.
> > 
> > Ezequiel Garcia (8):
> >   media: cedrus: h264: Support profile and level controls
> >   media: Rename stateful codec control macros
> >   media: Clean stateless control includes
> >   media: controls: Add the stateless codec control class
> >   media: uapi: Move parsed H264 pixel format out of staging
> >   media: uapi: Move the H264 stateless control types out of staging
> >   media: uapi: move H264 stateless controls out of staging
> >   media: docs: Move the H264 stateless codec uAPI
> > 
> > Jonas Karlman (1):
> >   media: rkvdec: h264: Support profile and level controls
> > 
> >  .../userspace-api/media/v4l/common.rst        |   1 +
> >  .../userspace-api/media/v4l/dev-mem2mem.rst   |   2 +-
> >  .../media/v4l/ext-ctrls-codec-stateless.rst   | 674 +++++++++++++++
> >  .../media/v4l/ext-ctrls-codec.rst             | 696 +---------------
> >  .../media/v4l/extended-controls.rst           |   8 +-
> >  .../media/v4l/pixfmt-compressed.rst           |  14 +-
> >  .../media/v4l/vidioc-g-ext-ctrls.rst          |   6 +-
> >  drivers/media/common/cx2341x.c                |   4 +-
> >  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c  |   2 +-
> >  drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |   2 +-
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  45 +-
> >  drivers/staging/media/hantro/hantro_drv.c     |  26 +-
> >  drivers/staging/media/hantro/hantro_h264.c    |   8 +-
> >  drivers/staging/media/hantro/hantro_hw.h      |   4 +-
> >  drivers/staging/media/rkvdec/rkvdec-h264.c    |   8 +-
> >  drivers/staging/media/rkvdec/rkvdec.c         |  39 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus.c   |  45 +-
> >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  12 +-
> >  include/media/fwht-ctrls.h                    |   2 +-
> >  include/media/h264-ctrls.h                    |  27 +-
> >  include/media/hevc-ctrls.h                    |  10 +-
> >  include/media/mpeg2-ctrls.h                   |   4 +-
> >  include/media/v4l2-ctrls.h                    |   1 -
> >  include/media/v4l2-h264.h                     |   2 +-
> >  include/media/vp8-ctrls.h                     |   2 +-
> >  include/uapi/linux/v4l2-controls.h            | 788 +++++++++++++-----
> >  include/uapi/linux/videodev2.h                |   8 +
> >  27 files changed, 1422 insertions(+), 1018 deletions(-)
> >  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-codec-stateless.rst
> >