Re: [linux-sunxi] [PATCH v2 1/2] media: v4l: Add definitions for the HEVC slice format and controls

[Paul Kocialkowski]

Hi,

On Tue, 2019-01-08 at 18:00 +0800, Ayaka wrote:
> 
> Sent from my iPad
> 
> > On Jan 8, 2019, at 4:38 PM, Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> wrote:
> > 
> > Hi,
> > 
> > > On Tue, 2019-01-08 at 09:16 +0800, Ayaka wrote:
> > > 
> > > Sent from my iPad
> > > 
> > > > On Jan 7, 2019, at 5:57 PM, Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > > > On Mon, 2019-01-07 at 11:49 +0800, Randy Li wrote:
> > > > > > On 12/12/18 8:51 PM, Paul Kocialkowski wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Wed, 2018-12-05 at 21:59 +0100, Jernej Åkrabec wrote:
> > > > > > 
> > > > > > > > +
> > > > > > > > +#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 {
> > > > > > > > +    __u32    buffer_tag;
> > > > > > > > +    __u8    rps;
> > > > > > > > +    __u8    field_pic;
> > > > > > > > +    __u16    pic_order_cnt[2];
> > > > > > > > +};
> > > > > 
> > > > > Please add a property for reference index, if that rps is not used for 
> > > > > this, some device would request that(not the rockchip one). And 
> > > > > Rockchip's VDPU1 and VDPU2 for AVC would request a similar property.
> > > > 
> > > > What exactly is that reference index? Is it a bitstream element or
> > > > something deduced from the bitstream?
> > > > 
> > > picture order count(POC) for HEVC and frame_num in AVC. I think it is
> > > the number used in list0(P slice and B slice) and list1(B slice).
> > 
> > The picture order count is already the last field of the DPB entry
> > structure. There is one for each field picture.
> As we are not sure whether there is a field coded slice or CTU, I
> would hold this part and else about the field.

I'm not sure what you meant here, sorry.

> > > > > Adding another buffer_tag for referring the memory of the motion vectors 
> > > > > for each frames. Or a better method is add a meta data to echo picture 
> > > > > buffer,  since the picture output is just the same as the original, 
> > > > > display won't care whether the motion vectors are written the button of 
> > > > > picture or somewhere else.
> > > > 
> > > > The motion vectors are passed as part of the raw bitstream data, in the
> > > > slices. Is there a case where the motion vectors are coded differently?
> > > No, it is an additional cache for decoder, even FFmpeg having such
> > > data, I think allwinner must output it into somewhere.
> > 
> > Ah yes I see what you mean! This is handled internally by our driver
> > and not exposed to userspace. I don't think it would be a good idea to
> > expose this cache or request that userspace allocates it like a video
> > buffer.
> > 
> No, usually the driver should allocate, as the user space have no
> idea on size of each devices.
> But for advantage user, application can fix a broken picture with a
> proper data or analysis a object motion from that.
> So I would suggest attaching this information to a picture buffer as
> a meta data. 

Right, the driver will allocate chunks of memory for the decoding
metadata used by the hardware decoder.

Well, I don't think V4L2 has any mechanism to expose this data for now
and since it's very specific to the hardware implementation, I guess
the interest in having that is generally pretty low.

That's maybe something that could be added later if someone wants to
work on it, but I think we are better off keeping this metadata hidden
by the driver for now.

