by Jernej Skrabec

[permalink] [raw]

Subject: Re: Re: [PATCH v2 2/9] media: cedrus: h264: Support profile and level controls

Dne torek, 17. november 2020 ob 20:40:09 CET je Ezequiel Garcia napisal(a):
> On Tue, 2020-11-17 at 20:24 +0100, Jernej Škrabec wrote:
> > Hi Ezequiel,
> >
> > sorry for late review.
> >
> > First of all, this patch doesn't break anything. However, see comment
below.
> >
> > Dne petek, 13. november 2020 ob 22:51:14 CET je Ezequiel Garcia
napisal(a):
> > > Cedrus supports H.264 profiles from Baseline to High,
> > > up to Level 5.1, except for the Extended profile
> > >
> > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and
> > > V4L2_CID_MPEG_VIDEO_H264_LEVEL so that userspace can
> > > query the driver for the supported profiles and levels.
> > >
> > > Signed-off-by: Ezequiel Garcia <[email protected]>
> > > ---
> > > drivers/staging/media/sunxi/cedrus/cedrus.c | 21 +++++++++++++++++++++
> > > 1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/
staging/
> > media/sunxi/cedrus/cedrus.c
> > > index 9a102b7c1bb9..8b0e97752d27 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > @@ -103,6 +103,27 @@ static const struct cedrus_control
cedrus_controls[] =
> > {
> > > .codec = CEDRUS_CODEC_H264,
> > > .required = false,
> > > },
> > > + {
> > > + .cfg = {
> > > + .id =
> > V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > + .min =
> > V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
> > > + .def =
> > V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> > > + .max =
> > V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > + .menu_skip_mask =
> > > +
> > BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > + },
> > > + .codec = CEDRUS_CODEC_H264,
> > > + .required = false,
> > > + },
> > > + {
> > > + .cfg = {
> > > + .id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> > > + .min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
> > > + .max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
> >
> > I went through several datasheets and only newer ones (H6, H616) state
max.
> > supported level, which is 4.2. Please change it in next revision.
> >
> > After that, you can add
> > Reviewed-by: Jernej Skrabec <[email protected]>
> >
>
> Note that I used level 5.1 based on a commit from you:
> """
> media: cedrus: h264: Fix 4K decoding on H6
>
> Due to unknown reason, H6 needs larger intraprediction buffer for 4K
> videos than other SoCs. This was discovered by playing 4096x2304 video,
> which is maximum what H6 VPU is supposed to support.
> """
>
> I guessed this meant it supported level 5 or higher.
> (Now that I think about it, I meant at least H6, does).
>
> According to https://en.wikipedia.org/wiki/Advanced_Video_Coding#Levels,
> level 4.2 is up to 2,048×1,[email protected].

Strange, then I guess datasheet is wrong (wouldn't be first time).
Unfortunatelly there is no documentation for Cedrus capabilities, so
everything is either educated guess or tested on HW. Documentation for older
than H6 SoCs always mention only 1080p @ 60fps limit, even though several of
them are capable of decoding 4K H264 videos (I'm not sure about max. fps
though).

>
> Frankly, I'm open to put whatever value makes you happy.

To be honest, I'm not sure what is correct value here. It may depend on Cedrus
core variant.

Best regards,
Jernej

>
> Thanks,
> Ezequiel
>
> > Best regards,
> > Jernej
> >
> > > + },
> > > + .codec = CEDRUS_CODEC_H264,
> > > + .required = false,
> > > + },
> > > {
> > > .cfg = {
> > > .id = V4L2_CID_MPEG_VIDEO_HEVC_SPS,
> > > --
> > > 2.27.0
> > >
> > >
> >
> >
>
>
>

2020-11-17 19:58:03

by Ezequiel Garcia

[permalink] [raw]

Subject: Re: Re: [PATCH v2 2/9] media: cedrus: h264: Support profile and level controls

On Tue, 2020-11-17 at 20:55 +0100, Jernej Škrabec wrote:
> Dne torek, 17. november 2020 ob 20:40:09 CET je Ezequiel Garcia napisal(a):
> > On Tue, 2020-11-17 at 20:24 +0100, Jernej Škrabec wrote:
> > > Hi Ezequiel,
> > >
> > > sorry for late review.
> > >
> > > First of all, this patch doesn't break anything. However, see comment
> below.
> > > Dne petek, 13. november 2020 ob 22:51:14 CET je Ezequiel Garcia
> napisal(a):
> > > > Cedrus supports H.264 profiles from Baseline to High,
> > > > up to Level 5.1, except for the Extended profile
> > > >
> > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and
> > > > V4L2_CID_MPEG_VIDEO_H264_LEVEL so that userspace can
> > > > query the driver for the supported profiles and levels.
> > > >
> > > > Signed-off-by: Ezequiel Garcia <[email protected]>
> > > > ---
> > > > drivers/staging/media/sunxi/cedrus/cedrus.c | 21 +++++++++++++++++++++
> > > > 1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/
> staging/
> > > media/sunxi/cedrus/cedrus.c
> > > > index 9a102b7c1bb9..8b0e97752d27 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > > @@ -103,6 +103,27 @@ static const struct cedrus_control
> cedrus_controls[] =
> > > {
> > > > .codec = CEDRUS_CODEC_H264,
> > > > .required = false,
> > > > },
> > > > + {
> > > > + .cfg = {
> > > > + .id =
> > > V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > > + .min =
> > > V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
> > > > + .def =
> > > V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> > > > + .max =
> > > V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > > + .menu_skip_mask =
> > > > +
> > > BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > > + },
> > > > + .codec = CEDRUS_CODEC_H264,
> > > > + .required = false,
> > > > + },
> > > > + {
> > > > + .cfg = {
> > > > + .id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> > > > + .min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
> > > > + .max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
> > >
> > > I went through several datasheets and only newer ones (H6, H616) state
> max.
> > > supported level, which is 4.2. Please change it in next revision.
> > >
> > > After that, you can add
> > > Reviewed-by: Jernej Skrabec <[email protected]>
> > >
> >
> > Note that I used level 5.1 based on a commit from you:
> > """
> > media: cedrus: h264: Fix 4K decoding on H6
> >
> > Due to unknown reason, H6 needs larger intraprediction buffer for 4K
> > videos than other SoCs. This was discovered by playing 4096x2304 video,
> > which is maximum what H6 VPU is supposed to support.
> > """
> >
> > I guessed this meant it supported level 5 or higher.
> > (Now that I think about it, I meant at least H6, does).
> >
> > According to https://en.wikipedia.org/wiki/Advanced_Video_Coding#Levels,
> > level 4.2 is up to 2,048×1,[email protected].
>
> Strange, then I guess datasheet is wrong (wouldn't be first time).
> Unfortunatelly there is no documentation for Cedrus capabilities, so
> everything is either educated guess or tested on HW. Documentation for older
> than H6 SoCs always mention only 1080p @ 60fps limit, even though several of
> them are capable of decoding 4K H264 videos (I'm not sure about max. fps
> though).
>
> > Frankly, I'm open to put whatever value makes you happy.
>
> To be honest, I'm not sure what is correct value here. It may depend on Cedrus
> core variant.
>

We can drop the level information as well, and just keep the profile one.

Maximum resolution is already enforced, via TRY_FMT, anyway.

Thanks,
Ezequiel

> Best regards,
> Jernej
>
> >
> > Thanks,
> > Ezequiel
> >
> > > Best regards,
> > > Jernej
> > >
> > > > + },
> > > > + .codec = CEDRUS_CODEC_H264,
> > > > + .required = false,
> > > > + },
> > > > {
> > > > .cfg = {
> > > > .id = V4L2_CID_MPEG_VIDEO_HEVC_SPS,
> > > > --
> > > > 2.27.0
> > > >
> > > >
> >
> >
>
>