Re: [PATCH 0/5] Stateful Encoding: final bits

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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