Message-ID: <2302767.NG923GbCHz@kista>
Date:   Mon, 14 Feb 2022 20:26:34 +0100
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     mchehab@...nel.org, ezequiel@...guardiasur.com.ar,
        p.zabel@...gutronix.de, gregkh@...uxfoundation.org,
        mripard@...nel.org, paul.kocialkowski@...tlin.com, wens@...e.org,
        hverkuil-cisco@...all.nl, jonas@...boo.se, nicolas@...fresne.ca,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        kernel@...labora.com, Alex Bee <knaerzche@...il.com>,
        jc@...esim.co.uk
Subject: Re: Re: [RFC] media: uapi: Move HEVC stateless controls out of staging

Dne ponedeljek, 14. februar 2022 ob 18:25:01 CET je Benjamin Gaignard 
napisal(a):
> 
> Le 13/02/2022 à 12:33, Jernej Škrabec a écrit :
> > Hi Benjamin,
> >
> > CC: Alex, John
> >
> > Sorry for late response, but I've been very busy last week.
> >
> > First of all, thank you for doing this! It's about time that HEVC moves
> > forward.
> >
> > Dne torek, 01. februar 2022 ob 13:34:39 CET je Benjamin Gaignard 
napisal(a):
> >> The HEVC stateless 'uAPI' was staging and marked explicitly in the
> >> V4L2 specification that it will change and is unstable.
> >>
> >> Note that these control IDs were never exported as a public API,
> >> they were only defined in kernel-local headers (hevc-ctrls.h).
> >>
> >> While moving the controls out of staging they are renamed and
> >> control IDs get new numbers.
> >> Drivers (Hantro, Cedrus) and Documentation are updated accordaly.
> > accordaly -> accordingly
> >
> >> Additional structures fields has been added for RKVDEC driver usage.
> > You should do separate patch for that, preceding this one. One patch 
should
> > only do one thing.
> 
> I will do that in v2
> 
> >
> > I also suggest that you add additional patch for removing bit_size field in
> > struct v4l2_ctrl_hevc_slice_params. Similar fields were already removed 
from
> > MPEG2 and H264 structures. Bit size can be deduced from output buffer size 
and
> > it doesn't hurt if bit size in Cedrus is set to bigger value than actual 
slice
> > bit size.
> 
> ok
> 
> >
> >> Hantro dedicated control is moving to hantro-media.h
> >> Since hevc-ctrls.h content has been dispatched in others file, remove it.
> >>
> >> fluster tests results on IMX8MQ is 77/147 for HEVC codec.
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> > Note that Cedrus still needs additional information in order to decode 
some
> > HEVC videos. Missing info is num_entry_point_offsets and list of all
> > entry_point_offset_minus1 (obviously, num_entry_point_offsets in size).
> >
> > I suggest that this is represented in a new control, which would use 
dynamic
> > array feature, written by Hans. While Cedrus supports max. 256 entries, it 
can
> > be much bigger in theory, but in reality, it's much smaller (like 4-8
> > entries).
> 
> I haven't seen yet any user for these fields but I will create a new control 
like
> #define V4L2_CID_STATELESS_HEVC_ENTRY_POINT	(V4L2_CID_CODEC_STATELESS_BASE + 
407)
> 
> struct v4l2_ctrl_hevc_entry_point_offset {
> 	__u32	entry_point_offset_minus1;
> };

Yeah, Cedrus is currently the only mainline driver that needs that in order to 
fully work. I think John used num_entry_point_offsets in his (out of tree) RPi 
HEVC decoding driver too.

Wouldn't be easier to just use u32 directly? This is just array of numbers, so 
nothing else will be added in that struct...

Anyway, once you add this, I'll quickly update driver to take advantage of it.

> 
> and add it in the documentation:
> ``V4L2_CID_STATELESS_HEVC_ENTRY_POINT (struct)``
>      Specifies the i-th entry point offset in bytes and is represented by
>      offset_len_minus1 plus 1 bits.
>      This control is a dynamically sized array. The number of entry point
>      offsets is reported by the ``elems`` field.
>      This bitstream parameter is defined according to :ref:`hevc`.
>      They are described in section 7.4.7.1 "General slice segment header
>      semantics" of the specification.
> 
> >
> > Last but not least, data_bit_offset should be better defined. Currently it
> > points right after last header bit, just like Cedrus needs it. However, 
there
> > is padding after that, at least 1 bit and 8 bits at most, so slice data 
always
> > starts from byte aligned address. It probably make sense to rework that 
field
> > to be byte offset, not bit, just like in VA-API. Note that RPi HEVC driver 
also
> > uses byte aligned address directly. Cedrus would need some kind of 
workaround
> > and only one that works is this one:
> > https://github.com/bootlin/libva-v4l2-request/blob/master/src/h265.c#L191-L209
> 
> If Cedrus driver is happy with this definition I will keep it like that.
> When providing offset in bit is more accurate and any driver can align the 
value
> if needed, the reverse (byte -> bit) isn't possible.

If I'm not mistaken, HEVC standard actually requires that slice data starts at 
byte aligned address, so nothing would be lost for correctness of uAPI.

I already had this discussion with John and IIRC conclusion was to have byte 
aligned value here. John, can you please confirm if my interpretation is 
correct?

Best regards,
Jernej

> 
> Regards,
> Benjamin
> 
> >
> > Best regards,
> > Jernej
> >
> >
> 

Powered by blists - more mailing lists