lkml.org 
[lkml]   [2022]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 2/2] media: staging: tegra-vde: Support V4L stateless video decoder API
From
Date
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

\
 
 \ /
  Last update: 2022-01-12 20:05    [W:3.143 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site