* [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag @ 2019-09-05 10:15 Philipp Zabel 2019-09-05 10:15 ` [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override Philipp Zabel ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Philipp Zabel @ 2019-09-05 10:15 UTC (permalink / raw) To: linux-media Cc: Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Tomasz Figa, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, kernel This flag tells the kernel whether the slice header contained the num_ref_idx_l[01]_active_minus1 syntax elements, or whether the num_ref_idx_l[01]_default_active_minus1 from PPS should be used instead. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ include/media/h264-ctrls.h | 1 + 2 files changed, 4 insertions(+) diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst index bc5dd8e76567..451a5b0f2a35 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` - 0x00000008 - + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE + - 0x00000010 + - Corresponds to the num_ref_idx_active_override_flag syntax element. ``Prediction Weight Table`` diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h index e877bf1d537c..dab519aea9bf 100644 --- a/include/media/h264-ctrls.h +++ b/include/media/h264-ctrls.h @@ -133,6 +133,7 @@ struct v4l2_h264_pred_weight_table { #define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD 0x02 #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED 0x04 #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH 0x08 +#define V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE 0x10 struct v4l2_ctrl_h264_slice_params { /* Size in bytes, including header */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override 2019-09-05 10:15 [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Philipp Zabel @ 2019-09-05 10:15 ` Philipp Zabel 2019-09-05 10:34 ` Tomasz Figa 2019-09-05 10:31 ` [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Tomasz Figa 2019-10-03 21:07 ` Paul Kocialkowski 2 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2019-09-05 10:15 UTC (permalink / raw) To: linux-media Cc: Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Tomasz Figa, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, kernel If the slice header had the num_ref_idx_active_override flag set, we should use the num_ref_idx_l[01]_active_minus1 fields instead of the defaults from the PPS. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/staging/media/hantro/hantro_g1_h264_dec.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c index 7ab534936843..544cf92ab62c 100644 --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c @@ -28,6 +28,8 @@ static void set_params(struct hantro_ctx *ctx) const struct v4l2_ctrl_h264_pps *pps = ctrls->pps; struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx); struct hantro_dev *vpu = ctx->dev; + unsigned char refidx0_active; + unsigned char refidx1_active; u32 reg; /* Decoder control register 0. */ @@ -101,9 +103,16 @@ static void set_params(struct hantro_ctx *ctx) vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5); /* Decoder control register 6. */ + if (sps->flags & V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE) { + refidx0_active = slices[0].num_ref_idx_l0_active_minus1 + 1; + refidx1_active = slices[0].num_ref_idx_l1_active_minus1 + 1; + } else { + refidx0_active = pps->num_ref_idx_l0_default_active_minus1 + 1; + refidx1_active = pps->num_ref_idx_l1_default_active_minus1 + 1; + } reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) | - G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) | - G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) | + G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(refidx0_active) | + G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(refidx1_active) | G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size); vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6); -- 2.20.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override 2019-09-05 10:15 ` [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override Philipp Zabel @ 2019-09-05 10:34 ` Tomasz Figa 2019-09-05 11:04 ` Philipp Zabel 0 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2019-09-05 10:34 UTC (permalink / raw) To: Philipp Zabel Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > If the slice header had the num_ref_idx_active_override flag set, we > should use the num_ref_idx_l[01]_active_minus1 fields instead of the > defaults from the PPS. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/staging/media/hantro/hantro_g1_h264_dec.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > index 7ab534936843..544cf92ab62c 100644 > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > @@ -28,6 +28,8 @@ static void set_params(struct hantro_ctx *ctx) > const struct v4l2_ctrl_h264_pps *pps = ctrls->pps; > struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx); > struct hantro_dev *vpu = ctx->dev; > + unsigned char refidx0_active; > + unsigned char refidx1_active; > u32 reg; > > /* Decoder control register 0. */ > @@ -101,9 +103,16 @@ static void set_params(struct hantro_ctx *ctx) > vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5); > > /* Decoder control register 6. */ > + if (sps->flags & V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE) { > + refidx0_active = slices[0].num_ref_idx_l0_active_minus1 + 1; > + refidx1_active = slices[0].num_ref_idx_l1_active_minus1 + 1; > + } else { > + refidx0_active = pps->num_ref_idx_l0_default_active_minus1 + 1; > + refidx1_active = pps->num_ref_idx_l1_default_active_minus1 + 1; > + } > reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) | > - G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) | > - G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) | > + G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(refidx0_active) | > + G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(refidx1_active) | > G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size); > vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6); My understanding was that Hantro G1 parses the slices itself and handles the override flag internally. Best regards, Tomasz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override 2019-09-05 10:34 ` Tomasz Figa @ 2019-09-05 11:04 ` Philipp Zabel 0 siblings, 0 replies; 12+ messages in thread From: Philipp Zabel @ 2019-09-05 11:04 UTC (permalink / raw) To: Tomasz Figa Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Thu, 2019-09-05 at 19:34 +0900, Tomasz Figa wrote: > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > If the slice header had the num_ref_idx_active_override flag set, we > > should use the num_ref_idx_l[01]_active_minus1 fields instead of the > > defaults from the PPS. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/staging/media/hantro/hantro_g1_h264_dec.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_g1_h264_dec.c b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > index 7ab534936843..544cf92ab62c 100644 > > --- a/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > +++ b/drivers/staging/media/hantro/hantro_g1_h264_dec.c > > @@ -28,6 +28,8 @@ static void set_params(struct hantro_ctx *ctx) > > const struct v4l2_ctrl_h264_pps *pps = ctrls->pps; > > struct vb2_v4l2_buffer *src_buf = hantro_get_src_buf(ctx); > > struct hantro_dev *vpu = ctx->dev; > > + unsigned char refidx0_active; > > + unsigned char refidx1_active; > > u32 reg; > > > > /* Decoder control register 0. */ > > @@ -101,9 +103,16 @@ static void set_params(struct hantro_ctx *ctx) > > vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL5); > > > > /* Decoder control register 6. */ > > + if (sps->flags & V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE) { > > + refidx0_active = slices[0].num_ref_idx_l0_active_minus1 + 1; > > + refidx1_active = slices[0].num_ref_idx_l1_active_minus1 + 1; > > + } else { > > + refidx0_active = pps->num_ref_idx_l0_default_active_minus1 + 1; > > + refidx1_active = pps->num_ref_idx_l1_default_active_minus1 + 1; > > + } > > reg = G1_REG_DEC_CTRL6_PPS_ID(slices[0].pic_parameter_set_id) | > > - G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(pps->num_ref_idx_l0_default_active_minus1 + 1) | > > - G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(pps->num_ref_idx_l1_default_active_minus1 + 1) | > > + G1_REG_DEC_CTRL6_REFIDX0_ACTIVE(refidx0_active) | > > + G1_REG_DEC_CTRL6_REFIDX1_ACTIVE(refidx1_active) | > > G1_REG_DEC_CTRL6_POC_LENGTH(slices[0].pic_order_cnt_bit_size); > > vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL6); > > My understanding was that Hantro G1 parses the slices itself and > handles the override flag internally. Oh, in that case this patch is incorrect and the first one is indeed unnecessary. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-05 10:15 [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Philipp Zabel 2019-09-05 10:15 ` [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override Philipp Zabel @ 2019-09-05 10:31 ` Tomasz Figa 2019-09-05 10:39 ` Philipp Zabel 2019-10-03 21:07 ` Paul Kocialkowski 2 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2019-09-05 10:31 UTC (permalink / raw) To: Philipp Zabel Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > This flag tells the kernel whether the slice header contained the > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > instead. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > include/media/h264-ctrls.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > index bc5dd8e76567..451a5b0f2a35 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > - 0x00000008 > - > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > + - 0x00000010 > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > As far as I remember, the idea was for the userspace to always put the right num_ref_idx in the slice_params and the drivers always use that. Was there any problem with that? Best regards, Tomasz > ``Prediction Weight Table`` > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index e877bf1d537c..dab519aea9bf 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -133,6 +133,7 @@ struct v4l2_h264_pred_weight_table { > #define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD 0x02 > #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED 0x04 > #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH 0x08 > +#define V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE 0x10 > > struct v4l2_ctrl_h264_slice_params { > /* Size in bytes, including header */ > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-05 10:31 ` [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Tomasz Figa @ 2019-09-05 10:39 ` Philipp Zabel 2019-09-05 14:17 ` Nicolas Dufresne 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2019-09-05 10:39 UTC (permalink / raw) To: Tomasz Figa Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > This flag tells the kernel whether the slice header contained the > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > instead. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > include/media/h264-ctrls.h | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > index bc5dd8e76567..451a5b0f2a35 100644 > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > - 0x00000008 > > - > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > + - 0x00000010 > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > As far as I remember, the idea was for the userspace to always put the > right num_ref_idx in the slice_params and the drivers always use that. > Was there any problem with that? I don't think so, at least for currently known hardware. In that case we should drop the unused num_ref_idx_l[01]_default_active_minus1 fields from struct v4l2_ctrl_h264_pps and document that userspace should fill the defaults into v4l2_ctrl_h264_slice_params themselves if num_ref_idx_active_override_flag wasn't set. regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-05 10:39 ` Philipp Zabel @ 2019-09-05 14:17 ` Nicolas Dufresne 2019-09-09 7:09 ` Tomasz Figa 0 siblings, 1 reply; 12+ messages in thread From: Nicolas Dufresne @ 2019-09-05 14:17 UTC (permalink / raw) To: Philipp Zabel, Tomasz Figa Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer [-- Attachment #1: Type: text/plain, Size: 2031 bytes --] Le jeudi 05 septembre 2019 à 12:39 +0200, Philipp Zabel a écrit : > On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > This flag tells the kernel whether the slice header contained the > > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > > instead. > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > > include/media/h264-ctrls.h | 1 + > > > 2 files changed, 4 insertions(+) > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > index bc5dd8e76567..451a5b0f2a35 100644 > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > > - 0x00000008 > > > - > > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > > + - 0x00000010 > > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > > > > As far as I remember, the idea was for the userspace to always put the > > right num_ref_idx in the slice_params and the drivers always use that. > > Was there any problem with that? > > I don't think so, at least for currently known hardware. > > In that case we should drop the unused > num_ref_idx_l[01]_default_active_minus1 fields from struct > v4l2_ctrl_h264_pps and document that userspace should fill > the defaults into v4l2_ctrl_h264_slice_params themselves if > num_ref_idx_active_override_flag wasn't set. It might have been added in a previous effort to allow reconstructing the bitstream from the structures. > > regards > Philipp [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-05 14:17 ` Nicolas Dufresne @ 2019-09-09 7:09 ` Tomasz Figa 2019-09-09 7:30 ` Philipp Zabel 0 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2019-09-09 7:09 UTC (permalink / raw) To: Nicolas Dufresne Cc: Philipp Zabel, Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Thu, Sep 5, 2019 at 11:17 PM Nicolas Dufresne <nicolas.dufresne@collabora.com> wrote: > > Le jeudi 05 septembre 2019 à 12:39 +0200, Philipp Zabel a écrit : > > On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > > > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > This flag tells the kernel whether the slice header contained the > > > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > > > instead. > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > --- > > > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > > > include/media/h264-ctrls.h | 1 + > > > > 2 files changed, 4 insertions(+) > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > index bc5dd8e76567..451a5b0f2a35 100644 > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > > > - 0x00000008 > > > > - > > > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > > > + - 0x00000010 > > > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > > > > > > > As far as I remember, the idea was for the userspace to always put the > > > right num_ref_idx in the slice_params and the drivers always use that. > > > Was there any problem with that? > > > > I don't think so, at least for currently known hardware. > > > > In that case we should drop the unused > > num_ref_idx_l[01]_default_active_minus1 fields from struct > > v4l2_ctrl_h264_pps and document that userspace should fill > > the defaults into v4l2_ctrl_h264_slice_params themselves if > > num_ref_idx_active_override_flag wasn't set. > > It might have been added in a previous effort to allow reconstructing > the bitstream from the structures. Wouldn't one still be able to reconstruct a valid (but not exact) stream without that flag, given the assumption above? Best regards, Tomasz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-09 7:09 ` Tomasz Figa @ 2019-09-09 7:30 ` Philipp Zabel 2019-09-09 9:40 ` Tomasz Figa 0 siblings, 1 reply; 12+ messages in thread From: Philipp Zabel @ 2019-09-09 7:30 UTC (permalink / raw) To: Tomasz Figa, Nicolas Dufresne Cc: Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Mon, 2019-09-09 at 16:09 +0900, Tomasz Figa wrote: > On Thu, Sep 5, 2019 at 11:17 PM Nicolas Dufresne > <nicolas.dufresne@collabora.com> wrote: > > > > Le jeudi 05 septembre 2019 à 12:39 +0200, Philipp Zabel a écrit : > > > On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > > > > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > This flag tells the kernel whether the slice header contained the > > > > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > > > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > > > > instead. > > > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > --- > > > > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > > > > include/media/h264-ctrls.h | 1 + > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > index bc5dd8e76567..451a5b0f2a35 100644 > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > > > > - 0x00000008 > > > > > - > > > > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > > > > + - 0x00000010 > > > > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > > > > > > > > > > As far as I remember, the idea was for the userspace to always put the > > > > right num_ref_idx in the slice_params and the drivers always use that. > > > > Was there any problem with that? > > > > > > I don't think so, at least for currently known hardware. > > > > > > In that case we should drop the unused > > > num_ref_idx_l[01]_default_active_minus1 fields from struct > > > v4l2_ctrl_h264_pps and document that userspace should fill > > > the defaults into v4l2_ctrl_h264_slice_params themselves if > > > num_ref_idx_active_override_flag wasn't set. > > > > It might have been added in a previous effort to allow reconstructing > > the bitstream from the structures. > > Wouldn't one still be able to reconstruct a valid (but not exact) > stream without that flag, given the assumption above? If the Hantro G1, as appears to be the case, parses the slice header and decides itself whether to use the override from the slice or the default that was written to a register, it needs the num_ref_idx_l[01]_default_active_minus1 field to fill the register, but doesn't need either the num_ref_idx_l[01]_active_minus1 override nor the flag. A decoder that doesn't parse the slice header can always be told to use the override (thus no need to have the flag in the uapi), if userspace fills the default into the override fields as a fallback. Such a decoder does need the num_ref_idx_l[01]_active_minus1 override, but doesn't need the num_ref_idx_l[01]_default_active_minus1 field nor the flag. That is my current understanding of the intention behind this interface, I hope this is accurate. I've tried to make the docs reflect this in ("media: uapi: h264: clarify num_ref_idx_l[01]_(default_)active fields") [1]. [1] https://lore.kernel.org/linux-media/20190905114210.9232-1-p.zabel@pengutronix.de/T/#u regards Philipp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-09 7:30 ` Philipp Zabel @ 2019-09-09 9:40 ` Tomasz Figa 2019-10-03 21:06 ` Paul Kocialkowski 0 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2019-09-09 9:40 UTC (permalink / raw) To: Philipp Zabel Cc: Nicolas Dufresne, Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Jonas Karlman, Paul Kocialkowski, Boris Brezillon, Sasha Hauer On Mon, Sep 9, 2019 at 4:30 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Mon, 2019-09-09 at 16:09 +0900, Tomasz Figa wrote: > > On Thu, Sep 5, 2019 at 11:17 PM Nicolas Dufresne > > <nicolas.dufresne@collabora.com> wrote: > > > > > > Le jeudi 05 septembre 2019 à 12:39 +0200, Philipp Zabel a écrit : > > > > On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > > > > > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > This flag tells the kernel whether the slice header contained the > > > > > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > > > > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > > > > > instead. > > > > > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > > --- > > > > > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > > > > > include/media/h264-ctrls.h | 1 + > > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > index bc5dd8e76567..451a5b0f2a35 100644 > > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > > > > > - 0x00000008 > > > > > > - > > > > > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > > > > > + - 0x00000010 > > > > > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > > > > > > > > > > > > > As far as I remember, the idea was for the userspace to always put the > > > > > right num_ref_idx in the slice_params and the drivers always use that. > > > > > Was there any problem with that? > > > > > > > > I don't think so, at least for currently known hardware. > > > > > > > > In that case we should drop the unused > > > > num_ref_idx_l[01]_default_active_minus1 fields from struct > > > > v4l2_ctrl_h264_pps and document that userspace should fill > > > > the defaults into v4l2_ctrl_h264_slice_params themselves if > > > > num_ref_idx_active_override_flag wasn't set. > > > > > > It might have been added in a previous effort to allow reconstructing > > > the bitstream from the structures. > > > > Wouldn't one still be able to reconstruct a valid (but not exact) > > stream without that flag, given the assumption above? > > If the Hantro G1, as appears to be the case, parses the slice header and > decides itself whether to use the override from the slice or the default > that was written to a register, it needs the > num_ref_idx_l[01]_default_active_minus1 field to fill the register, but > doesn't need either the num_ref_idx_l[01]_active_minus1 override nor the > flag. > > A decoder that doesn't parse the slice header can always be told to use > the override (thus no need to have the flag in the uapi), if userspace > fills the default into the override fields as a fallback. Such a decoder > does need the num_ref_idx_l[01]_active_minus1 override, but doesn't need > the num_ref_idx_l[01]_default_active_minus1 field nor the flag. > > That is my current understanding of the intention behind this interface, > I hope this is accurate. > I've tried to make the docs reflect this in ("media: uapi: h264: clarify > num_ref_idx_l[01]_(default_)active fields") [1]. > > [1] https://lore.kernel.org/linux-media/20190905114210.9232-1-p.zabel@pengutronix.de/T/#u Yes, makes sense for me. Best regards, Tomasz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-09 9:40 ` Tomasz Figa @ 2019-10-03 21:06 ` Paul Kocialkowski 0 siblings, 0 replies; 12+ messages in thread From: Paul Kocialkowski @ 2019-10-03 21:06 UTC (permalink / raw) To: Tomasz Figa Cc: Philipp Zabel, Nicolas Dufresne, Linux Media Mailing List, Hans Verkuil, Ezequiel Garcia, Jonas Karlman, Boris Brezillon, Sasha Hauer [-- Attachment #1: Type: text/plain, Size: 4558 bytes --] Hi, On Mon 09 Sep 19, 18:40, Tomasz Figa wrote: > On Mon, Sep 9, 2019 at 4:30 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > On Mon, 2019-09-09 at 16:09 +0900, Tomasz Figa wrote: > > > On Thu, Sep 5, 2019 at 11:17 PM Nicolas Dufresne > > > <nicolas.dufresne@collabora.com> wrote: > > > > > > > > Le jeudi 05 septembre 2019 à 12:39 +0200, Philipp Zabel a écrit : > > > > > On Thu, 2019-09-05 at 19:31 +0900, Tomasz Figa wrote: > > > > > > On Thu, Sep 5, 2019 at 7:15 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > > This flag tells the kernel whether the slice header contained the > > > > > > > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > > > > > > > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > > > > > > > instead. > > > > > > > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > > > --- > > > > > > > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > > > > > > > include/media/h264-ctrls.h | 1 + > > > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > > index bc5dd8e76567..451a5b0f2a35 100644 > > > > > > > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > > > > > > > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > > > > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > > > > > > > - 0x00000008 > > > > > > > - > > > > > > > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > > > > > > > + - 0x00000010 > > > > > > > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > > > > > > > > > > > > > > > > > > As far as I remember, the idea was for the userspace to always put the > > > > > > right num_ref_idx in the slice_params and the drivers always use that. > > > > > > Was there any problem with that? > > > > > > > > > > I don't think so, at least for currently known hardware. > > > > > > > > > > In that case we should drop the unused > > > > > num_ref_idx_l[01]_default_active_minus1 fields from struct > > > > > v4l2_ctrl_h264_pps and document that userspace should fill > > > > > the defaults into v4l2_ctrl_h264_slice_params themselves if > > > > > num_ref_idx_active_override_flag wasn't set. > > > > > > > > It might have been added in a previous effort to allow reconstructing > > > > the bitstream from the structures. > > > > > > Wouldn't one still be able to reconstruct a valid (but not exact) > > > stream without that flag, given the assumption above? > > > > If the Hantro G1, as appears to be the case, parses the slice header and > > decides itself whether to use the override from the slice or the default > > that was written to a register, it needs the > > num_ref_idx_l[01]_default_active_minus1 field to fill the register, but > > doesn't need either the num_ref_idx_l[01]_active_minus1 override nor the > > flag. > > > > A decoder that doesn't parse the slice header can always be told to use > > the override (thus no need to have the flag in the uapi), if userspace > > fills the default into the override fields as a fallback. Such a decoder > > does need the num_ref_idx_l[01]_active_minus1 override, but doesn't need > > the num_ref_idx_l[01]_default_active_minus1 field nor the flag. > > > > That is my current understanding of the intention behind this interface, > > I hope this is accurate. > > I've tried to make the docs reflect this in ("media: uapi: h264: clarify > > num_ref_idx_l[01]_(default_)active fields") [1]. > > > > [1] https://lore.kernel.org/linux-media/20190905114210.9232-1-p.zabel@pengutronix.de/T/#u > > Yes, makes sense for me. Sorry for being late to the party and catching up with this just now. It looks like the conclusion is that we need to keep both the PPS default and slice-specific set of fields to cover both hardware that parses the slice header and hardware that doesn't. In H265, I had evicted the PPS default value and the flag in favor of only providing a slice-specific value, but I should certainly revise this to have the two sets of values and the flag. This should allow covering hardware that does slice header parsing itself. Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag 2019-09-05 10:15 [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Philipp Zabel 2019-09-05 10:15 ` [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override Philipp Zabel 2019-09-05 10:31 ` [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Tomasz Figa @ 2019-10-03 21:07 ` Paul Kocialkowski 2 siblings, 0 replies; 12+ messages in thread From: Paul Kocialkowski @ 2019-10-03 21:07 UTC (permalink / raw) To: Philipp Zabel Cc: linux-media, Hans Verkuil, Ezequiel Garcia, Nicolas Dufresne, Tomasz Figa, Jonas Karlman, Boris Brezillon, kernel [-- Attachment #1: Type: text/plain, Size: 1931 bytes --] Hi, On Thu 05 Sep 19, 12:15, Philipp Zabel wrote: > This flag tells the kernel whether the slice header contained the > num_ref_idx_l[01]_active_minus1 syntax elements, or whether the > num_ref_idx_l[01]_default_active_minus1 from PPS should be used > instead. Looks good to me: Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 3 +++ > include/media/h264-ctrls.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > index bc5dd8e76567..451a5b0f2a35 100644 > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst > @@ -1860,6 +1860,9 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > * - ``V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH`` > - 0x00000008 > - > + * - ``V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE > + - 0x00000010 > + - Corresponds to the num_ref_idx_active_override_flag syntax element. > > ``Prediction Weight Table`` > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index e877bf1d537c..dab519aea9bf 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -133,6 +133,7 @@ struct v4l2_h264_pred_weight_table { > #define V4L2_H264_SLICE_FLAG_BOTTOM_FIELD 0x02 > #define V4L2_H264_SLICE_FLAG_DIRECT_SPATIAL_MV_PRED 0x04 > #define V4L2_H264_SLICE_FLAG_SP_FOR_SWITCH 0x08 > +#define V4L2_H264_SLICE_FLAG_NUM_REF_IDX_ACTIVE_OVERRIDE 0x10 > > struct v4l2_ctrl_h264_slice_params { > /* Size in bytes, including header */ > -- > 2.20.1 > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-03 21:07 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-05 10:15 [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Philipp Zabel 2019-09-05 10:15 ` [PATCH 2/2] media: hantro: h264: per-slice num_ref_idx_l[01]_active override Philipp Zabel 2019-09-05 10:34 ` Tomasz Figa 2019-09-05 11:04 ` Philipp Zabel 2019-09-05 10:31 ` [PATCH 1/2] media: uapi: h264: Add num_ref_idx_active_override_flag Tomasz Figa 2019-09-05 10:39 ` Philipp Zabel 2019-09-05 14:17 ` Nicolas Dufresne 2019-09-09 7:09 ` Tomasz Figa 2019-09-09 7:30 ` Philipp Zabel 2019-09-09 9:40 ` Tomasz Figa 2019-10-03 21:06 ` Paul Kocialkowski 2019-10-03 21:07 ` Paul Kocialkowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).