Re: [PATCH v2 3/4] media: uapi: hevc: Add segment address field

From: Jernej Åkrabec
Date: Thu Jan 09 2020 - 09:46:46 EST


Hi!

Dne sreda, 08. januar 2020 ob 15:31:26 CET je Mauro Carvalho Chehab
napisal(a):
> Em Fri, 13 Dec 2019 17:04:27 +0100
>
> Jernej Skrabec <jernej.skrabec@xxxxxxxx> escreveu:
> > If HEVC frame consists of multiple slices, segment address has to be
> > known in order to properly decode it.
> >
> > Add segment address field to slice parameters.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > ---
> >
> > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 5 ++++-
> > include/media/hevc-ctrls.h | 5 ++++-
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst index
> > aab1451e54d4..5415d5babcc2 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > @@ -3975,6 +3975,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >
> > * - __u32
> >
> > - ``data_bit_offset``
> > - Offset (in bits) to the video data in the current slice data.
> >
> > + * - __u32
> > + - ``slice_segment_addr``
> > + -
> >
> > * - __u8
> >
> > - ``nal_unit_type``
> > -
> >
> > @@ -4052,7 +4055,7 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >
> > - ``num_rps_poc_lt_curr``
> > - The number of reference pictures in the long-term set.
> >
> > * - __u8
> >
> > - - ``padding[7]``
> > + - ``padding[5]``
> >
> > - Applications and drivers must set this to zero.
> >
> > * - struct :c:type:`v4l2_hevc_dpb_entry`
> >
> > - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> >
> > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> > index 1592e52c3614..3e2e32098312 100644
> > --- a/include/media/hevc-ctrls.h
> > +++ b/include/media/hevc-ctrls.h
> > @@ -167,6 +167,9 @@ struct v4l2_ctrl_hevc_slice_params {
> >
> > __u32 bit_size;
> > __u32 data_bit_offset;
> >
> > + /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header
*/
> > + __u32 slice_segment_addr;
> > +
>
> Why are you adding it in the middle of the data? This will break any
> existing userspace code that might be relying on it.
>
> Ok, I know that this header is not yet under include/uapi,and there's a
> warning on it for letting people know that it shouldn't be used anywhere.
>
> Still, people might be using it.

I did it that way to keep fields aligned. Cedrus is currently the only
mainlined driver to use this structure and userspace users are also very rare
at this time.

Anyway, not all HEVC features are supported at this time, which means that
this structure might grow. Once that happens, you'll lose binary compatibility
anyway, but source compatibility should be fine.

Best regards,
Jernej

>
> > /* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
> > __u8 nal_unit_type;
> > __u8 nuh_temporal_id_plus1;
> >
> > @@ -200,7 +203,7 @@ struct v4l2_ctrl_hevc_slice_params {
> >
> > __u8 num_rps_poc_st_curr_after;
> > __u8 num_rps_poc_lt_curr;
> >
> > - __u8 padding;
> > + __u8 padding[5];
> >
> > /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header
*/
> > struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>
> Cheers,
> Mauro