Re: [PATCH v2] media: v4l2-ctrls: validate HEVC slice reference lists

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

Hi Nicolas,

On Thu 09 Apr 26, 10:14, Nicolas Dufresne wrote:
> Le jeudi 09 avril 2026 à 15:52 +0200, Paul Kocialkowski a écrit :
> > Hi Nicolas,
> > 
> > On Mon 23 Mar 26, 09:41, Nicolas Dufresne wrote:
> > > > +
> > > > +		for (i = 0; i <= p_hevc_slice_params->num_ref_idx_l0_active_minus1;
> > > > +		     i++)
> > > > +			if (p_hevc_slice_params->ref_idx_l0[i] >=
> > > > +			    V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
> > > > +				return -EINVAL;
> > > 
> > > That one is a breaking change since userspace already passes off limit values
> > > such as 0xff when a reference is missing (was lost). See:
> > > 
> > > 	47825b1646a6a9eca0f90baa3d4f98947c2add96
> > > 
> > > The hardware may or may not be capable of doing concealment, but with this
> > > change, we bring down all drivers to failing the decode completely.
> > 
> > So while some decoders may be able to deal with missing references, it seems
> > that cedrus should still error out in that case. I don't think it will be very
> > happy if we configure the hardware with L0/L1 lists that don't match what was
> > used for the encode.
> 
> The L0/L1 list passed by application do match the decoder list, but has gaps. By
> the spec, decoder should be gap resistant. You are better place them me to know
> what the Cedrus hardware can and cannot do. This is rarely well document, and in
> RE case like this, you probably have to do some trial and errors.

Indeed. My concern here is more specific to cedrus: since the references are
written to a SRAM area we have to configure something for the entry or it will
just keep the value from the previous frame.

Also the current cedrus h265 code doesn't check for cases of missing references,
which will certainly blow up when trying to access the corresponding DPB index.

> > But maybe we could pick up another (existing or empty) reference to replace the
> > missing one, which would be better than failing to decode the frame. IMO this
> > would be best done by userspace, but maybe we'd need some indication to know
> > that the hardware cannot deal with missing references.
> 
> Exact, experimenting first seems key, every hardware I've worked with behaves
> differently. Hantro G2 notably tends to cause system wide issues on imx8mq if
> you don't carefully select your replacement. Yet, after testing and looking at
> the visual result, its way better (visually) to pick a replacement. I have
> patches coming that tracks which frame have decoded successfully and holds
> initialized MV data (which was the reason it was going wild).
> 
> Note that going one step further, on HEVC we could use the poc to find a valid
> replacement that is temporarily closer, but that would simply be an enhancement
> for streams with reordering.

Right, for both H.264 and H.265 we could have a (hopefully generic) way to find
the most recent frame to replace a missing reference, and return an error status
with a full payload size.

All the best,

Paul

> Nicolas
> 
> > 
> > What do you think?
> > 
> > All the best,
> > 
> > Paul
> > 
> > > > +
> > > > +		if (p_hevc_slice_params->slice_type != V4L2_HEVC_SLICE_TYPE_B)
> > > > +			break;
> > > > +
> > > > +		if (p_hevc_slice_params->num_ref_idx_l1_active_minus1 >=
> > > > +		    V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
> > > > +			return -EINVAL;
> > > 
> > > Ack.
> > > 
> > > > +
> > > > +		for (i = 0; i <= p_hevc_slice_params->num_ref_idx_l1_active_minus1;
> > > > +		     i++)
> > > > +			if (p_hevc_slice_params->ref_idx_l1[i] >=
> > > > +			    V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
> > > > +				return -EINVAL;
> > > 
> > > Same.
> > > 
> > > cheers,
> > > Nicolas
> > > 
> > > >  		break;
> > > >  
> > > >  	case V4L2_CTRL_TYPE_HEVC_EXT_SPS_ST_RPS:
> > 
> > 



-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]