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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le mercredi 30 mars 2022 à 08:15 +0300, Dan Carpenter a écrit :
> 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().

There is two schools in the context of video stream decoding. I'm not saying
this driver is quite there in term of visual corruption reporting, this is
something I expect we'll improve in the long term. But here's the two schools:

- Freeze on the last non-corrupted image (Apple style)
- Display slightly distorted image with movement (the rest of the world)

In order to give users that choice, I must try decoding as much as I can
regardless if there is a missing a reference or not. That wasn't the goal in
this MR, but in the long run we'll remember this and mark the buffer as
corrupted (using the ERROR but setting a payload size to the picture size). This
leaves the users the option to drop or to keep the visually corrupted image. In
video streaming, corrupted stream could look relatively fine, this cannot be
judge noticing 1 reference frame missing (it could be referenced in only 1
macro-block).

Educational bit behind, I think we should keep going and not "error out". Also,
for debugging purpose, it is nicer if we can get a complete report of non-memory
backed references. As a missing reference in 1 frame, may have implication in
later frame, or may cause other frames to be missed.

One thing I was thinking though, is that through using raw pr_debug() I'm not
giving much context to my trace, so it would be hard to associate it with the
driver instance (m2m are multi-instance) it was running against. But I didn't
want to add a new tracing wrapper for that driver in this patchset as it was
already relatively large patchset. Though, these traces have been previous to
make the driver work (as long as you test with a single instance).

If its all right with everyone, I'd leave it like this for this round, we can
dedicated a patchset on improve this driver tracing in the future.

> 
> > > >  	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.

I'll do that in v2, in same patch, looks minor enough. I think if using bool
could guaranty that only 1 or 0 is  possible, it would be even better, but don't
think C works like this.

Thanks again for your comments.

> 
> regards,
> dan carpenter
>