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
