Re: [PATCH] hantro: Fix JPEG encoder ENUM_FRAMESIZE on RK3399

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

Hi,

Le vendredi 23 décembre 2022 à 13:28 -0300, Ezequiel Garcia a écrit :
> Hi everyone,
> 
> On Fri, Dec 23, 2022 at 11:17 AM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > The frmsize structure was left initialize to 0, as side effect, the driver was
> > reporting an invalid frmsize.
> > 
> >   Size: Stepwise 0x0 - 0x0 with step 0/0
> > 
> > Fix this by replicating the constraints in the raw formats too. This fixes
> > taking picture in Gnome Cheese Software, or any software using GSteamer
> > encodebin feature.
> > 
> > Fixes: 775fec69008d30 ("media: add Rockchip VPU JPEG encoder driver")
> 
> The frmsize is only for bitstream formats (see comment in struct hantro_fmt).
> If I can read code correctly, this was broken by this commit:
> 
> commit 79c987de8b35421a2a975982929f150dd415f8f7
> Author: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Date:   Mon Apr 4 18:06:40 2022 +0200
> 
>     media: hantro: Use post processor scaling capacities
> 
> Before that commit we used to return EINVAL for enum_framesizes
> in RAW formats. I guess we missed that :-)

I see, and gstreamer had a quirk for such a bogus response. Let me explain why
its bogus, for the general knowlege. A driver that supports ENUM_FRAMESIZE but
does not return any sizes, is in theory a driver that does not support anything.
Fortunaly, GStreamer considered not having a single framesize bogus, and would
fallback to the old school try_fmt() dance to find the supported sizes.

So yes, it used to work in gstreamer, and its indeed
79c987de8b35421a2a975982929f150dd415f8f7 that broke it. I'll correct his in V2.

> 
> To be completely honest, I'm not sure if we used to support encodebin,
> and I'm not too sure how to approach this issue, but I would really
> love to start with something super simple like:
> 
> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c
> b/drivers/media/platform/verisilicon/hantro_v4l2.c
> index 2c7a805289e7..0b28f86b7463 100644
> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> @@ -161,8 +161,11 @@ static int vidioc_enum_framesizes(struct file
> *file, void *priv,
>         }
> 
>         /* For non-coded formats check if postprocessing scaling is possible */
> -       if (fmt->codec_mode == HANTRO_MODE_NONE &&
> hantro_needs_postproc(ctx, fmt)) {
> -               return hanto_postproc_enum_framesizes(ctx, fsize);
> +       if (fmt->codec_mode == HANTRO_MODE_NONE)
> +        if (hantro_needs_postproc(ctx, fmt))
> +            return hanto_postproc_enum_framesizes(ctx, fsize);
> +        else
> +            return -ENOTTY;
>         } else if (fsize->index != 0) {
>                 vpu_debug(0, "invalid frame size index (expected 0, got %d)\n",
>                           fsize->index);
> 
> (ENOTTY was suggested by Nicolas on IRC)
> 
> Nicolas also pointed out that our current handling of frmsize is not correct,
> as we cannot express different constraints on combinations of RAW
> and bitstream formats.
> 
> This seems to call for a rework of enum_framesizes, so frmsize
> is not static but somehow obtained per-codec.

So I'll respin along these line to we more or less "revert back" to working
state. Though having a framesize enumeration on encoder raw (OUTPUT queue) is
what makes most sense so that will have to be revisited with a corrected
mechanism, as whenever we add VP8 and H.264 encoding, we'll need different range
per codec. I'll check in January with my colleague, we might do that inside the
VP8 encoder branch (that is nearly ready and will be sent after the holidays),
or could be an intermediate set.

regards,
Nicolas

> 
> Thanks,
> Ezequiel
> 
> > Reported-by: Robert Mader <robert.mader@collabora.com>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> And thanks a lot for the report and the patch!
>