Le jeudi 13 juin 2019 à 08:48 +0200, Hans Verkuil a écrit : > On 6/3/19 1:28 PM, Hans Verkuil wrote: > > Since Tomasz was very busy with other things, I've taken over this > > patch series. This v4 includes his draft changes and additional changes > > from me. > > > > This series attempts to add the documentation of what was discussed > > during Media Workshops at LinuxCon Europe 2012 in Barcelona and then > > later Embedded Linux Conference Europe 2014 in Düsseldorf and then > > eventually written down by Pawel Osciak and tweaked a bit by Chrome OS > > video team (but mostly in a cosmetic way or making the document more > > precise), during the several years of Chrome OS using the APIs in > > production. > > > > Note that most, if not all, of the API is already implemented in > > existing mainline drivers, such as s5p-mfc or mtk-vcodec. Intention of > > this series is just to formalize what we already have. > > > > Thanks everyone for the huge amount of useful comments to previous > > versions of this series. Much of the credits should go to Pawel Osciak > > too, for writing most of the original text of the initial RFC. > > > > This v4 incorporates all known comments (let me know if I missed > > something!) and should be complete for the decoder. > > > > For the encoder there are two remaining TODOs for the API: > > > > 1) Setting the frame rate so bitrate control can make sense, since > > they need to know this information. > > > > Suggested solution: require support for ENUM_FRAMEINTERVALS for the > > coded pixelformats and S_PARM(OUTPUT). Open question: some drivers > > (mediatek, hva, coda) require S_PARM(OUTPUT), some (venus) allow both > > S_PARM(CAPTURE) and S_PARM(OUTPUT). I am inclined to allow both since > > this is not a CAPTURE vs OUTPUT thing, it is global to both queues. > > Alternative proposal: > > 1) Add support for fractions (struct v4l2_fract) as a control type: > V4L2_CTRL_TYPE_FRACT. > > 2) Add a new V4L2_CID_MPEG_FRAME_INTERVAL control. Is the MPEG namespace historical ? That might be confusing for users. > > Encoders shall support this control. > > 3) For backwards compatibility reasons encoder drivers still have to > support G/S_PARM, but this can now be implemented by standard helpers > that query this control. Drivers also have to implement ENUM_FRAMEINTERVALS. That's won't be very friendly for UI generator like qv4l2. Support for v4l2_fract as control should include a way to describe the supported values of that control the usual way I think. Also, long term, it would be nice to have two sets of frame rates. The one that the HW can handle "real-time" and the one that can be used for bitrate calculation. So staying away from ENUM_FRAMEINTERVALS for bitrate configuration would be nicer. > If the range of intervals is always the same regardless of the frame size, > then a helper can be used that queries the min/max/step of the control, but > if it is dependent on the frame size, then it has to be implemented in the > driver itself. > > I'm sticking to frame intervals instead of frame rates for the simple reason > that that's what V4L2 has used since the beginning. I think it is too confusing > to change this to a frame rate. This is just my opinion, though. I suggested frame rate since this is what I saw implemented by HW registers (if you think it's worth it, I can try and make a list). Also, frame-interval steps are not compatible with frame-rate steps (something that was raised through a venus driver bug) last year. Even v4l2-ctl was displaying that in a very confusing way. Something as simple as 1 to 30 fps cannot be exposed through ENU_FRAMEINTERVALS. You are forced to expose the full fractional range of interval, from 1/30 to 1/1. For Venus it was not that much of a trouble, since its stores a framerate as Q16.. > > I also chose to make this a codec control, not a generic user control: this > value together with the bit rate control(s) determine the compression size, > it does not determine the actual time it takes for the encoder to compress > the raw frames. Hence it is really not the same thing as the frame interval That's a good point. > of a video capture device. If we want to use a control for that as well in > the future as a replacement for G/S_PARM, then that should be a new control. > And we would like need per-pad controls as well in order to implement that. > Which is a lot more work. > > Regards, > > Hans
