Hello Hans, On Tue, 19 Nov 2019 12:34:52 +0100, Hans Verkuil wrote: > This series adds support for fractions in the control framework, > and a way to obtain the min and max values of compound controls > such as v4l2_fract. > > Next it adds the V4L2_CID_MPEG_VIDEO_ENC_FRAME_RATE control to > set the framerate for the encoder. > > The next patch adds support for the V4L2_BUF_FLAG_TOO_SMALL flag > to signal that the capture buffer was too small. > > The final patch adds the encoder spec (unchanged from v3). > > Michael, can you add support for V4L2_CID_MPEG_VIDEO_ENC_FRAME_RATE > to your encoder driver? Let me know if something isn't working. I implemented the control and hooked it up with S_PARM as well. The implementation was straightforward without any real issues. I'll send a patch in reply to this mail. Having a control for configuring the frame rate that is encoded into the SPS feels correct. This is in line with configuring the bitrate, level, etc. However, after seeing the implementation and fiddling around with it in userspace, I am not convinced that S_PARM should be used signal the rate at which frames are submitted. Setting the frame submission rate to something different than the frame rate of the stream would be most interesting for transcoding use cases. The user space would either want to run the encoding as fast as possible or, if there are multiple encoding processes, as fast as possible with properly shared resources. Boiling this information down into a single number (and calling is "rate at which frames are submitted") sounds kind of wrong, because the userspace does not know which submission rate would lead to a good result. Using the frame rate for such a setting seems pretty unique to the allegro encoder. Other encoders might use different mechanisms to boost the encoding speed, e.g., might be able to temporarily increase the clock rate of the codec. In these cases, the driver would need to translate the "framerate" set via S_PARM to a clock rate for the codec. This does not sound right. However, in the end, this would lead to exposing single parameters that allow to tune the codec via generic controls. This does not seem to be the right way, at all. Maybe we could have a control that tells the encoder to "run as fast as possible" or to "run with as little resources as possible", which would be applicable to more encoders and the driver would have to decide how to implement this "profile". Still, I am not really sure, if this is the proper way to solve this. > I need to add a test control for this to vivid as well and add support > for this to v4l2-ctl, that's on my TODO list. > > Open questions: > > 1) Existing encoder drivers use S_PARM to signal the frameperiod, > but as discussed in Lyon this should be the rate at which frames are > submitted for encoding, which can be different from > V4L2_CID_MPEG_VIDEO_ENC_FRAME_RATE. But given the current use-cases > I was wondering if calling S_PARM should set the ENC_FRAME_RATE > control as well, and you would need to explicitly set the control > after S_PARM if the two are not the same. This would mean that > existing applications that don't know about the control can keep working. In the patch I did exactly that and we should be backwards compatible to applications that use only S_PARM. Michael
