Messages in this thread | | | Subject | Re: [PATCH v3 1/2] media: v4l: Add definitions for the HEVC slice format and controls | From | Paul Kocialkowski <> | Date | Fri, 19 Apr 2019 09:22:51 +0200 |
| |
Hi,
On Tue, 2019-04-02 at 13:51 +0200, Hans Verkuil wrote: > On 2/14/19 10:53 AM, Paul Kocialkowski wrote:
[...]
> > +.. c:type:: v4l2_hevc_dpb_entry > > + > > +.. cssclass:: longtable > > + > > +.. flat-table:: struct v4l2_hevc_dpb_entry > > + :header-rows: 0 > > + :stub-columns: 0 > > + :widths: 1 1 2 > > + > > + * - __u32 > > + - ``buffer_tag`` > > + - The V4L2 buffer tag that matches the associated reference picture. > > This struct documentation is out of date with the actual struct!
Woops, sorry about that. I'll make another pass on the structures vs docs before submitting the next version.
> > +#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE 0x01 > > +#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER 0x02 > > +#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR 0x03 > > + > > +#define V4L2_HEVC_DPB_ENTRIES_NUM_MAX 16 > > + > > +struct v4l2_hevc_dpb_entry { > > + __u64 timestamp; > > I would call this 'reference_ts' rather than plain 'timestamp'. > Or at least some other name that reflects what the timestamp > refers to.
Well, I think using "ts" instead of "timestamp" is much less explicit (I'm really not a big fan of abbreviations...).
Also, this field is in the dpb entry structure and since a dpb entry describes a reference, I find it quite straightforward already without any added prefix. The prefix feels redundant with the name of the structure that holds the element.
I think the same should apply for mpeg-2 and h.264.
> > + __u8 rps; > > + __u8 field_pic; > > + __u16 pic_order_cnt[2]; > > + __u8 padding[2]; > > padding fields here or elsewhere are not documented. Do you really > need them? What if you changed rps and field_pic to __u16 instead? > > Padding fields are painful to maintain in my experience.
I think we'll need to rework the controls to include flags and such in the not-too-distant future. So there will be an occasion to change it then (the same work also needs to be done for MPEG-2).
If you prefer that we make the change now and later update it along with the controls, I can definitely do that too.
Cheers,
Paul
-- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com
| |