<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
hverkuil: <u>tfiga</u>: ping
bbrezillon: <u>pinchartl</u>: I'm about to resurect this discussion https://www.mail-archive.com/linux-media@vger.kernel.org/msg135914.html
<br> any reason https://patchwork.linuxtv.org/patch/29178/ and https://patchwork.linuxtv.org/patch/29177 got stalled
<br> I see that sailus and you were disagreed on how data_offset should be negotiated, but the discussion seems to have stopped after that
<br> s/were//
tfiga: <u>hverkuil</u>: pong
hverkuil: <u>tfiga</u>: good evening to you!
tfiga: <u>hverkuil</u>: good morning :)
hverkuil: What is the status of the stateful codec specification?
tfiga: hmm, a temporary -ENOCYCLES :(
hverkuil: The main open item as far as I know is whether or not a stateful codec should initially have an empty format (widthxheight = 0x0) or not.
bbrezillon: <u>tfiga</u>: oh, you're here :)
<br> started to work on https://www.mail-archive.com/linux-media@vger.kernel.org/msg135914.html
hverkuil: I've modified vicodec to always report a valid format, and that seems to make life much easier.
tfiga: <u>hverkuil</u>: it's a hay fever season for me and I'm a bit less productive because of that :(
bbrezillon: <u>tfiga</u>: do you have a test case for this?
tfiga: <u>hverkuil</u>: I think reporting a valid format makes sense indeed
hverkuil: <u>tfiga</u>: Still winter here in Oslo, no hay in sight :-)
tfiga: <u>bbrezillon</u>: hello :)
hverkuil: <u>tfiga</u>: I have one other question:
bbrezillon: <u>tfiga</u>: yes, should have started by saying "Hi" :)
tfiga: <u>bbrezillon</u>: yeah, I was writing a reply to your email
hverkuil: regarding the stateful encoder:
tfiga: well, we have some Chromium OS tests that exercise that
hverkuil: Initially the output format is set to 720p in vicodec.
tfiga: <u>hverkuil</u>: OUTPUT format you mean?
hverkuil: If I want to encode a 1080p stream, then I call S_FMT with 1920x1080 for the OUTPUT, but while this updates the buffer size it keeps the current crop rectangle at 1280x720.
<br> This is according to the V4L2 spec, but it is somewhat unexpected for a codec.
<br> If you go to the other direction (i.e. set the format to 640x480), then the crop rectangle is also adjusted, because the driver has no choice.
<br> I just want to verify that this is indeed the way it should work. And it might need additional attention in the encoder spec.
<br> (Note that setting the format to 1920x1080 will actually change it to 1920x1088)
<br> <u>bbrezillon</u>: don't repurpose data_offset. We need new streaming ioctls that solve this, and many other issues, properly.
bbrezillon: <u>hverkuil</u>: ok
tfiga: <u>bbrezillon</u>: I believe I posted some reasons about why I think data_offset is not a good idea, let me look it up
<br> <u>hverkuil</u>: I'd reset the crop rectangle when the format changes
hverkuil: It's a (much) bigger job, but it's time to bite the bullet and fix a lot of technical debt.
bbrezillon: <u>hverkuil</u>: I don't have a problem with that :)
hverkuil: <u>tfiga</u>: so if I set the format to 1920x1080, then the encoder will set the crop rectangle to that resolution and then adjust the format to 1920x1088 (if required by the codec), right?
<br> That would have to be documented in the v4 of the encoder spec, though.
bbrezillon: <u>tfiga</u>: you listed a few things you thought should be fixed here https://www.mail-archive.com/linux-media@vger.kernel.org/msg135914.html
tfiga: <u>hverkuil</u>: let me recheck what's written currently
<br> but IMHO it should reset the crop to the adjusted format
bbrezillon: <u>tfiga</u>: but pinchartl's reply is the last one I see in the thread
<br> or maybe you replied somewhere else
tfiga: so that the CROP_DEFAULT matches the OUTPUT format and CROP is reset to CROP_DEFAULT
hverkuil: <u>tfiga</u>: note that for a normal OUTPUT device (not an m2m device) S_FMT will keep the crop rectangle and not change it (unless the new format is smaller than the crop rectangle). This is according to the principle that ioctls shouldn't change more than strictly necessary.
<br> <u>tfiga</u>: so doing something different for codecs would need a good argument.
tfiga: <u>bbrezillon</u>: I think that was in another thread, looking for it
hverkuil: that said, I believe there is a good reason for encoders to do this. Decoders do the same for the CAPTURE queue based on information stored in the encoded stream, although it is implicit in that case.
<br> <u>bbrezillon</u>: This is the last time (I believe) new ioctls were discussed: https://www.mail-archive.com/linux-media@vger.kernel.org/msg135164.html
bbrezillon: <u>hverkuil</u>: thanks for the pointer
hverkuil: <u>bbrezillon</u>: I have plans to work on this 'sometime this year
<br> but getting the codec support finished has to be done first.
<br> If you need this earlier, then you can work on it as well (I don't know what you need it for).
bbrezillon: <u>hverkuil</u>: I see a lot of things discussed in this thread
<br> do you think it's possible to split that a bit?
<br> I mean, maybe I can work on the new multi-planar ioctls (plus new timestamping approach since it seems to depend on it) leaving the rest of the suggestions for later
hverkuil: <u>bbrezillon</u>: Well, the multiplanar and timestamping code is already done (https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3).
<br> <u>bbrezillon</u>: what's more interesting is how the buffer memory is specified in a more generic manner. And since this is a new API it needs to be reviewed, it needs compliance tests, etc.
bbrezillon: <u>hverkuil</u>: I meant starting from there and extending it to expose buffer offsets for each place, etc
<br> *plane
hverkuil: Oh, I misunderstood you. Yes, the new streaming ioctls are independent from anything else.
<br> So starting with the patch above and extending the way how the memory layout is defined would be a good approach.
bbrezillon: ok, I'll work on that then
<br> thx
hverkuil: Note that it is not sufficient to just cater to your own requirement, it should look at the generic case.
bbrezillon: I guess so
hverkuil: And I have a gut feeling that we might end up with new format ioctls as well.
bbrezillon: not sure I'll make it generic enough at my first attempt, but that's the whole point of posting the result to the ML ;)
hverkuil: I think a good concept to keep in mind (and possibly make explicit in the data structures as well) is that you have memory planes and color component planes.
<br> One memory plane can contain 1 or more color component planes.
<br> The _MPLANE API as we have today was made to support multiple memory planes.
bbrezillon: yep
hverkuil: But we need to specify this in a much more generic way. And if multiple color component planes are in one memory plane, then the padding before and after each color component plane and the padding after each line in each color component plane has to be specified as well.
bbrezillon: well, if we go for the DRM approach where each component is described with a fd/handle+offset+stride tuple, we should be good to handle all cases
hverkuil: One major problem we have today is that the pixelformats also define the padding between planes, and that makes it very inflexible.
pinchartl: <u>bbrezillon</u>: I think it was just that we lost interest :-)
<br> I still think we need a better data_offset mechanism
bbrezillon: s/component/component plane/
pinchartl: especially when using the same dmabuf handle for different planes with the multiplanar API
bbrezillon: that's for sure
hverkuil: DRM also has a modifier (forgot what it was called), that signals what tiling format is used. It's something you can take a look at as well if it is possible to integrate.
bbrezillon: yep
<br> we definitely want to have the modifier as well
hverkuil: Some convergence between v4l2 and drm in this respect will be welcome.
bbrezillon: especially if the videc or camera support different tiling formats
<br> DRM/V4L convergence: I'll think about it
hverkuil: <u>bbrezillon</u>: just where possible and reasonable. The two subsystems have different histories, but if we can make it easier to map one format description to the other, then why not?
bbrezillon: I'm not so fond of modifying the DRM struct layouts to match the one used in the V4L API, but if it's just about sharing modifiers definitions I guess it's okay
pinchartl: <u>hverkuil</u>: the biggest item on our plate is then using the same 4CC definitions...
mripard: <u>hverkuil</u>: I made some good progress recently to split out the drm_format_info structure out of DRM so that v4l drivers could use it at least
bbrezillon: <u>pinchartl</u>: hm, they're exposed to userspace, so I'm not sure it's even doable
hverkuil: <u>bbrezillon</u>: I don't think we want to change drm. After all, we are the ones designing a new API, so it is much easier for us to incorporate drm ideas.
pinchartl: <u>mripard</u>: how does that work, with different 4CCs ?
<br> <u>bbrezillon</u>: I know... :-(
mripard: work is basically done in DRM, I need to convert to one v4l2 driver to see how it could integrate there
<br> <u>pinchartl</u>: yep :/
hverkuil: <u>pinchartl</u>: the best you can do there is make a mapping. Or make completely new defines that both drm and v4l use.
pinchartl: <u>bbrezillon</u>: we could, if we create new ioctls, switch to the DRM 4CCs for the ones that conflict, but it would be quite messy
hverkuil: and redefine the existing drm/v4l format defines to the new shared defines.
bbrezillon: <u>pinchartl</u>: well, modifiers is a new thing in V4L, so that once can easily be shared
<br> and yes, for the new API we could use DRM's 4CC
pinchartl: <u>bbrezillon</u>: regarding data_offset, this maps to the DRM framebuffer offsets, I think it's needed. we may also need a header size or similar field for the existing use cases of data_offset
bbrezillon: and do the conversion in V4L
<br> <u>pinchartl</u>: hverkuil wanted to create a new field for that
<br> instead of re-using the old one
<br> so data_offset would still be used for header offsets
<br> and another field would be created for buffer offsets (buf_offset)
pinchartl: I *think* we need two fields
<br> and if we create a new ioctl
hverkuil: call it something else in the new API. Just redesign how to define the memory layout.
pinchartl: we're free to use the names we want for them
<br> I4m sure we'll bikeshed the naming
hverkuil: <u>pinchartl</u>: right.
<br> data_offset was just a bad idea (and a bad name).
bbrezillon: ok, hdr_offset and buf_offset
<br> and I'll let you bikeshed to death when the time comes ;-)
<br> I'll just focus on the PoC for now, and get back to you once I have something that's worth sharing
<br> thank you all for your inputs
hverkuil: You're welcome! It's good to see someone picking this up where I left it.
bbrezillon: <u>mripard</u>: and so I guess I'm interested in the drm_format_info split you worked on ;)
<br> if you have it pushed somewhere that'd be great
pinchartl: <u>bbrezillon</u>: we'll bikeshed on names, but please make sure you patches contain a clear and detailed documentation of how those fields are used, especially in conjunction
hverkuil: <u>tfiga</u>: on re-reading https://hverkuil.home.xs4all.nl/codec-api/uapi/v4l/dev-encoder.html it states at the end of step 3: 'Setting the source resolution will reset the selection rectangles to their default values, based on the new resolution, as described in the step 5 below.'.
<br> <u>tfiga</u>: note that 'step 5' should be 'step 4'
<br> <u>tfiga</u>: this is at least partially documenting the behavior. I think it also needs to explicitly document that the crop rectangle will be set to the width and height of the format before width and height are adjusted by the driver to comply to alignment restrictions.
<br> <u>tfiga</u>: I'll just reply to the v3 encoder spec post.
mripard: <u>bbrezillon</u>: I wanted to send it some time next week, I can push a branch if you want
dafna2: paulk-leonov: Hi, I have a question regarding the stateless cedrus encoder. I implement now the stateless vicodec driver. I already implemented the decoder. But I have a problem with the stateless encoder. the stateful decoder saves the last decoded frame in an inner private buffer to be used as a reference frame for the next p-frame. In the stateless encoder the decoded frame is not saved, so I replace it with the last frame
<br> (the original raw frame from userspace) , but this does not work, the resulting video looks bad https://pasteboard.co/I497fB7.png
<br> paulk-leonov: do you know how the cedrus driver handles that ?
tfiga: <u>hverkuil</u>: thanks!
bbrezillon: <u>mripard</u>: yep, this way I can base my work on top
paulk-leonov: dafna2, hi! Well, we haven't dealt with the encoding part at all so far, so I'm not very familiar with the issue
<br> Is the issue that nothing guarantees that previous frames are still available to the encoder when processing a subsequent frame or did I misunderstand?
***: svarbanov has quit IRC (Ping timeout: 246 seconds)
hverkuil: paulk-leonov: as I understand it, the encoder is supposed to derive P/B frames not from the original uncompressed frame, but from a compressed-and-then-decompressed frame. Since the decoder will also use a decoded frame as the reference frame.
<br> But that implies that a stateless encoder needs to keep a buffer somewhere with the 'compressed-and-then-decompressed' frame.
paulk-leonov: oh
<br> I definitely didn't think of that
hverkuil: I'm very curious how cedrus (and also rockchip) encoders handle that.
<br> Internal memory? Scratch buffer somewhere?
paulk-leonov: our VPU does have some SRAM available, but I doubt it stores full frames there
mripard: <u>bbrezillon</u>: https://github.com/mripard/linux/tree/drm-v4l-formats
bbrezillon: <u>mripard</u>: thanks
mripard: as said, I haven't tackled the v4l2 sides of things yet
<br> but that should get you started
hverkuil: <u>tfiga</u>: ^^^^^^ do you know how the rockchip stateless encoder implements this? (see discussion with paulk-leonov)
mripard: you'd just need to add the v4l2 fourcc and a lookup function
paulk-leonov: hverkuil, looking at https://github.com/uboborov/ffmpeg_h264_H3/blob/master/sunxi/h264enc.c it does allocate extra buffers for reference
hverkuil: internally in the driver?
<br> ah, that's userspace code.
bbrezillon: <u>mripard</u>: nice!
<br> AFAIU, pinchartl was also suggesting to use the DRM 4CCs for the new v4l2_format APIs and do the conversion in V4L
hverkuil: paulk-leonov: I suspect that this is something the driver might have to do. The format that the encoder uses for those decoded reference frames is probably custom anyway (some tiled format, I expect)
<br> HW specific, and therefor a task for the driver.
paulk-leonov: hverkuil, indeed, there is good chance that this is the case
<br> I have nothing against letting the driver manage that on its own
hverkuil: makes sense.
<br> <u>dafna2</u>: so the best solution is that vicodec will allocate an internal buffer that contains the 'compressed-then-decompressed' reference frame. It can use an internal macroblock format, though, so no need to convert it back to the output pixelformat.
mripard: <u>bbrezillon</u>: I didn't know that was an option, but that could work too :)
hverkuil: <u>bbrezillon</u>: I don't know if that's an option. Or even something we want.
bbrezillon: I'm fine with mripard's approach, which I find elegant
<br> just saying that it does not match what pinchartl proposed ;)
<br> <u>mripard</u>: have you already thought about passing modifiers?
hverkuil: paulk-leonov: by having the encoder driver allocating the reference frame buffers, we don't need to refer to reference frames at all anymore. That simplifies the API for stateless encoders.
paulk-leonov: hverkuil, IIRC I was told by people who worked on the cedrus encoder earlier that it should be similar to a stateful interface
<br> which makes sense as far as I can see
mripard: <u>bbrezillon</u>: I was mostly interested in the subsampling, planes, etc parameters, so I haven't really think about it yet
bbrezillon: ok, it's kind of orthogonal to the pixel format anyway (both are needed when dealing with tiled-formats)
<br> <u>mripard</u>: can you Cc me on this series?
-: kbingham spots talk about unifying DRM/V4L2 formats and glances around with interest.
hverkuil: paulk-leonov: it looks like you might be right. So the driver would just return e.g. the mpeg2 parameters using the request API so that userspace can create the bitstream, but otherwise it is really the same as the stateful encoder.
mripard: <u>bbrezillon</u>: sure
<br> <u>bbrezillon</u>: on your collabora address or kernel.org
<br> ?
bbrezillon: collabora
paulk-leonov: hverkuil, I wonder what the userspace interface for the output should be -- there may be cases where the hardware can construct the bitstream on its own
<br> also our current codec controls were explicitly tailored for decoding, so that would have to change if we want to reuse them for encoding
<br> s/userspace interface for the output/userspace interface for the destination/
mripard: <u>bbrezillon</u>: ack
hverkuil: paulk-leonov: if the HW can construct the bitstream, then the driver should just use the stateful encoder API. In that case there is no need to be stateless.
paulk-leonov: yes fair enough
hverkuil: paulk-leonov: I don't think you need to change anything for the stateless encoder w.r.t. the controls. The only change is that the timestamp to refer to reference buffers is unused.
<br> Which suggests that this should perhaps be in a separate control.
<br> Then encoders wouldn't have to support that.
paulk-leonov: I see
<br> Well, I'll have to do some more investigation on how our h264 encoder works to really have a clear idea of this
hverkuil: definitely.
paulk-leonov: it seems that some bitstream parameters also need to be set before triggering decoding
***: griffinp has quit IRC (Quit: ZNC - http://znc.in)
<br> vkoul has quit IRC (Quit: ZNC 1.7.0+deb0+trusty1 - https://znc.in)
<br> tonyk has left
tonyk: <u>hverkuil</u>: why vivid doesn't support both single and multiplanar at same time? is this a requirement from V4L2?
hverkuil: yes, that's a V4L2 requirement.
<br> Something I would like to get rid of eventually.
bbrezillon: <u>hverkuil</u>: for the timestamp in v4l_buffer, we should use __kernel_timespec, right?
hverkuil: <u>bbrezillon</u>: no, use __u64 (ns since 1/1/1970).
bbrezillon: <u>hverkuil</u>: I see you've used an __u64, was it because __kernel_timespec was not defined at that time
hverkuil: we use that for all new APIs.
bbrezillon: ok
hverkuil: No, it's simply because it is much easier to manipulate, and there are no problems with y2038.
<br> Actually, the timestamp in v4l2_buffer is from CLOCK_MONOTONIC, so it is since boot time.
koike: <u>hverkuil</u>: regarding supporting multiplanar and single planar at the same time, at least from the docs it looks that the driver can support both at same time https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/planar-apis.html#planar-apis " An application can choose whether to use one or the other by passing a corresponding buffer type to its ioctl calls", I didn't find in the docs anything about
<br> V4L2_CAP_VIDEO_CAPTURE and V4L2_CAP_VIDEO_CAPTURE_MPLANE being multually exclusive, should we update the docs?
hverkuil: There are no drivers that allow both. I think the original intention was to be able to do that, but I think it simply became too complex.
<br> There may also have been a technical reason for it, I'm not sure.
<br> A new streaming API should fix this in some way.
tonyk: <u>hverkuil</u>: should I write single-planar or single plane? or are both correct?
hverkuil: single-planar
tonyk: and when I want to have a "compressed" name, it's correct to use splanar and mplanar (for instance, in function names)
hverkuil: yes, actually you'll often see sp_ or mp_ prefixes (or suffixes) being used.
bbrezillon: <u>hverkuil</u>: format/modifier settings should still happen separately, right?
hverkuil: yes, that goes through the FMT ioctls. I think I mentioned earlier today that I suspect those also need a new version.
bbrezillon: through VIDIOC_{S,G}_EXT_FMT if we want to change the ioctl
<br> yep
<br> well, if we want to pass the modifiers, yes
hverkuil: Since with the new streaming ioctls we also want to get rid of the single vs multiplanar mess, it makes no sense to still have to keep it in the FMT ioctls.
<br> And frankly, both struct v4l2_format and struct v4l2_buffer are badly designed (complex 32 bit compat code) and it has become hard to add new information.
bbrezillon: v4l2_buffer, I'm working on it based on your proposal
<br> what do you have in mind for v4l2_format
<br> apart from getting rid of the MPLANE vs !MPLANE distinction?
hverkuil: ditch the difference between single and multiplanar. Rework the layout of the fields to avoid compat32 code, probably enlarge the struct,
<br> One thing to research is adding crop and compose rectangles to the video format. The interaction between setting a format and setting crop/compose rectangles is complex and perhaps setting them all in one go makes it easier to use. Or perhaps not. Something to look at.
pinchartl: <u>hverkuil</u>: very good idea
hverkuil: And obviously it should be able to handle and report the more complex memory layouts that v4l2_ext_buffer will also support.
bbrezillon: hm, so you'd describe that in both places?
<br> or just in the v4l2_format?
hverkuil: <u>bbrezillon</u>: it's a good question.
bbrezillon: I mean, offsets and stride could be a per-buffer thing
<br> but I'd guess you'd have to reconfigure the HW when things change, which is probably not so efficient
pinchartl: not all hardware may be able to handle per-buffer stride
hverkuil: Mostly it should be part of v4l2_ext_format.
pinchartl: but some may
<br> this should be taken into consideration, and if we don't support per-buffer stride, that should be documented at least in commit logs, with the rationale
bbrezillon: what about per-buffer offsets ?
hverkuil: But some things (e.g. the size of a header before the active video starts) might be variable.
pinchartl: I don't know if there are use cases for per-buffer stride
<br> per-buffer offsets, on the other hand, are needed
hverkuil: I've never seen variable strides. I can't think of a use case either.
<br> <u>pinchartl</u>: I agree
bbrezillon: I don't see any
pinchartl: if we have per-buffer offset we could possibly implement compose support in the V4L2 core
hverkuil: It's basically what data_offset attempted to do (badly).
<br> <u>pinchartl</u>: can you explain?
pinchartl: composing, with a DMA engine that supports configurable stride, is just a matter of setting the stride to the buffer width, and setting the DMA address to buffer address + compose top * stride + compose left
hverkuil: ok, but that's driver specific, so what does this have to do with the v4l2 core?
pinchartl: it's not driver-specific
<br> computing buffer address + compose top * stride + compose left could be done in the core
<br> the driver would only see DMA address, image width, buffer stride
<br> and wouldn't need to handle the compose rectangle explicitly
hverkuil: Ah, the precompute step is in v4l2-core, not the actual programming of the DMA engine. Now I get it.
pinchartl: of course :-)
<br> my point is that the driver doesn't need to be aware of compose to program the DMA engine
<br> so I wonder if we could keep compose handling fully out of drivers
<br> possibly not if we have to scale
<br> to be researched
<br> because right now most drivers don't support compose, while they could
<br> maybe we don't even need to specify the compose rectangle from userspace if we have data offset (userspace would compute buffer address + compose top * stride + compose left itself for that) and stride
bbrezillon: let me summarize:
<br> v4l2_ext_format -> add stride and modifiers + rework things pointed by hverkuil
<br> v4l2_ext_buffer -> add buf_offsets and make sure multi plane and single plane formats are handled in a consistent way
hverkuil: note that v4l2_pix_format already has a stride (bytesperline).
shuah: <u>hverkuil</u>: I am working on patches - sorry travel and conference is adding time
hverkuil: <u>shuah</u>: no problem. You have the whole 5.2 cycle to get this merged :-)
shuah: <u>hverkuil</u>: I suspected as much. Thanks :)
bbrezillon: <u>hverkuil</u>: I was planning on extending v4l2_pix_format_mplane (or create a new type) that would work for single planar formats too