Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Alexandre Courbot]

Hi Hans,

On Tue, Nov 27, 2018 at 1:41 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>
> On 11/26/2018 05:07 PM, Tomasz Figa wrote:
> > On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>
> >> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
> >>> Hi Hans,
> >>>
> >>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >>>>
> >>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
> >>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> >>>>>>> Accept the buffer size requested by client and compare it
> >>>>>>> against driver calculated size and set the maximum to
> >>>>>>> bitstream plane.
> >>>>>>>
> >>>>>>> Signed-off-by: Malathi Gottam <mgottam@xxxxxxxxxxxxxx>
> >>>>>>
> >>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> >>>>>> never the other way around.
> >>>>>
> >>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> >>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
> >>>>> paragraph point 2:
> >>>>>
> >>>>>     ``sizeimage``
> >>>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
> >>>>>        match hardware requirements
> >>>>>
> >>>>> Similar patch we be needed for decoder as well.
> >>>>
> >>>> I may have missed that change since it wasn't present in v1 of the stateful
> >>>> encoder spec.
> >>>
> >>> It's been there from the very beginning, even before I started working
> >>> on it. Actually, even the early slides from Kamil mention the
> >>> application setting the buffer size for compressed streams:
> >>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
> >>>
> >>>>
> >>>> Tomasz, what was the reason for this change? I vaguely remember some thread
> >>>> about this, but I forgot the details. Since this would be a departure of
> >>>> the current API this should be explained in more detail.
> >>>
> >>> The kernel is not the place to encode assumptions about optimal
> >>> bitstream chunk sizes. It depends on the use case and the application
> >>> should be able decide. It may for example want to use smaller buffers,
> >>> optimizing for the well compressible video streams and just reallocate
> >>> if bigger chunks are needed.
> >>>
> >>>>
> >>>> I don't really see the point of requiring userspace to fill this in. For
> >>>> stateful codecs it can just return some reasonable size. Possibly calculated
> >>>> using the provided width/height values or (if those are 0) some default value.
> >>>
> >>> How do we decide what's "reasonable"? Would it be reasonable for all
> >>> applications?
> >>
> >> In theory it should be the minimum size that the hardware supports. But it is
> >> silly to i.e. provide the size of one PAGE as the minimum. In practice you
> >> probably want to set sizeimage to something larger than that. Depending on
> >> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
> >> would be.
> >>
> >>>
> >>>>
> >>>> Ditto for decoders.
> >>>>
> >>>> Stanimir, I certainly cannot merge this until this has been fully nailed down
> >>>> as it would be a departure from the current API.
> >>>
> >>> It would not be a departure, because I can see existing stateful
> >>> drivers behaving like that:
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
> >>
> >> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
> >> It should have been caught at least for the mtk driver.
> >>
> >
> > Perhaps we should make it a part of the spec then?
> >
> > Actually I'm not really sure if we can say that this is out of spec
> > There has been no clear spec for the stateful codecs for many years,
> > with drivers doing wildly whatever they like and applications ending
> > up relying on those quirks.
>
> The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the
> driver that sets this value. The spec for v4l2_plane_pix_format is
> unfortunately not so clear.
>
> > My spec actually attempts to incorporate what was decided on the
> > earlier summits, including what's in Kamil's slides, the drivers are
> > already doing and existing applications rely on. The sizeimage
> > handling is just a part of it.
> >
> >>>
> >>> Also, Chromium has been setting the size on its own for long time
> >>> using its own heuristics.
> >>>
> >>>>
> >>>> And looking at the venus patch I do not see how it helps userspace.
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>>>
> >>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> >>>>>> of VIDIOC_REQBUFS.
> >>>
> >>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
> >>> a format for it and the format returned by it would have the sizeimage
> >>> as hardcoded in the driver...
> >>
> >> ???
> >>
> >> Userspace can change the sizeimage to whatever it wants before calling
> >> CREATE_BUFS as long as it is >= the sizeimage of the current format.
> >>
> >> If we want to allow smaller sizes, then I think that would not be
> >> unreasonable for stateful codecs. I actually think that drivers can
> >> already do this in queue_setup(), but the spec would have to be updated
> >> a bit.
> >
> > Existing applications rely on REQBUFS honoring the size they set in
> > sizeimage, though...
>
> REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the
> first place.
>
> However (and now I remember the real problem with CREATE_BUFS) the spec for
> CREATE_BUFS says that it will not change the provided sizeimage value. So
> any adjustments required due to specific alignment requirements won't be
> applied, all CREATE_BUFS can do is to reject it.
>
> So what this boils down to is a change to the spec:
>
> For compressed formats (and only those!) userspace can set sizeimage to a
> proposed value. The driver may either ignore it and just set its own value,
> or modify it to satisfy HW requirements. The returned value will be used
> by REQBUFS when it allocates buffers.
>
> I think this is reasonable, provided the spec is updated accordingly.
>
> As far as I can tell this shouldn't cause any backwards compatibility
> problems, and it should be easy to test in v4l2-compliance.

Do you mean that this patch is acceptable provided the stateful codec
specification is updated accordingly?

For our (Chromium) needs this seems to do the job, so:

Tested-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>

Although I would like to also have the equivalent for the decoder's
OUTPUT queue, either as a v4 or a follow-up patch.