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