Re: [RFC] media: uapi: Move HEVC stateless controls out of staging

[John Cox]

On Mon, 14 Feb 2022 20:26:34 +0100, you wrote:

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

Can we tell if this control is needed from userland? There's no great
point in filling it in if the driver isn't going to use it.

>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.

num_entry_points is a useful field  (in the slice header preferably) for
the RPi hardware as whilst it doesn't need to know the offsets it does
need to construct a table with one entry per offset (for cabac state
purposes) so it needs to know how many there are. It is possible to
infer the number from the slice_segment address in the next slice header
but that involves keeping around more state from one request to the
next.

>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?

Yes slice_segment_data only occurs afer slice_segment_header (7.3.6.1)
and that ends with byte_alignment().

Regards

John Cox

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