(Added Nicolas as I'd like his input as well) Reply below: On 12/20/19 2:47 PM, Michael Tretter wrote: > 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 think you raise good points. So this means that we need this new control (required for stateful encoders) and optionally allow S_PARM to be used as an alternative way to set the same control. I prefer to make S_PARM optional and require the control, since I hate S_PARM :-) It would mean that all existing stateful encoders need to implement this new control, but I think that's a good idea anyway. > >> 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 > Thanks for working on this! Happy New Year, Hans
