| Message ID | 20190530211516.1891-3-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cedrus: Improvements/cleanup | expand |
On Thu, May 30, 2019 at 11:15:11PM +0200, Jernej Skrabec wrote: > Reference index count in VE_H264_PPS reg should come from PPS control. > However, this is not really important because reference index count is > in our case always overridden by that from slice header. > > Cc: nicolas@ndufresne.ca > Cc: boris.brezillon@collabora.com > Cc: jonas@kwiboo.se > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> > --- > We have to decide if we drop pps->num_ref_idx_l0_default_active_minus1 > and pps->num_ref_idx_l1_default_active_minus1 fields or add > num_ref_idx_l0_active_override_flag and num_ref_idx_l0_active_override_flag > to slice control. > > Current control doesn't have those two flags, so in Cedrus override flag is > always set and we rely on userspace to set slice->num_ref_idx_l0_active_minus1 > and slice->num_ref_idx_l1_active_minus1 to correct values. This means that > values stored in PPS are not needed and always ignored by VPU. > > If I understand correctly, algorithm is very simple: > > ref_count = PPS->ref_count > if (override_flag) > ref_count = slice->ref_count > > It seems that VAAPI provides only final value. In my opinion we should do the > same - get rid of PPS default ref index count fields. The rationale was to be as conservative as possible and just expose everything that is in the bitstream in those controls to accomodate for as many weird hardware as possible. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Dne ponedeljek, 03. junij 2019 ob 13:46:20 CEST je Maxime Ripard napisal(a): > On Thu, May 30, 2019 at 11:15:11PM +0200, Jernej Skrabec wrote: > > Reference index count in VE_H264_PPS reg should come from PPS control. > > However, this is not really important because reference index count is > > in our case always overridden by that from slice header. > > > > Cc: nicolas@ndufresne.ca > > Cc: boris.brezillon@collabora.com > > Cc: jonas@kwiboo.se > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Acked-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > --- > > We have to decide if we drop pps->num_ref_idx_l0_default_active_minus1 > > and pps->num_ref_idx_l1_default_active_minus1 fields or add > > num_ref_idx_l0_active_override_flag and > > num_ref_idx_l0_active_override_flag > > to slice control. Actually only one flag is in bitstream valid for both l0 and l1 ref list. > > > > Current control doesn't have those two flags, so in Cedrus override flag > > is > > always set and we rely on userspace to set > > slice->num_ref_idx_l0_active_minus1 and > > slice->num_ref_idx_l1_active_minus1 to correct values. This means that > > values stored in PPS are not needed and always ignored by VPU. > > > > If I understand correctly, algorithm is very simple: > > > > ref_count = PPS->ref_count > > if (override_flag) > > > > ref_count = slice->ref_count > > > > It seems that VAAPI provides only final value. In my opinion we should do > > the same - get rid of PPS default ref index count fields. > > The rationale was to be as conservative as possible and just expose > everything that is in the bitstream in those controls to accomodate > for as many weird hardware as possible. Ok, so then we should add that override flag, which would align with h264 specs and you can still do same trick in VAAPI library which it's currently used in Cedrus driver - always set override flag and fill out only slice reflist count. At the end it shouldn't matter for proper decoding in any driver. Best regards, Jernej
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index a30bb283f69f..cc8d17f211a1 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c @@ -340,12 +340,8 @@ static void cedrus_set_params(struct cedrus_ctx *ctx, // picture parameters reg = 0; - /* - * FIXME: the kernel headers are allowing the default value to - * be passed, but the libva doesn't give us that. - */ - reg |= (slice->num_ref_idx_l0_active_minus1 & 0x1f) << 10; - reg |= (slice->num_ref_idx_l1_active_minus1 & 0x1f) << 5; + reg |= (pps->num_ref_idx_l0_default_active_minus1 & 0x1f) << 10; + reg |= (pps->num_ref_idx_l1_default_active_minus1 & 0x1f) << 5; reg |= (pps->weighted_bipred_idc & 0x3) << 2; if (pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE) reg |= VE_H264_PPS_ENTROPY_CODING_MODE;
Reference index count in VE_H264_PPS reg should come from PPS control. However, this is not really important because reference index count is in our case always overridden by that from slice header. Cc: nicolas@ndufresne.ca Cc: boris.brezillon@collabora.com Cc: jonas@kwiboo.se Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- We have to decide if we drop pps->num_ref_idx_l0_default_active_minus1 and pps->num_ref_idx_l1_default_active_minus1 fields or add num_ref_idx_l0_active_override_flag and num_ref_idx_l0_active_override_flag to slice control. Current control doesn't have those two flags, so in Cedrus override flag is always set and we rely on userspace to set slice->num_ref_idx_l0_active_minus1 and slice->num_ref_idx_l1_active_minus1 to correct values. This means that values stored in PPS are not needed and always ignored by VPU. If I understand correctly, algorithm is very simple: ref_count = PPS->ref_count if (override_flag) ref_count = slice->ref_count It seems that VAAPI provides only final value. In my opinion we should do the same - get rid of PPS default ref index count fields. drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)