| Message ID | 20191119113457.57833-1-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| Headers | show |
| Series | Stateful Encoding: final bits | expand |
Hi 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. Thanks. Add will add the control and send patches. > 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. I am slightly confused, because I thought that S_PARM and V4L2_CID_MPEG_VIDEO_ENC_FRAME_RATE should be used exactly the other way around, but it makes sense to use S_PARM for configuring the frame rate for producing frames in the hardware. For encoding live the rate at which frames are submitted to the encoder is the same as the framerate of the produced stream. They only differ for use cases where we want to encode faster or slower than the playback rate of the resulting stream. I guess assuming that they are by default the same and only adapt the control if necessary should be fine. Michael > > 2) I do believe that we need a RELEASE/DEL/DESTROY_BUFS ioctl in > order to do proper handling of the V4L2_BUF_FLAG_TOO_SMALL case. > I am inclined to postpone adding this flag until we have that ioctl. > We can still merge the stateful encoder spec if we mention that that > is work in progress. > > Regards, > > Hans > > Hans Verkuil (4): > v4l2-ctrls: add support for v4l2_fract types > v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL > v4l2-controls.h: add V4L2_CID_MPEG_VIDEO_ENC_FRAME_RATE > videodev2.h: add V4L2_BUF_FLAG_TOO_SMALL flag > > Tomasz Figa (1): > media: docs-rst: Document memory-to-memory video encoder interface > > Documentation/media/uapi/v4l/buffer.rst | 9 + > Documentation/media/uapi/v4l/dev-encoder.rst | 608 ++++++++++++++++++ > Documentation/media/uapi/v4l/dev-mem2mem.rst | 1 + > .../media/uapi/v4l/ext-ctrls-codec.rst | 8 + > Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 5 + > Documentation/media/uapi/v4l/v4l2.rst | 2 + > .../media/uapi/v4l/vidioc-encoder-cmd.rst | 51 +- > .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 15 +- > .../media/uapi/v4l/vidioc-queryctrl.rst | 6 + > .../media/videodev2.h.rst.exceptions | 3 + > .../media/common/videobuf2/videobuf2-core.c | 12 +- > .../media/common/videobuf2/videobuf2-v4l2.c | 4 + > drivers/media/i2c/imx214.c | 4 +- > drivers/media/v4l2-core/v4l2-ctrls.c | 222 ++++++- > include/media/v4l2-ctrls.h | 72 ++- > include/media/videobuf2-core.h | 4 + > include/uapi/linux/v4l2-controls.h | 1 + > include/uapi/linux/videodev2.h | 6 + > 18 files changed, 980 insertions(+), 53 deletions(-) > create mode 100644 Documentation/media/uapi/v4l/dev-encoder.rst >
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
(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
Le lundi 06 janvier 2020 à 16:02 +0100, Hans Verkuil a écrit : > (Added Nicolas as I'd like his input as well) Sorry for the delay, I was pretty far behind on tracking this ML. > > 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. Indeed that rase a strong point. In all scenarios I have in mind you're analyses is correct. It's binary: a) This is live and then the frame rate and the speed matches b) This is offline processing, and then we'd like to use as much power as possible What I think is important in what you raise, is that unlike the Allegro encoder, other encoders may be able to control usage in a more dynamic ways which indeed is not guarantied to correlate with framerate (could be utilisation driver clock scaling). Then yes, a control to tell the Allegro encoder that we are doing offline processing would work, though it can be optional, as other encoder with more dynamic performance scaling won't need that hint to work. > > 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 share your preference, but that means more userspace backward compatibility code is needed. Notably, I'll have try the new one and fallback for this case to stay compatible with older kernel. > > > > 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
Le vendredi 20 décembre 2019 à 14:47 +0100, Michael Tretter a écrit : > 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. As per today's IRC discussion, adding a new FRAME_RATE control will in the end only move a functionality from one place to another with a different form. If we want to be reasonnable, despite our common dislike of s_parm, I believe we should stay were we are with S_PARM, and just make sure drivers don't use this to scale the HW performance, or not make this the default at least. In the end, the S_PARM will tell the encoder what to do for CBR with a B/s configuration and will allow the driver to calculate the contraints to be written into bitstream headers when supported by the bitstream. What I'm suggesting for now is to scope out this change until we have a better reason to ask userspace folks to port. Is that reasonnable ? And we can forcus on other aspects. > > Michael