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.
