| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When | |
|---|---|---|---|
| hverkuil | tfiga: ping | [06:48] | |
| tfiga | hverkuil: pong | [06:48] | |
| hverkuil | tfiga: back from vacation? | [06:48] | |
| tfiga | yep, almost caught up | [06:48] | |
| hverkuil | Hope you had a good time, and are back with your batteries all charged up :-)
Do you think you'll be able to post a new version of the stateful codecs specs this/next week? | [06:50] | |
| tfiga | hverkuil: yeah, I'm doing all my best to get back to the spec asap | [06:50] | |
| hverkuil | Excellent. | [06:51] | |
| tfiga | thanks for reviewing Mediatek ISP patches
that's actually one of my significant time consumers | [06:51] | |
| hverkuil | I have a lot of patches for vicodec and v4l2-compliance pending, and I started preliminary work in trying to extend v4l2-mem2mem.c to provide more core support for some of the trickier corner cases that are hard for drivers to implement. Having a new version of the spec will help with that work.
I actually wanted to ask about mediatek: there are a bunch of different drivers, but it's a bit hard to get a good overview and what the overall status is. And in what order they want to merge them. | [06:52] | |
| tfiga | hverkuil: the overall status is as the patch revision suggets, RFC :)
especially the metadata needs a lot of work | [06:54] | |
| pinchartl | tfiga: we would benefit from a request API there, wouldn't we ? | [06:55] | |
| tfiga | we want to get everything else into a good shape first | [06:55] | |
| pinchartl | hverkuil: apart from adding support for more V4L2 formats, do you see something missing in "[PATCH 00/20] drm: Split out the formats API and move it to a common place" to make it usable for V4L2 ? | [06:56] | |
| tfiga | pinchartl: yes, and they're implementing it there | [06:56] | |
| hverkuil | pinchartl: no, I am fine with that series. And adding more v4l2 formats can be done in follow-up patches. | [06:56] | |
| pinchartl | hverkuil: ok, thanks. I think we should try to share format documentation in a common place, but that can also go on to
top tfiga: but without formats and selection rectangles, right ? | [06:57] | |
| hverkuil | pinchartl: you mentioned that you wanted to work on the control framework. What area exactly? I do like to have an idea of what you are planning to do :-) | [06:59] | |
| pinchartl | :-)
basically I'd like to be able to implement a state object (we've discussed that before) and that state object will store the values of the controls (among other things) but the values only and unless I've misunderstood the current request support implementation in the control framework, it's hard to dissociate the two so I'd like to work on that separating the description of a control from its state (which would be value + some other information, such as various flags) | [06:59] | |
| hverkuil | it's not so easy, primarily due to historical reasons. For controls with a payload it is actually already more-or-less in place (p_new and p_cur point to the values), although the values are allocated directly after the struct v4l2_ctrl, but that can be changed,
The problem is this bit in struct v4l2_ctrl: Â Â Â Â Â Â Â s32 val; Â Â Â Â Â Â Â struct { Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â s32 val; Â Â Â Â Â Â Â } cur; For simple s32 control types (i.e. 90+% of all controls) this is what drivers actually use, they don't go through p_cur or p_new. | [07:05] | |
| pinchartl | yes, I know it won't be easy
and it will likely be intrusive I'll try to find the least painful way | [07:06] | |
| tfiga | pinchartl: right, just buffers... | [07:06] | |
| pinchartl | and least intrusive | [07:07] | |
| tfiga | hverkuil: we would like to just get the code into good shape first | [07:07] | |
| hverkuil | If you want to separate values from the control step one would probably be that all drivers need to switch to using p_cur and p_new. I'm not opposed to that at all, but it is a lot of work. | [07:07] | |
| tfiga | metadata is going to take some time | [07:07] | |
| pinchartl | hverkuil: I'll try to do it :-)
and I was thinking about introducing inline helpers to access the value through p_cur and p_new, so that the structures could then be changed without affecting drivers | [07:07] | |
| hverkuil | tfiga: OK, no problem. Let me know when you want me to review more mediatek code. | [07:08] | |
| pinchartl | (one tree-wide patch is more than enough :-)) | [07:08] | |
| hverkuil | pinchartl: the good news is that you don't need to do this in a big-bang patch, but can fix each driver at a time. | [07:09] | |
| pinchartl | hverkuil: you're right, yes
ok, it's a tree-wide series, not a tree-wide patch which is better :-) | [07:09] | |
| hverkuil | Note that p_cur and p_new are already fine for simple integer controls, it's just that drivers use the val/cur.val fields directly. | [07:10] | |
| pinchartl | yes, but with inline accessors, it will be easier to then refactor the implementation in the backend if needed | [07:10] | |
| hverkuil | But I do agree that it is probably better to have accessors, yes. | [07:10] | |
| pinchartl | my very first step is to find time to work on this
after that I will let you know before touching the code anyway :-) | [07:12] | |
| hverkuil | Post an RFC patch with the proposed accessors first before starting the conversion of drivers. | [07:14] | |
| pinchartl | ok | [07:14] | |
| hverkuil | I agree that this would be a worthwhile job. I thought about doing it in the past, but there just was no real need for it.
We probably want two accessors that return a union v4l2_ctrl_ptr (one for the current and one for the new value), and two that build on top of that to return a s32. The vast majority of controls are of type s32, so that should have a simple accessor. | [07:14] | |
| pinchartl | yes, that sounds good to me | [07:17] | |
| hverkuil | Hmm, you'll need to provide a setter for type s32 as well for the same reason.
I don't think cur.val is ever set (in the rare cases that might happen it's OK to do that through a v4l2_ctrl_ptr) Having accessors can't hurt anyway. Adding those, then converting the virtual drivers to use them would be a first step. That's easy enough to merge. tfiga: just let me know if you need help with the stateful codec specs (or anything else related to that). | [07:18] | |
| tfiga | hverkuil: as for the order of drivers, seninf and P1 can only work together, so there is probably little point in getting them merged separately
DIP and FD are entirely m2m, so they could go separately but cleaning up the metadata is going to take some time... | [07:28] | |
| hverkuil | OK | [07:29] | |
| tfiga | hverkuil: if you end up with some time to spend on some review, I'd appreciate taking a look at the P1 driver code
I made 1 round of review before, but I'm sure there are still some problems left | [07:30] | |
| hverkuil | I did look at it a bit yesterday, but I'll see if I can do some more reviews today. | [07:33] | |
| *** | javier__ has left | [07:41] | |
| ........... (idle for 52mn) | |||
| tfiga | hverkuil: actually, forget about that P1 driver, because there is an m2m driver waiting on someone's review and that would likely have to be me...
| [08:33] | |
| hverkuil | ok | [08:34] | |
| tfiga | in the meantime, I just sat back to the codec spec
and a question: what do we do with CAPTURE sizeimage of the encoder? | [08:35] | |
| hverkuil | The minimum sizeimage depends on the width/height of the CAPTURE format, but userspace can provide larger values as well.
We're waiting for a v3 of the patch that modifies the spec w.r.t. sizeimage accordingly. (from Stanimir) | [08:36] | |
| tfiga | hverkuil: do you mean OUTPUT? | [08:38] | |
| hverkuil | No, CAPTURE. At least, I seem to remember that we decided not to copy width/height from OUTPUT to CAPTURE for encoders (may be wrong, I'd have to read up on it) | [08:39] | |
| tfiga | I think we ended up with the width/height of CAPTURE to mean the coded size of the stream and that's definitely a function of the source | [08:42] | |
| hverkuil | You are right.
The last suggestion was to have a similar SOURCE_CHANGE event (I actually think we can re-use it) whenever the OUTPUT changes in such a way that the CAPTURE buffers need to be drained and re-allocated. Which makes sense to me. It's nicely symmetrical with the decoder as well. Hmm, I'll reply to the sizeimage patch. I wonder why we can't just use VIDIOC_CREATEBUFS if you want larger buffers. | [08:45] | |
| tfiga | hverkuil: so the question is if we should disallow OUTPUT format changes when CAPTURE has buffers allocated
I wonder if that wouldn't break some apps or maybe not, because the initialization clearly sets both formats first but I wonder about gstreamer and ffmpeg ndufresne: any idea? | [09:03] | |
| hverkuil | I don't think there are any stateful encoders that allow OUTPUT format changes while OUTPUT buffers are allocated. Midstream resolution changes in an encoder aren't possible today AFAIK. | [09:07] | |
| tfiga | right, but having CAPTURE buffers allocated is not midstream yet
CAPTURE streaming could be considered midstream, though obviously having buffers allocated on OUTPUT locks down the OUTPUT format, because of the API design :P) | [09:08] | |
| hverkuil | I think if the new OUTPUT format would make the CAPTURE buffers too small, then that would effectively be a 'mid'stream change. Drivers can reject that (-EBUSY) if they don't support resolution changes, or accept it, but then when you start streaming you'll get a SOURCE_CHANGE event and an empty CAPTURE buffer marked LAST.
I'm OK with postponing describing resolution changes for an encoder for the first version. I don't think it is common functionality (but correct me if I am wrong). | [09:10] | |
| tfiga | agreed on not describing it yet
but we need to be clear on the behavior of format changes hmm -EBUSY for too small buffers could technically work, but sounds strange okay, let's just call it another way argh, let me think about this for a moment | [09:17] | |
| hverkuil | I think vicodec just accepts it, but when encoding it discovers that the CAPTURE buffer is too small and then both OUTPUT and CAPTURE buffer are returned with the ERROR flag set. | [09:20] | |
| tfiga | to be honest, I'd prefer that behavior in the spec
then the userspace is given full freedom to choose the buffer size if it's too small, it could retry | [09:21] | |
| hverkuil | No, I'm wrong. It returns EBUSY if the size of the CAPTURE format is too small. | [09:23] | |
| tfiga | it would be also somewhat consistent with the decoder, because the OUTPUT sizeimage there would be always controlled by the userspace | [09:24] | |
| hverkuil | tfiga: so you prefer returning EBUSY as vicodec does today?
(sorry, I'm a bit confused) lunch | [09:26] | |
| ...... (idle for 28mn) | |||
| tfiga | hverkuil: no, I prefer not to have the OUTPUT format change the CAPTURE sizeimage
but I guess that's lost already :) | [09:59] | |
| hverkuil | tfiga: ah, OK. Treat the CAPTURE and OUTPUT formats independently, just as for the decoder. | [10:03] | |
| ................................................. (idle for 4h3mn) | |||
| mjourdan: ping | [14:06] | ||
| mjourdan | hverkuil: pong | [14:07] | |
| hverkuil | mjourdan: congrats on resolving the fw situation! That's good news. | [14:08] | |
| mjourdan | Yes, we're very happy ! Was quite the hassle so glad that it's out of the way :) | [14:08] | |
| hverkuil | I was wondering if you can test this driver using v4l2-compliance from my vicodec branch: https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=vicodec
You would run it with the '--stream-from foo.mpg' option. That v4l2-compliance version is close to what we hope will be the final stateful codec spec. I'd be interested in the test result. BTW, you also need to provide the -s option, otherwise it won't stream from the mpg file. | [14:09] | |
| mjourdan | sure, will do. It's cool that compliance is extended to decoder tests.
Will the fact that I don't support the SRC_CHANGE event with MPEG2 be a problem ? (thus the V4L2_FMT_FLAG_FIXED_RESOLUTION patch within the series) | [14:12] | |
| hverkuil | No, that's not a problem/
It would only be a problem if the mpg file would in fact contain a resolution switch. | [14:13] | |
| mjourdan | Okay, I was referring to this step of the spec: "This step only applies to coded formats that contain resolution information in the stream. Continue queuing/dequeuing bitstream buffers to/from the OUTPUT queue via VIDIOC_QBUF() and VIDIOC_DQBUF(). The buffers will be processed and returned to the client in order, until required metadata to configure the CAPTURE queue are found. This is indicated by the decoder sending a V4L2_EVE
SOURCE_CHANGEÂ event withV4L2_EVENT_SRC_CH_RESOLUTIONÂ source change type." which, from my testings with the chromium v4l2 stack (although with h264 - not mpeg2), holds the capture queue on hold until such an event is signaled by the driver | [14:19] | |
| ....... (idle for 34mn) | |||
| hverkuil: looks like I'm getting a few fails so far (https://pastebin.com/zEhWucpJ). I don't think a decode session happened, maybe you stop before if there are fails ? | [14:56] | ||
| jernej | ndufresne: Do you know for any public documentation of Google's reference VP9 decoding IP core? That's the one used on Allwinner H6. | [15:01] | |
| ........ (idle for 35mn) | |||
| hverkuil | mjourdan: regarding the first fail: just copy the try_decoder_cmd code from the vicodec driver in https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=vicodec
That code will move to v4l2-mem2mem.c eventually since it will be the same for all codecs. It does try to stream, but it fails immediately on the wrong field value. | [15:36] | |
| mjourdan | Okay for the first one
I've been trying to point out where those fields errors come from but I'm not sure I do set vbuf->field = V4L2_FIELD_NONE, but that's only when I give the buffer back to userspace. Do I need to set it somewhere else ? (on output queue) | [15:38] | |
| hverkuil | Also set it in buf_prepare. VIDIOC_QBUF calls that, and that's where you can set field to FIELD_NONE.
Look in e.g. vicodec_buf_prepare(). | [15:40] | |
| mjourdan | Ah I see. I'm using v4l2_m2m_ioctl_prepare_buf as of now. Will this change also be moved to v4l2 m2m ? | [15:41] | |
| hverkuil | I meant the buf_prepare callback of struct vb2_ops. | [15:42] | |
| mjourdan | oh, right.
(sorry, it gets a bit confusing at time :D) | [15:42] | |
| hverkuil | ttyl | [15:44] | |
| jernej | hverkuil: I'm working on M2M deinterlace driver for Allwinner SoCs and I wonder if currently the only possibility is to have single input queue with buffer holding both fields?
it would be easier for userspace to provide both fields in separate buffers | [15:44] | |
| *** | benjiG has left | [15:58] | |
| ........ (idle for 39mn) | |||
| hverkuil | jernej: two buffers, each with a field, are fine. I think there is a deinterlacer in mainline already that does this. | [16:37] | |
| jernej | hverkuil: can you please point me to it? I only found one m2m deinterlace driver, which accepted only one buffer with both fields | [16:39] | |
| ................... (idle for 1h32mn) | |||
| ndufresne | jernej, not atm, that would do you have a reg file to look at, something it helps as often the doc don't say which design it it documenting | [18:11] | |
| jernej | ndufresne: I'm not sure what you mean | [18:16] | |
| ndufresne | is there any vendor driver existing ?
or are we totally in the blue ? jernej, do you remember the name of the chip company they bought ? | [18:27] | |
| jernej | ndufresne: driver exist in form of a blob
let me check company name | [18:31] | |
| ndufresne | jernej, do you know if this is referring to the same IP, https://www.webmproject.org/hardware/vp9/ | [18:31] | |
| jernej | yes, it is | [18:31] | |
| ndufresne | and G2, is that for Hantro G2 %? | [18:32] | |
| jernej | maybe, yes | [18:32] | |
| ......................................................... (idle for 4h42mn) | |||
| *** | Whoopie has quit IRC (Ping timeout: 245 seconds) | [23:14] | |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |