On 11/27/2018 09:16 AM, Alexandre Courbot wrote: > 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? Yes. Although I would update the spec in a separate patch. It is not really part of the codec spec as such, more a prerequisite. Regards, Hans > 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. >
