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
