Re: [RFC] media: uapi: Move HEVC stateless controls out of staging
From: Benjamin Gaignard
Date: Tue Feb 15 2022 - 09:52:23 EST
Le 15/02/2022 à 15:37, John Cox a écrit :
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.
In v2 I have remove the structure and only keep a dynamic v4l2 control (an array
of integer). Control num_elems could be used as num_entry_points field.
Userland can test if the driver implement the control to decide to fill or not.
It is already the case in GStreamer for Hantro dedicate control.
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().
I have done a patch for that change in v2.
Regards,
Benjamin
Regards
John Cox
Best regards,
Jernej
Regards,
Benjamin
Best regards,
Jernej