Re: [PATCH v1 2/2] media: staging: tegra-vde: Support V4L stateless video decoder API

[Dmitry Osipenko <digetx@gmail.com>]

12.01.2022 19:49, Nicolas Dufresne пишет:
> Le mercredi 12 janvier 2022 à 18:39 +0300, Dmitry Osipenko a écrit :
>> Expose Tegra video decoder as a generic V4L M2M stateless video decoder.
> 
> Thanks for working on this. Note that it would be nice to provide V4L2
> compliance test result, and if this is actually possible, provide fluster
> conformance results using ffmpeg, gstreamer, chromium or your own decoder,
> though if its your own, it would be nice to share a bit more so we can check
> that it's not interpreting the uAPI differently from other (we'd like drivers to
> work on multiple userland ideally).

Thank you for taking a look at this patch. Now I recalled that wanted to
run V4L2 compliance test, but forgot to do that.

I'll take a look at fluster, it's new to me.

> As usual I leave to other doing proper review, I added a comment below, pointing
> out the presence of a bitstream parser in this driver, and suggested an
> amendment to the spec to get rid of this. For me the code looks otherwise quite
> straight forward, is there any known issue that would keep this driver in
> staging ?

V4L decoding works on par with the legacy custom UAPI used by this
driver. I wish the hardware spec was made public, so we could support
more complex streams, but since it's not going to happen, I think
nothing keeps this driver in staging. It's working good for what is
supported.

> Please see below ...
> 
...
>> +	slice_type = tegra_h264_parse_slice_type(bitstream + bitstream_offset,
>> +						 bitstream_size);
> 
> Oh, this is a bit unfortunate, we didn't expect frame based decoder to ever need
> the slice_type (only available to slice based decoders). I've lookahead and
> notice a bitstream parsing, with emulation byte handling and Golum code. I
> expect to see maintainers concerns with doing this, the goals of the interface
> was to avoid parsing in kernel space (security in mind).

Initially I patched GStreamer to perform the flagging and it worked
okay. GStreamer even has variable for that, which is unused by the code
[1]. But in the end I decided that a universal solution will be a better
option.

[1]
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-bad/gst-libs/gst/codecs/gsth264picture.h#L119

> If so, I may suggest to drop this fallback, and propose an amendment to the
> spec, we can require flagging KEYFRAME/PFRAME/BFRAME on the OUTPUT buffer, this
> won't break any drivers/userland on other HW, and will benefit possibly other HW
> in the future. I can volunteer to patch GStreamer and LibreELEC ffmpeg if we
> agree to this. Not sure how it works for Chromium, or if it actually make sense
> to support here.
> 
> (expecting feedback from Hans and Ezequiel here)

Amending the spec will be great, although it's not clear how to flag
frame that consists of slices having different types.

...
>> +static const struct v4l2_ctrl_config ctrl_cfgs[] = {
>> +	{	.id = V4L2_CID_STATELESS_H264_DECODE_PARAMS,	},
>> +	{	.id = V4L2_CID_STATELESS_H264_SPS,		},
>> +	{	.id = V4L2_CID_STATELESS_H264_PPS,		},
>> +	{
>> +		.id = V4L2_CID_STATELESS_H264_DECODE_MODE,
>> +		.min = V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
>> +		.max = V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
>> +		.def = V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
>> +	},
>> +	{
>> +		.id = V4L2_CID_STATELESS_H264_START_CODE,
>> +		.min = V4L2_STATELESS_H264_START_CODE_ANNEX_B,
>> +		.max = V4L2_STATELESS_H264_START_CODE_ANNEX_B,
>> +		.def = V4L2_STATELESS_H264_START_CODE_ANNEX_B,
>> +	},
>> +	{
>> +		.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>> +		.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
>> +		.max = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
>> +		.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> 
> No action needed, just be aware that exposing BASELINE is a small lie, since FMO
> and ASO feature are not supported in the uAPI.

Okay