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

[Jernej Åkrabec]

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