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

[Hans Verkuil <hverkuil@xs4all.nl>]

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
>