Re: [PATCH v5 04/11] media: uapi: h264: Add the concept of start code

Paul Kocialkowski
2019-08-14T11:49:37Z

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

Hi,

On Wed 14 Aug 19, 10:11, Hans Verkuil wrote:
> On 8/12/19 9:35 PM, Ezequiel Garcia wrote:
> > Stateless decoders have different expectations about the
> > start code that is prepended on H264 slices. Add a
> > menu control to express the supported start code types
> > (including no start code).
> >
> > Drivers are allowed to support only one start code type,
> > but they can support both too.
> >
> > Note that this is independent of the H264 decoding mode,
> > which specifies the granularity of the decoding operations.
> > Either in frame-based or slice-based mode, this new control
> > will allow to define the start code expected on H264 slices.
> >
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes in v5:
> > * Improve specification as suggested by Hans.
> > Changes in v4:
> > * New patch.
> > ---
> > .../media/uapi/v4l/ext-ctrls-codec.rst | 33 +++++++++++++++++++
> > .../media/uapi/v4l/pixfmt-compressed.rst | 3 +-
> > drivers/media/v4l2-core/v4l2-ctrls.c | 9 +++++
> > include/media/h264-ctrls.h | 6 ++++
> > 4 files changed, 50 insertions(+), 1 deletion(-)
> >
>
> <snip>
>
> > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
> > index e6c510877f67..31555c99f64a 100644
> > --- a/include/media/h264-ctrls.h
> > +++ b/include/media/h264-ctrls.h
> > @@ -27,6 +27,7 @@
> > #define V4L2_CID_MPEG_VIDEO_H264_SLICE_PARAMS (V4L2_CID_MPEG_BASE+1003)
> > #define V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS (V4L2_CID_MPEG_BASE+1004)
> > #define V4L2_CID_MPEG_VIDEO_H264_DECODING_MODE (V4L2_CID_MPEG_BASE+1005)
> > +#define V4L2_CID_MPEG_VIDEO_H264_STARTCODE (V4L2_CID_MPEG_BASE+1006)
>
> I almost forgot: can you change this to _START_CODE? Since it is two words?

Agreed, I like it better this way too.

> Thanks!
>
> Hans
>
> >
> > /* enum v4l2_ctrl_type type values */
> > #define V4L2_CTRL_TYPE_H264_SPS 0x0110
> > @@ -41,6 +42,11 @@ enum v4l2_mpeg_video_h264_decoding_mode {
> > V4L2_MPEG_VIDEO_H264_FRAME_BASED_DECODING,
> > };
> >
> > +enum v4l2_mpeg_video_h264_start_code {
> > + V4L2_MPEG_VIDEO_H264_NO_STARTCODE,
> > + V4L2_MPEG_VIDEO_H264_ANNEX_B_STARTCODE,

Could we apply the same START_CODE renaming here too?

I was also thinking that it would be slightly more readable put like this,
with START_CODE as a prefix since it's common to both options and the name of
the enum:

- V4L2_MPEG_VIDEO_H264_START_CODE_NONE
- V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B

What do you think?

Cheers,

Paul

> > +};
> > +
> > #define V4L2_H264_SPS_CONSTRAINT_SET0_FLAG 0x01
> > #define V4L2_H264_SPS_CONSTRAINT_SET1_FLAG 0x02
> > #define V4L2_H264_SPS_CONSTRAINT_SET2_FLAG 0x04
> >
>

--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]