linux-kernel - Re: [PATCH v6 1/2] media: uapi: Add H264 low-level decoder API compound controls.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 3 Apr 2019 11:32:24 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     hans.verkuil@...co.com, acourbot@...omium.org,
        sakari.ailus@...ux.intel.com,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        tfiga@...omium.org, posciak@...omium.org,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Chen-Yu Tsai <wens@...e.org>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
        nicolas.dufresne@...labora.com, jenskuske@...il.com,
        jernej.skrabec@...il.com, jonas@...boo.se, ezequiel@...labora.com,
        linux-sunxi@...glegroups.com,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH v6 1/2] media: uapi: Add H264 low-level decoder API
 compound controls.

On 4/3/19 9:29 AM, Maxime Ripard wrote:
> Hi Hans,
> 
> On Tue, Apr 02, 2019 at 01:39:21PM +0200, Hans Verkuil wrote:
>>>> +struct v4l2_h264_dpb_entry {
>>>> +	__u64 timestamp;
>>>
>>> As mentioned above, I suggest to rename this to 'reference_ts'.
>>>
>>> But see also the discussion below.
>>>
>>>> +	__u16 frame_num;
>>>> +	__u16 pic_num;
>>>> +	/* Note that field is indicated by v4l2_buffer.field */
>>>> +	__s32 top_field_order_cnt;
>>>> +	__s32 bottom_field_order_cnt;
>>>> +	__u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
>>>> +};
>>>> +
>>>> +#define V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC	0x01
>>>> +
>>>> +struct v4l2_ctrl_h264_decode_param {
>>>> +	__u32 num_slices;
>>>> +	__u32 nal_ref_idc;
>>>> +	__u8 ref_pic_list_p0[32];
>>>> +	__u8 ref_pic_list_b0[32];
>>>> +	__u8 ref_pic_list_b1[32];
>>>> +	__s32 top_field_order_cnt;
>>>> +	__s32 bottom_field_order_cnt;
>>>> +	__u32 flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
>>>> +	struct v4l2_h264_dpb_entry dpb[16];
>>>
>>> Should the reference timestamp be included in this control? Or be
>>> separated into its own control?
>>>
>>> Reference frames do not apply to stateless encoders (the driver will
>>> have to maintain the reference frames internally). Everything else
>>> is equally valid for both stateless encoders and decoders, but only
>>> stateless decoders need to provide the reference frames.
>>>
>>> So in this case we would need to create a new control:
>>>
>>> #define V4L2_CID_MPEG_VIDEO_H264_DECODE_REF_TS	(V4L2_CID_MPEG_BASE+1005)
>>>
>>> Which is a u64 array control (16 elements for H.264).
>>>
>>> You would need to do something similar for MPEG2.
>>
>> The question is if this is worth the extra effort. An alternative is
>> to just specify that these reference timestamps are always to be zeroed
>> by stateless encoders and are unused.
>>
>> I'm undecided on this.
> 
> I'll address your other comments, thanks!
> 
> For that part, I haven't really looked at the encoder parameters yet,
> but I guess we should have other controls to be passed there as well
> (like the target level of encoding). We can probably reuse the
> existing controls for that though.
> 
> From the way it looks, our encoder needs to put some parameters (the
> level and feature flags, mostly) and will output the entire
> bitstream. I'm not really sure if it qualifies as a stateless encoder,
> but that would require far less than what we have in those controls
> already.

This sounds like a stateful encoder. A stateless encoder would rely on
userspace to mux the metadata and the compressed video into a valid
bitstream, and it doesn't seem that that's the case here.

> 
> Either way, maybe the good way forward here would be to way for an
> H264 / MPEG2 stateless encoder to come up, either ours or some other,
> to see what should we do about this. How does that sound?

I think so too.

I think splitting off the reference timestamps into a separate control
is probably overkill.

Regards,

	Hans

> 
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>