> > > > > > > > +
> > > > > > > > +struct v4l2_hevc_pred_weight_table {
> > > > > > > > +    __u8    luma_log2_weight_denom;
> > > > > > > > +    __s8    delta_chroma_log2_weight_denom;
> > > > > > > > +
> > > > > > > > +    __s8    delta_luma_weight_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +    __s8    luma_offset_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +    __s8    delta_chroma_weight_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
> > > > > > > > +    __s8    chroma_offset_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
> > > > > > > > +
> > > > > > > > +    __s8    delta_luma_weight_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +    __s8    luma_offset_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +    __s8    delta_chroma_weight_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
> > > > > > > > +    __s8    chroma_offset_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
> > > > > > > > +};
> > > > > > > > +
> > > > > Those properties I think are not necessary are applying for the 
> > > > > Rockchip's device, may not work for the others.
> > > > 
> > > > Yes, it's possible that some of the elements are not necessary for some
> > > > decoders. What we want is to cover all the elements that might be
> > > > required for a decoder.
> > > I wonder whether allwinner need that, those sao flag usually ignored
> > > by decoder in design. But more is better than less, it is hard to
> > > extend a v4l2 structure  in the future, maybe a new HEVC profile
> > > would bring a new property, it is still too early for HEVC.
> > 
> > Yes this is used by our decoder. The idea is to have all the basic
> > bitstream elements in the structures (even if some decoders don't use
> > them all) and add others for extension as separate controls later.
> > 
> > > > > > > > +struct v4l2_ctrl_hevc_slice_params {
> > > > > > > > +    __u32    bit_size;
> > > > > > > > +    __u32    data_bit_offset;
> > > > > > > > +
> > > > > > > > +    /* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
> > > > > > > > +    __u8    nal_unit_type;
> > > > > > > > +    __u8    nuh_temporal_id_plus1;
> > > > > > > > +
> > > > > > > > +    /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
> > > > > > > > +    __u8    slice_type;
> > > > > > > > +    __u8    colour_plane_id;
> > > > > ----------------------------------------------------------------------------
> > > > > > > > +    __u16    slice_pic_order_cnt;
> > > > > > > > +    __u8    slice_sao_luma_flag;
> > > > > > > > +    __u8    slice_sao_chroma_flag;
> > > > > > > > +    __u8    slice_temporal_mvp_enabled_flag;
> > > > > > > > +    __u8    num_ref_idx_l0_active_minus1;
> > > > > > > > +    __u8    num_ref_idx_l1_active_minus1;
> > > > > Rockchip's decoder doesn't use this part.
> > > > > > > > +    __u8    mvd_l1_zero_flag;
> > > > > > > > +    __u8    cabac_init_flag;
> > > > > > > > +    __u8    collocated_from_l0_flag;
> > > > > > > > +    __u8    collocated_ref_idx;
> > > > > > > > +    __u8    five_minus_max_num_merge_cand;
> > > > > > > > +    __u8    use_integer_mv_flag;
> > > > > > > > +    __s8    slice_qp_delta;
> > > > > > > > +    __s8    slice_cb_qp_offset;
> > > > > > > > +    __s8    slice_cr_qp_offset;
> > > > > > > > +    __s8    slice_act_y_qp_offset;
> > > > > > > > +    __s8    slice_act_cb_qp_offset;
> > > > > > > > +    __s8    slice_act_cr_qp_offset;
> > > > > > > > +    __u8    slice_deblocking_filter_disabled_flag;
> > > > > > > > +    __s8    slice_beta_offset_div2;
> > > > > > > > +    __s8    slice_tc_offset_div2;
> > > > > > > > +    __u8    slice_loop_filter_across_slices_enabled_flag;
> > > > > > > > +
> > > > > > > > +    /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture timing SEI message */
> > > > > > > > +    __u8    pic_struct;
> > > > > I think the decoder doesn't care about this, it is used for display.
> > > > 
> > > > The purpose of this field is to indicate whether the current picture is
> > > > a progressive frame or an interlaced field picture, which is useful for
> > > > decoding.
> > > > 
> > > > At least our decoder has a register field to indicate frame/top
> > > > field/bottom field, so we certainly need to keep the info around.
> > > > Looking at the spec and the ffmpeg implementation, it looks like this
> > > > flag of the bitstream is the usual way to report field coding.
> > > It depends whether the decoder cares about scan type or more, I
> > > wonder prefer general_interlaced_source_flag for just scan type, it
> > > would be better than reading another SEL.
> > 
> > Well we still need a way to indicate if the current data is top or
> > bottom field for interlaced. I don't think that knowing that the whole
> > video is interlaced would be precise enough.
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > > > > > > +
> > > > > > > > +    /* 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];
> > > > > > > > +    __u8    num_active_dpb_entries;
> > > > > > > > +    __u8    ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +    __u8    ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> > > > > > > > +
> > > > > > > > +    __u8    num_rps_poc_st_curr_before;
> > > > > > > > +    __u8    num_rps_poc_st_curr_after;
> > > > > > > > +    __u8    num_rps_poc_lt_curr;
> > > > > > > > +
> > > > > > > > +    /* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
> > > > > > > > +    struct v4l2_hevc_pred_weight_table pred_weight_table;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > #endif
> > > > -- 
> > > > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > > > Embedded Linux and kernel engineering
> > > > https://bootlin.com
> > > > 
> > -- 
> > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > Embedded Linux and kernel engineering
> > https://bootlin.com
> > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com