Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Mar 29, 2022 at 04:54:55PM -0400, Nicolas Dufresne wrote:
> Le mardi 29 mars 2022 à 11:13 +0300, Dan Carpenter a écrit :
> > On Mon, Mar 28, 2022 at 03:59:31PM -0400, Nicolas Dufresne wrote:
> > > @@ -738,23 +735,26 @@ static void lookup_ref_buf_idx(struct rkvdec_ctx *ctx,
> > >  		struct vb2_queue *cap_q = &m2m_ctx->cap_q_ctx.q;
> > >  		int buf_idx = -1;
> > >  
> > > -		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > +		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE) {
> > >  			buf_idx = vb2_find_timestamp(cap_q,
> > >  						     dpb[i].reference_ts, 0);
> > > +			if (buf_idx < 0)
> > > +				pr_debug("No buffer for reference_ts %llu",
> > > +					 dpb[i].reference_ts);
> > 
> > pr_debug() is too quiet.  Make it pr_err().  Set buf_idx to zero instead
> > leaving it as an error code.
> 
> Thanks for the suggestion, I'm just a bit uncomfortable using pr_err() for
> something that is not a driver error, but userland error. Perhaps you can
> educate me on the policy in this regard, but malicous userland being able to
> flood the logs very easily is my main concern here.
> 
> About the negative idx, it is being used set dpb_valid later on. H.264 error
> resilience requires that these frames should be marked as "unexisting" but still
> occupy space in the DPB, this is more or less what I'm trying to implement here.
> Setting it to 0 would basically mean to refer to DPB index 0, which is
> relatively random pick. I believe your suggestion is not taking into
> consideration what the code is doing, but it would fall in some poor-man
> concealment which I would rather leave to the userland.
> 

To be honest, I just saw that it was a negative idx and freaked out.  I
didn't look at any context...

You're right that we don't to allow the user to spam the dmesg.  Ideally
we would return an error.  A second best solution is to do a pr_err_once().

> > >  	for (j = 0; j < RKVDEC_NUM_REFLIST; j++) {
> > > -		for (i = 0; i < h264_ctx->reflists.num_valid; i++) {
> > > -			u8 dpb_valid = run->ref_buf_idx[i] >= 0;
> > > -			u8 idx = 0;
> > > +		for (i = 0; i < builder->num_valid; i++) {
> > > +			struct v4l2_h264_reference *ref;
> > > +			u8 dpb_valid;
> > > +			u8 bottom;
> > 
> > These would be better as type bool.
> 
> I never used a bool for bit operations before, but I guess that can work, thanks
> for the suggestion. As this deviates from the original code, I suppose I should
> make this a separate patch ?

I just saw the name and wondered why it was a u8.  bool does make more
sense and works fine for the bitwise stuff.  But I don't really care at
all.

regards,
dan carpenter