On Mon, Mar 18, 2019 at 1:10 AM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Tomasz, > > On Fri, Mar 15, 2019 at 01:18:17PM +0900, Tomasz Figa wrote: > > On Fri, Oct 26, 2018 at 10:42 PM Laurent Pinchart wrote: > > > On Friday, 26 October 2018 14:41:26 EEST Tomasz Figa wrote: > > >> On Thu, Sep 20, 2018 at 11:42 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > > >>> Some parts of the V4L2 API are awkward to use and I think it would be > > >>> a good idea to look at possible candidates for that. > > >>> > > >>> Examples are the ioctls that use struct v4l2_buffer: the multiplanar > > >>> support is really horrible, and writing code to support both single and > > >>> multiplanar is hard. We are also running out of fields and the timeval > > >>> isn't y2038 compliant. > > >>> > > >>> A proof-of-concept is here: > > >>> > > >>> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a > > >>> 95549df06d9900f3559afdbb9da06bd4b22d1f3 > > >>> > > >>> It's a bit old, but it gives a good impression of what I have in mind. > > >> > > >> On a related, but slightly different note, I'm wondering how we should > > >> handle a case where we have an M format (e.g. NV12M with 2 memory > > >> planes), but only 1 DMA-buf with all planes to import. That generally > > >> means that we have to use the same DMA-buf FD with an offset for each > > >> plane. In theory, v4l2_plane::data_offset could be used for this, but > > >> the documentation says that it should be set by the application only > > >> for OUTPUT planes. Moreover, existing drivers tend to just ignore > > >> it... > > > > > > The following patches may be of interest. > > > > > > https://patchwork.linuxtv.org/patch/29177/ > > > https://patchwork.linuxtv.org/patch/29178/ > > > > [+CC Boris] > > > > Thanks Laurent for pointing me to those patches. > > > > Repurposing the data_offset field may sound like a plausible way to do > > it, but it's not, for several reasons: > > > > 1) The relation between data_offset and other fields in v4l2_buffer > > makes it hard to use in drivers and userspace. > > Could you elaborate on this ? Well, it's not a critical issue, but having the application need to always add data_offset to bytesused and length makes the code a bit messy, because the values don't really relate to the plane anymore, but the buffer (or not even, since length would be less than the buffer length for planes other than the last). > > > 2) It is not handled by vb2, so each driver would have to > > explicitly add data_offset to the plane address and subtract it from > > plane size and/or bytesused (since data_offset counts into plane size > > and bytesused), > > We should certainly handle that in the V4L2 core and/or in vb2. I think > we should go one step further and handle the compose rectangle there > too, as composing for capture devices is essentially offsetting the > buffer and setting the correct stride. I wonder if it was a mistake to > expose compose rectangle on capture video nodes, maybe stride + offset > would be a better API. > > > 3) For CAPTURE buffers, it's actually defined as set-by-driver > > (https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/buffer.html#struct-v4l2-plane), > > so anything userspace sets there is bound to be ignored. I'm not sure > > if we can change this now, as it would be a compatibility issue. > > > > (There are actually real use cases for it, i.e. the venus driver > > outputs VPx encoded frames prepended with the IVF header, but that's > > not what the V4L2 VPx formats expect, so the data_offset is set by the > > driver to point to the raw bitstream data.) > > Doesn't that essentially create a custom format though ? Who consumes > the IVF header ? Nobody consumes it. I believe data_offset was made exactly for this purpose, to skip useless headers and avoid format proliferation. > > Another use case is handling of embedded data with CSI-2. > > CSI-2 sensors can send multiple types of data multiplexed in a single > virtual channels. Common use cases include sending a few lines of > metadata, or sending optical black lines, in addition to the main image. > A CSI-2 source could also send the same image in multiple formats, but I > haven't seen that happening in practice. The CSI-2 standard tags each > line with a data type in order to differentiate them on the receiver > side. On the receiver side, some receivers allow capturing different > data types in different buffers, while other support a single buffer > only, with or without data type filtering. It may thus be that a sensor > sending 2 lines of embedded data before the image to a CSI-2 receiver > that supports a single buffer will leave the user with two options, > capturing the image only or capturing both in the same buffer (really > simple receivers may only offer the last option). Reporting to the user > how data is organized in the buffer is needed, and the data_offset field > is used for this. > > This being said, I don't think it's a valid use case fo data_offset. As > mentioned above a sensor could send more than one data type in addition > to the main image (embedded data + optical black is one example), so a > single data_offset field wouldn't allow differentiating embedded data > from optical black lines. I think a more powerful frame descriptor API > would be needed for this. The fact that the buffer layout doesn't change > between frames also hints that this should be supported at the format > level, not the buffer level. FYI, this topic was also brought in some threads about the stateless video decoders, where it could be useful to still pass the full bitstream to the decoder, but tag it with locations of particular parts in the buffer (NAL headers, PPS, SPS, slice header, slice data, etc.) Best regards, Tomasz
