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

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

(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