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

[Maxime Ripard]

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.

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?

Maxime

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

Attachment: signature.asc
Description: PGP signature